如何让我的代码更“专业”？ C＃.NET [英] How do I make my code more "professional"? C# .NET
问题描述
我有我之前创建的这个项目..
这是一个简单的文件分类器，可以完成工作，确保代码很乱并且看起来很糟糕，所以我很想知道。 />
我能做些什么让它看起来更专业？
我应该做些什么不同，以及在创建项目时我应该怎么想？
I have this project that I created the other day..
It's a simple file sorter that get's the job done, sure the code is messy and looks horrible, So I was wonering.
What can I do to make it look more professional?
What should I have done different and what should I think about in the future when creating projects?
using System;
using System.IO;
using System.Windows.Forms;
namespace dropshit
{
public partial class frmMain : Form
{
public frmMain()
{
//Allow the drop feature
AllowDrop = true;
InitializeComponent();
}
private void frmMain_DragEnter(object sender, DragEventArgs e)
{
//If you drag files into the application it reacts with a DragDropEffect
//https://msdn.microsoft.com/en-us/library/system.windows.forms.dragdropeffects(v=vs.110).aspx
if (e.Data.GetDataPresent(DataFormats.FileDrop))
e.Effect = DragDropEffects.Copy;
}
private void frmMain_DragDrop(object sender, DragEventArgs e)
{
//Create a string array to keep all the files in when dropped into the listBox aka lBox
string[] files = (string[])e.Data.GetData(DataFormats.FileDrop);
lBox.Items.AddRange(files);
#region AllFiles
//This is where you would add and or modify all the file extensions it can sort.
//So far it supports .txt .exe .PNG .jpg .lnk .rar
#region TextFiles
#region FolderPaths
//Define a two folderpaths
string txtFolderPath = System.Environment.GetFolderPath(Environment.SpecialFolder.Desktop);
string txtPathFolder = System.IO.Path.Combine(txtFolderPath, "Text Files");
#endregion
//Create a Messagebox with 3 options - Yes, No, Cancel.
DialogResult dr = MessageBox.Show("Would you like to sort the files", "Option", MessageBoxButtons.YesNoCancel);
//if the user presses "Yes"
if (dr == DialogResult.Yes)
{
//For each Text file in the ListBox
foreach (var txtFile in lBox.Items)
{
//Create a string variable that grabs and holds the file extension
string fileExtension = Path.GetExtension(txtFile.ToString());
//if the FileExtension is equal to .txt then we want to do something
if (fileExtension == Path.GetExtension(".txt"))
{
//Create a string and combine the two parameters;
//first being the txtFolderPath and the second being the name of the folder Line: 38
string folderPath = System.IO.Path.Combine(txtFolderPath, "Textfiles");
//create the folder with the name "Textfiles"
System.IO.Directory.CreateDirectory(folderPath);
//If the file extension is equal to .txt
if (fileExtension == Path.GetExtension(".txt"))
{
//create a string to hold the txtFile name
string fileName = txtFile.ToString();
//get the file path
string filePath = System.IO.Path.GetFileName(fileName);
string path = Environment.GetFolderPath(Environment.SpecialFolder.Desktop) + "\\Textfiles";
//Manipulate the two paths together
string targetPath = System.IO.Path.Combine(path, filePath);
//And finally move the file from the original destination to the newly created folder.
//Now rinse and repeat everything for a new File Extension. You can just copy the "exeFiles" region below
//to use as a template, although Keep in mind that it does not have the MessageBox Dialog in it
//Because there is no need to.
if (!File.Exists(targetPath))
{
System.IO.File.Move(fileName, targetPath);
}
}
}
}
#endregion
#region exeFiles
#region FolderPaths
string exeFolderPath = System.Environment.GetFolderPath(Environment.SpecialFolder.Desktop);
string exePathFolder = System.IO.Path.Combine(exeFolderPath, "Executables");
#endregion
foreach (var exeFile in lBox.Items)
{
string fileExtension = Path.GetExtension(exeFile.ToString());
if (fileExtension == Path.GetExtension(".exe"))
{
string folderPath = System.IO.Path.Combine(exeFolderPath, "Executables");
System.IO.Directory.CreateDirectory(folderPath);
if (fileExtension == Path.GetExtension(".exe"))
{
string fileName = exeFile.ToString();
string filePath = System.IO.Path.GetFileName(fileName);
string path = Environment.GetFolderPath(Environment.SpecialFolder.Desktop) + "\\Executables";
string targetPath = System.IO.Path.Combine(path, filePath);
if (!File.Exists(targetPath))
{
System.IO.File.Move(fileName, targetPath);
}
}
}
}
#endregion
#region imgFiles
#region FolderPaths
string imgFolderPath = System.Environment.GetFolderPath(Environment.SpecialFolder.Desktop);
string imgPathFolder = System.IO.Path.Combine(imgFolderPath, "AllImages");
#endregion
foreach (var imgFile in lBox.Items)
{
string fileExtension = Path.GetExtension(imgFile.ToString());
if (fileExtension == Path.GetExtension(".PNG"))
{
string folderPath = System.IO.Path.Combine(imgFolderPath, "Images");
System.IO.Directory.CreateDirectory(imgPathFolder);
if (fileExtension == Path.GetExtension(".PNG"))
{
string fileName = imgFile.ToString();
string filePath = System.IO.Path.GetFileName(fileName);
string path = Environment.GetFolderPath(Environment.SpecialFolder.Desktop) + "\\AllImages";
string targetPath = System.IO.Path.Combine(path, filePath);
if (!File.Exists(targetPath))
{
System.IO.File.Move(fileName, targetPath);
}
}
}
}
foreach (var imgFile in lBox.Items)
{
string fileExtension = Path.GetExtension(imgFile.ToString());
if (fileExtension == Path.GetExtension(".jpg"))
{
string folderPath = System.IO.Path.Combine(imgFolderPath, "AllImages");
System.IO.Directory.CreateDirectory(imgPathFolder);
if (fileExtension == Path.GetExtension(".jpg"))
{
string fileName = imgFile.ToString();
string filePath = System.IO.Path.GetFileName(fileName);
string path = Environment.GetFolderPath(Environment.SpecialFolder.Desktop) + "\\AllImages";
string targetPath = System.IO.Path.Combine(path, filePath);
if (!File.Exists(targetPath))
{
System.IO.File.Move(fileName, targetPath);
}
}
}
}
#endregion
#region lnkFiles
#region FolderPaths
string lnkFolderPath = System.Environment.GetFolderPath(Environment.SpecialFolder.Desktop);
string lnkPathFolder = System.IO.Path.Combine(lnkFolderPath, "Shortcuts");
#endregion
foreach (var lnkFile in lBox.Items)
{
string fileExtension = Path.GetExtension(lnkFile.ToString());
if (fileExtension == Path.GetExtension(".lnk"))
{
string folderPath = System.IO.Path.Combine(lnkFolderPath, "Shortcuts");
System.IO.Directory.CreateDirectory(folderPath);
if (fileExtension == Path.GetExtension(".lnk"))
{
string fileName = lnkFile.ToString();
string filePath = System.IO.Path.GetFileName(fileName);
string path = Environment.GetFolderPath(Environment.SpecialFolder.Desktop) + "\\Shortcuts";
string targetPath = System.IO.Path.Combine(path, filePath);
if (!File.Exists(targetPath))
{
System.IO.File.Move(fileName, targetPath);
}
}
}
}
#endregion
#region rarFiles
#region FolderPaths
string rarFolderPath = System.Environment.GetFolderPath(Environment.SpecialFolder.Desktop);
string rarPathFolder = System.IO.Path.Combine(rarFolderPath, "RarFiles");
#endregion
foreach (var rarFile in lBox.Items)
{
string fileExtension = Path.GetExtension(rarFile.ToString());
if (fileExtension == Path.GetExtension(".rar"))
{
string folderPath = System.IO.Path.Combine(rarFolderPath, "RarFiles");
System.IO.Directory.CreateDirectory(folderPath);
if (fileExtension == Path.GetExtension(".rar"))
{
string fileName = rarFile.ToString();
string filePath = System.IO.Path.GetFileName(fileName);
string path = Environment.GetFolderPath(Environment.SpecialFolder.Desktop) + "\\RarFiles";
string targetPath = System.IO.Path.Combine(path, filePath);
if (!File.Exists(targetPath))
{
System.IO.File.Move(fileName, targetPath);
}
}
}
}
#endregion
#endregion
MessageBox.Show("Sorted " + files.Length + " Files");
}
lBox.Items.Clear();
}
private void frmMain_Move(object sender, EventArgs e)
{
//When minimized we create a BallonTip at the bottom right of our screen
//with a title and a message
if(this.WindowState == FormWindowState.Minimized)
{
this.Hide();
notifyIcon1.ShowBalloonTip(5000, "Title", "Your Message", ToolTipIcon.Info);
}
}
private void notifyIcon1_DoubleClick(object sender, EventArgs e)
{
//When the icon is double clicked we bring the form back up.
this.Show();
this.WindowState = FormWindowState.Normal;
}
private void notifyIcon1_BalloonTipClicked(object sender, EventArgs e)
{
//When the balloonTip is clicked then we bring the form back up.
this.Show();
this.WindowState = FormWindowState.Normal;
}
}
}
我尝试了什么：
我查看了谷歌上的一些帖子，所有人都说评论代码，但仍然让我的代码看起来像一个3岁的人已编码。
What I have tried:
I've looked over some posts on google and all of them say "Comment the code" but that still makes my code look like a 3 year old has coded it.
推荐答案
嗯......名为dropshit的名称空间没有帮助...也没有用代码中的相同信息对代码中的每一行进行注释代码行本身。区域的数量非常愚蠢 - 如果你需要一个区域内的区域，那么区域中的代码可能应该是一个方法。
基本上，代码应该是自我记录：方法，属性和变量名称应该有助于指导读者你正在做什么 - 你不应该在方法内部注释，除非它是一个棘手的位需要额外的解释。但是方法，属性和变量本身肯定可以从XML注释中受益 - 这两者都可以帮助你，因为它被提供给Intellisense，读者可以理解它的方法。
例如：
Well...namespaces called "dropshit" don't help...nor does commenting each line in the code with the same info you get in the code line itself. And the number of regions is pretty silly - if you need a region inside your method, the code in the region should probably be in a method instead.
Basically, code should be self documenting: the method, property and variable names should help guide the reader as to what you are doing - you shouldn't need comments inside methods unless it's a tricky bit that needs extra explanation. But methods, properties, and variables themselves can definitely benefit from XML commenting - this boths helps you because it is fed into Intellisense, and the reader to understand what the method is there for.
For example:
/// <summary>
/// Returns the Root video name from a file
/// (This removes "known" video name extenders such as a "Back"
/// suffix for the image of the back cover and so forth)
/// </summary>
/// <param name="path">Full path to the video or image file</param>
/// <returns>Root video name from extended name</returns>
private string GetRootVideoName(string path)
{
string root = Path.GetFileNameWithoutExtension(path);
if (root.EndsWith(" Back")) root = root.Substring(0, root.Length - 5).Trim();
else if (root.EndsWith(" Front")) root = root.Substring(0, root.Length - 6).Trim();
else if (root.EndsWith(" Both")) root = root.Substring(0, root.Length - 5).Trim();
return root;
}方法内部没有注释 - 因为代码很明显 - 但是请注意方法，以便明确它究竟是什么。
您正在做的评论不这样做：
Has no comments inside the method - because the code is obvious - but comments the method so it's clear what exactly it does.
The commenting you are doing doesn't do that:
//if the user presses "Yes"
if (dr == DialogResult.Yes)
{我可以通过阅读代码来说明条件的作用 - 评论是多余的，以后可能会出错，因为您可能会在以后更改为确定/取消框，评论不太可能更新。不必要的评论往往是一个没有经验的编码器的标志 - 这使它看起来像三个YO写的！ ：笑：
是的，我会降低评论的评论谢谢！
还有..人们发现我很有意思基本上复制粘贴了我制作的第一个扩展区域并将其用作模板，他们说我应该把它变得更简单，缩短它，我不认为那个好的开发人员还没找出哪个最高效 通过使用最小的代码来创建应用程序的方法。我不介意，因为我让应用程序做它应该做的事情，我真的不介意如何让它看起来更专业，通过缩短代码来提高代码的方式。不知道该怎么做，但我很乐意学习！
我使用地区，但我用它们来分组相关方法 - 我可能在一些人的眼中过度使用它们。这是VS为我添加的新添加的WinForms表单的标准表单模板：
I can tell what the condition is doing by reading the code - the comment is redundant and probably wrong later on, because you may change to a "OK / Cancel" box later and it's unlikely the comment will get updated. Commenting unnecessarily is often a sign of an inexperienced coder - and that makes it look like a three YO wrote it! :laugh:
Yeah I will lower down the ammount of comments thanks!
Also.. People found it funny that I basically copy pasted the first extension region I made and used it as a template, they said that I should of made it more simple, shortened it down, I dont think in that good of a developer yet to find out which is the most "Efficient" way of creating a application by using the smallest ammount of code. I didnt mind because I got the application to do what it was supposed to do and I really wouldnt mind how to make it look more "Professional" in a way where the code was a bit more advanced by shortening it down. Not sure how to but I'd love to learn!
I use regions, but I use them to "group" related methods - and I probably overuse them in some peoples eyes. This is a standard form template for a newly added WinForms form that VS adds for me these days:
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Windows.Forms;
using GeneralTesting.UtilityCode;
namespace GeneralTesting
{
/// <summary>
/// Main form for application
/// </summary>
public partial class frmMain : Form
{
#region Constants
#endregion
#region Fields
#region Internal
#endregion
#region Property bases
#endregion
#endregion
#region Properties
#endregion
#region Regular Expressions
#endregion
#region Enums
#endregion
#region Constructors
/// <summary>
/// Default constructor
/// </summary>
public frmMain()
{
InitializeComponent();
}
#endregion
#region Events
#region Event Constructors
#endregion
#region Event Handlers
#region Form
/// <summary>
/// Restore size and location (if the user doesn't
/// override it)
/// </summary>
/// <param name="sender"></param>
/// <param name="e"></param>
private void frmMain_Load(object sender, EventArgs e)
{
if ((ModifierKeys & Keys.Shift) == 0)
{
this.LoadLocation();
}
}
/// <summary>
/// Save the size and location (if the used doesn't
/// override it)
/// </summary>
/// <param name="sender"></param>
/// <param name="e"></param>
private void frmMain_FormClosing(object sender, FormClosingEventArgs e)
{
if ((ModifierKeys & Keys.Shift) == 0)
{
this.SaveLocation();
}
}
/// <summary>
/// Called once when form displayed for first time
/// </summary>
/// <param name="sender"></param>
/// <param name="e"></param>
private void frmMain_Shown(object sender, EventArgs e)
{
}
#endregion
#endregion
#endregion
#region Threads
#endregion
#region Public Methods
#endregion
#region Overrides
#endregion
#region Private Methods
#endregion
}
}
You can see that I group related items so they are easy to find, but easy to collapse out of sight when I don’t need them.
You can see that I group related items so they are easy to find, but easy to collapse out of sight when I don't need them.
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,FormMainorMainFormwould 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 usePath.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
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,FormMainorMainFormwould 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 usePath.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.
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.
这篇关于如何让我的代码更“专业”？ C＃.NET的文章就介绍到这了，希望我们推荐的答案对大家有所帮助，也希望大家多多支持IT屋！
