BladeLogan Ответов: 2

Как сделать свой код более "профессиональным"? C# .NET


У меня есть проект, который я создал на днях..
Это простой сортировщик файлов, который делает свою работу, конечно, код грязный и выглядит ужасно, так что я победил.
Что я могу сделать, чтобы это выглядело более профессионально?
Что я должен был сделать по-другому и о чем мне следует думать в будущем при создании проектов?


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;
        }
    }
}


Что я уже пробовал:

Я просмотрел несколько постов в google, и все они говорят: "прокомментируйте код", но это все равно делает мой код похожим на то, что его закодировал 3-летний ребенок.

Suvendu Shekhar Giri

"...мой код выглядит так, как будто его закодировал 3-летний ребенок."
Я так не думаю :)

BladeLogan

О!
Ну спасибо, сэр! :Д

P_Z

Привет,
private void frmMain_DragEnter(Object sender, DragEventArgs e) слишком длинный и помимо того, что его ужасно поддерживать, большая часть функций повторяется. Вы можете легко создать другой метод, который принимает расширение, путь к папке и т.д. и повторно использовать метод в соответствии с расширениями файлов

2 Ответов

Рейтинг:
17

OriginalGriff

Ну...пространства имен, называемые "dropshit", не помогают...как и комментирование каждой строки кода с той же информацией, которую вы получаете в самой строке кода. А количество регионов довольно глупо - если вам нужен регион внутри вашего метода, то код в этом регионе, вероятно, должен быть в методе.
В принципе, код должен быть самодокументируемым: имена методов, свойств и переменных должны помочь читателю понять, что вы делаете - вам не нужны комментарии внутри методов, если только это не сложный бит, требующий дополнительного объяснения. Но сами методы, свойства и переменные определенно могут извлечь выгоду из XML - комментирования-это помогает вам, потому что оно подается в Intellisense, и читатель понимает, для чего существует этот метод.
Например:

/// <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;
    }
Не имеет комментариев внутри метода-потому что код очевиден , - но комментирует метод, чтобы было ясно, что именно он делает.
Комментирование, которое вы делаете, не делает этого:
//if the user presses "Yes"
if (dr == DialogResult.Yes)
{
Я могу сказать, что делает это условие, прочитав код - комментарий является избыточным и, вероятно, неправильным позже, потому что позже вы можете перейти на поле "ОК / Отмена", и вряд ли комментарий будет обновлен. Излишнее комментирование часто является признаком неопытного кодера - и это делает его похожим на то, что его написал три йо! :смеяться:

Да, я опущу вниз количество комментариев, Спасибо!
Также.. Люди находили забавным, что я в основном копирую вставленную мной первую область расширения и использую ее в качестве шаблона, они говорили, что я должен сделать ее более простой, сократить ее, я не думаю, что это хорошо для разработчика, чтобы выяснить, какой самый "эффективный" способ создания приложения с использованием наименьшего количества кода. Я не возражал, потому что получил приложение, чтобы делать то, что оно должно было делать, и я действительно не возражал бы, как сделать его более "профессиональным" таким образом, чтобы код был немного более продвинутым, сократив его. Не знаю, как это сделать, но мне бы очень хотелось научиться!


Я использую регионы, но я использую их для "группирования" связанных методов - и я, вероятно, злоупотребляю ими в глазах некоторых людей. Это стандартный шаблон формы для недавно добавленной формы WinForms, которую VS добавляет Для меня в эти дни:
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
        }
    }

Вы можете видеть, что я группирую связанные элементы, чтобы их было легко найти, но легко свернуть с глаз долой, когда они мне не нужны.


BladeLogan

Да, я опущу вниз количество комментариев, Спасибо!
Также.. Люди находили забавным, что я в основном копирую вставленную мной первую область расширения и использую ее в качестве шаблона, они говорили, что я должен сделать ее более простой, сократить ее, я не думаю, что это хорошо для разработчика, чтобы выяснить, какой самый "эффективный" способ создания приложения с использованием наименьшего количества кода. Я не возражал, потому что получил приложение, чтобы делать то, что оно должно было делать, и я действительно не возражал бы, как сделать его более "профессиональным" таким образом, чтобы код был немного более продвинутым, сократив его. Не знаю, как это сделать, но мне бы очень хотелось научиться!

OriginalGriff

Ответ обновлен

BladeLogan

Этот взгляд уже намного лучше моего!
Точнее, то, что говорили люди..

"- Вы должны использовать многоразовую "пустоту", класс, помощник или другое какое-то дерьмо для ваших папок: Код является C&P и изменяет только расширение файла, так что у вас в основном есть 20 строк для txt-файлов, "те же" 20 строк для exe-файлов, другие "те же" 20 строк для jpg, то же самое для rars... "

Видите ли вы в этом проблему? Должен ли я делать то, что он говорит, или в этом нет необходимости?

OriginalGriff

Повторять один и тот же код только с изменением имени папки не очень хорошая идея - рефакторинг его в метод, который вы передаете пару параметров, и он выполняет обе работы, - это лучшая идея-хотя бы потому, что если вам нужно изменить его позже, это означает, что он нуждается в изменениях только в одном месте, так что это "лучше", так как вы не можете забыть изменение или сделать ошибку. Написание метода общего назначения и вызов его из нескольких мест сделают ваш код короче, надежнее и более читабельным.

И почему вы используете рабочий стол? Заполни мой рабочий стол папками, и я начну медленно убивать тебя... :смеяться:
Есть места и получше:
https://www.codeproject.com/Tips/370232/Where-should-I-store-my-data
И если они нужны вашему пользователю, добавьте ярлыки на рабочий стол.

BladeLogan

Да, это имеет смысл! Спасибо, что объяснили это и не были грубы!
И спасибо за ссылку, очень полезно! :Д

OriginalGriff

Пожалуйста!

Рейтинг:
1

Philippe Mori

Ну, этот код гораздо больше похож на код школьного проекта, чем на профессиональный! 

Вот мои предложения:

#1 следует номенклатуре языка / фреймворка. В .NET оболочка Pascal обычно используется для имен классов и функций (ClassName, FunctionName). Переменные должны использовать верблюжий корпус (someVariable).

#2 Избегайте аббревиатур. В вашем случае, FormMain или MainForm это было бы лучшим названием класса.

#3 Избегайте бесполезных комментариев это повторяет код.

#4 избегайте длинных функций. Одна из рекомендаций заключается в том, что функции должны помещаться на экране. Если вам нужно прокрутить, то ваша функция, вероятно, слишком длинная. См. также пункт №16.

#5 Избегайте чрезмерного использования регионов. Моя рекомендация для формы, как правило, должна иметь один регион для частной реализации до начала событий. Я не рекомендую использовать регион для событий, чтобы вам не пришлось перемещать автоматически сгенерированный код.

В вашем случае многие регионы существуют потому, что вы дублируете код (см. #8 и #10). Это действительно плохое использование регионов.

#6 более надежная обработка имен файлов. Если вы хотите протестировать расширение, не думайте, что оно строчное. Вместо этого сделайте сравнение без учета регистра. Кроме того, не объединяйте каталог с помощью + оператор и жестко закодированный \. Всегда использовать Path.Combine.

#7 Используйте лучшие имена переменных. У вас есть много переменных с похожими именами (path, filePath, fileName...) и иногда именование даже несущественно (переменная с именем path в нем содержит имя файла, в то время как переменная с именем содержит путь). Тот факт, что вы функционируете очень долго и делаете больше, чем одну вещь, не помогает.

#8 не повторяйтесь (сухо) Код для каждого типа файлов очень похож. Вы должны написать функцию, которая содержала бы общий код.

И даже этот код имеет много дублированного кода (условие, комбинирующий путь...).

#9 не жестко заданных констант. Предпочтительнее помещать константы в объявление класса, а не в саму функцию. На самом деле, в вашем случае, вероятно, было бы предпочтительнее загрузить информацию о типе файла из какой-то конфигурации (или, возможно, из массива).

Это еще хуже в вашем случае, поскольку вы повторяете жестко закодированные константы несколько раз (для каждого типа файла). Поэтому, если вы решите изменить целевую папку, например, вам придется сделать это изменение по крайней мере дважды.

#10 избегайте избыточных условий. Выполнение теста

if (fileExtension == Path.GetExtension(".exe")) { ... }
дважды это бесполезно и только докажет, что вы не понимаете, что делаете. В вашем коде второе условие всегда не нужно.

#11 переместитесь за пределы кода цикла, что должно быть сделано только один раз. В вашем случае, когда вы перемещаете файлы, вы пытаетесь создать каталог на каждой итерации. Как правило, каталог создается один раз.

#12 Не используйте Yes/No / Cancel, если есть только 2 результата. В вашем случае, если вы представите 3 варианта, то Да будет сортировать файл, Нет сделал бы обработку без сортировки файлов и Отменить отменит операцию сброса. В вашем случае ОК / отмена может быть лучшим выбором. Да/Нет-это лучший выбор, когда вы хотите убедиться, что пользователь действительно прочитал вопрос... Когда-нибудь вам, возможно, придется скорректировать текст вашего вопроса в соответствии с выбором.

#13 Используйте оператор switch, если у вас есть несколько результатов диалога. Я думаю, что это облегчает просмотр каждого возможного результата из диалога. Однако, если код для одного случая больше, чем несколько строк, вы действительно должны поместить этот код в функцию.

#14 минимизация в системном трее должна быть настраиваемой пользователем. Я бы вообще рекомендовал, чтобы нестандартное поведение было необязательным. В случае, подобном этому, я бы сказал, что даже если у вас сейчас есть только один вариант, лучше всего использовать (частное) свойство, чтобы решить, следует ли выполнять пользовательский код.

#15 используйте утверждения. Учитывая предложение №13, я обычно думаю, что это хорошая идея добавить default: случай к оператору switch с a Debug.Assert(false); оператор для обнаружения случая, когда код не был бы обновлен после изменения отображаемых кнопок.

#16 следует принципу SRP. Класс или функция должны иметь единую ответственность.

#17 изучите твердые принципы проектирования. Кроме того, вы также можете узнать о TDA.

#18 поместите локализуемые строки в ресурсы.

#19 всегда переименовывайте элементы управления в конструкторе. Имя как notifyIcon1 не рекомендуется. В какой-то момент у вас может быть более одного элемента управления данного типа. Это особенно важно для кнопок. Кроме того, учитывая, что имена обработчиков событий по умолчанию и локализованные свойства зависят от имени элемента управления, лучше правильно назвать их заранее.

#20 инициализируйте значения по умолчанию в конструкторе. В вашем случае, AllowDrop инициализируется в конструкторе, но не зависит ни от какого аргумента, поэтому я бы предложил установить это свойство непосредственно в конструкторе.

Я бы инициализировал свойства в конструкторе, если они зависят от параметров конструктора или для некоторых сложных элементов управления (например, диаграмм или таблиц), иногда это может быть предпочтительнее.

Как правило, вы либо хотите выполнить всю инициализацию в конструкторе, либо вообще не используете конструктор и пишете свой собственный код. Смешивание того и другого может привести к путанице, поскольку некоторые свойства устанавливаются в одном месте, а другие-в другом.

# 21 избегайте смешивания пользовательского интерфейса и бизнес-кода. Иметь код плотно связанным с пользовательским интерфейсом-не очень хорошая идея. Если вы решите перенести код в другой фреймворк (WPF, UWP, ASP.NET...), то вы сможете повторно использовать код, который перемещает файлы, даже если после того, как подтверждение было сделано, код на самом деле больше не является пользовательским интерфейсом, пока он не будет завершен.

И даже если вы остаетесь с Windows Forms, в какой-то момент Вы можете решить, что в дополнение к такой обработке вы можете позволить пользователю выбирать файлы с помощью диалогового окна открытия файла (или диалогового окна выбора папки).

#22 обрабатывайте ошибки по мере необходимости. Возможно, вам захочется явно обработать некоторые ошибки, такие как файл, который не может быть перемещен или недостаточно свободного места на диске. Вы должны принять соответствующие меры в зависимости от того, что вы хотите сделать в таких случаях.

#23 используйте несколько файлов. Как следует из пункта 21, Вы должны переместить код, который управляет файлами, в свой собственный класс (а еще лучше-в сборку бизнес-уровня).

#24 используйте имя вашего приложения в качестве заголовка ваших сообщений. Это, как правило, хорошая идея, поскольку она дает понять, какое приложение генерирует сообщение, если несколько приложений работают одновременно.

#25 подумайте дважды, прежде чем использовать копирование и вставку. Если у вас есть один и тот же код, дублированный во многих местах, то, возможно, это должна быть функция.

И кстати, Visual Studio IDE поддерживает метод извлечения a из кода C#, так что это очень легко сделать.