Well, that code look a lot more like school project code than professional code!
Here are my suggestions:
#1 Follows the nomenclature of the language/framework. In .NET, Pascal casing is normally used for classes and functions names (
ClassName,
FunctionName) . Variables should use camel casing (
someVariable).
#2 Avoid abbreviation. In your case,
FormMain or
MainForm would be a better class name.
#3 Avoid useless comments that repeat the code.
#4 Avoid long functions. One recommandation is that functions should fit on the screen. If you have to scroll, then your function is probably too long. See also item #16.
#5 Avoir overuse of regions. My recommandation for a form would generally to have one region for private implementation before the events. I do not recommend using a region for events so that you don't have to move automatically generated code.
In your case, many of the regions are there because you duplicate code (see #8 and #10). This is a really bad use of regions.
#6 More robust file name handling. If you want to test an extension, don't assume it is lowercase. Do case insensitive comparison instead. Also, do not combine directory using
+ operator and hard-coded
\. Always use
Path.Combine.
#7 Use better variable names. You have a lot of variable with similar names (
path,
filePath,
fileName...) and sometime naming is even inconsistant (variable with path in it name contains file name while the variable with name contains the path). The fact that you function is very long and do more than one thing does not help.
#8 Don't repeat yourself (DRY). The code for each file types is very similar. You should write a function that would contains the common code.
And even that code have a lot of code duplicated (condition, combining path...).
#9 Avoid hard-coded constants. It is preferable to put constants in the class declaration and not in the function themselves. In fact, in your case, it would probably be preferable to load information about file type from some configuration instead (or maybe from an array).
This is even worst in your case as you are repeating hardcoded constants multiple times (for each file type). So if you decide to change the target folder for example, you have to make the change at least twice.
#10 Avoir redundant conditions. Doing the test
if (fileExtension == Path.GetExtension(".exe")) { ... } twice is useless and would only prove that you don't understand what you are doing. In your code, the second condition is always unnecessary.
#11 Move outside of the loop code that should be done only once. In your case, when you move files, you try to create the directory on each iteration. Generally, one would create the directory once.
#12 Don't use Yes/No/Cancel if there are only 2 outcomes. In your case, if you present 3 choices, the
Yes would sort the file,
No would do the processing without sorting files and
Cancel would cancel the drop operation. In your case, OK/Cancel might be the best choice. Yes/No is the best choice when you want to ensure that the user really read the question... Sometime, you might have to adjust the text of your question to fit the choices.
#13 Use switch statement if you have multiple dialog results. I think, it make it easier to see each possible outcome from the dialog. However, if the code for one case is more than a few lines, you should really put that code in a function.
#14 Minimizing to system tray should be user configurable. I would generally recommend that non standard behavior should be optional. In a case like this one, I would say that even if you have only one option now, it is best to use a (private) property to decide if the custom code should be executed.
#15 Use assertions. Given suggestion #13, I usually think it is a good idea to add
default: case to the switch statement with a
Debug.Assert(false); statement to detect the case where code would have not been updated after changing displayed buttons.
#16 Follows SRP principle. A class or a function should have a single responsability.
#17 Learn SOLID principles design. In addition, you might also want to learn about TDA.
#18 Put localizable strings in ressources.
#19 Always rename controls in the designer. Name like
notifyIcon1 are not recommended. At some point, you might have more than one control of a given type. This is particularly important for buttons. Also given that event handlers default names and localized properties depend on the control name, it is better to properly name them early.
#20 Initialize defaults in the designer. In your case,
AllowDrop is initialized in the constructor but does not depends on any argument, thus I would suggest to set that property directly in the designer.
I would initialize properties in the constructor if they depends on constructor parameters or for some complex controls (like charts or tables), sometime that might be preferable.
Generally, you either want to do all initialization in designer or don't user the designer at all and write your own code. Mixing both can be confusing as some property are set at one place and other at the other.
#21 Avoid mixing UI and business code. Having the code tightly coupled to the UI is not a good idea. If you decide to port the code to another framework (WPF, UWP, ASP.NET...), then you would be able to reuse the code that move files even though once the confirmation have been made, the code does not really the UI anymore until it is completed.
And even if you stay with Windows Forms, at some point, you might decide that in addition to do that kind of processing, you might allows the user to select files with a File open dialog (or Select folder dialog).
#22 Handle errors as appropriate. You might want to handle explicitly some errors like a file that cannot be moved or not enough available disk space. You have to take appropriate action depending on what you want to do in such cases.
#23 Use multiple files. As implied by item #21, you should move the code that manage files in its own class (and even better in the business layer assembly).
#24 Use your application name as the title of your message boxes. This is generally a good idea as it make it clear which application generate the message if multiple application are running at the same time.
#25 Think twice before using Copy and paste. If you have the same code duplicated at many location, then maybe it should be a function instead.
And by the way, Visual Studio IDE support the extract a method from C# code so this is very easy to do.
Submit an article or tip