Пишу для себя программу, которая разгребает каталог(например: рабочий стол) и раскидывает
файлы по каждому формату в заданный пользователем каталог (например: jpg кидает в папку
"Изображения", mp3 в папку "музыка", zip и rar в каталог архивы и т.д.).
Программа работает, для полного завершения осталось написать сервис чтобы работала
автономно (например как антивирус). Но для того чтобы приступить к разработке сервиса,
хочу попросить Вас сделать код-ревью, т.к. показать, к сожалению, некому. Все коллеги
разрабатывают ETL и далеки от данной тематике.
Буду Вам очень признателен, т.к. очень хочу быть профессиональным разработчиком.
Класс, где хранятся все параметры:
namespace Transporter
{
public class Data
{
public Data()
{
string fileName = settingsFile;
if (File.Exists(fileName) && !File.ReadAllText(fileName).Equals("") && File.ReadAllText(fileName)
!= null)
{
XElement xElem2 = XElement.Load(fileName);
formats = xElem2.Descendants("item")
.ToDictionary(x => (string)x.Attribute("format"),
x => (string)x.Attribute("path"));
}
else
{
formats = new Dictionary();
}
}
private static string settingsFile = "settings.xml";
private readonly Dictionary formats;
public Dictionary Formats
{
get { return formats; }
}
private string path = null;
public string Path
{
get { return path; }
set { path = value; }
}
public void Add(string format, string path)
{
if (!formats.ContainsKey(format))
formats.Add(format, path);
else
{
formats.Remove(format);
formats.Add(format, path);
}
}
public void Save()
{
string fileName = settingsFile;
if (File.Exists(fileName)) File.Delete(fileName);
using (FileStream fileStream = new FileStream(fileName, FileMode.Create))
{
XElement xElem = new XElement
(
"items",
formats.Select(x => new XElement("item", new XAttribute("format", x.Key),
new XAttribute("path", x.Value)))
);
xElem.Save(fileStream);
fileStream.Close();
}
}
public void RemoveSettingsFile()
{
if (File.Exists(settingsFile)) File.Delete(settingsFile);
}
}
public class item
{
[XmlAttribute]
public string path;
[XmlAttribute]
public string format;
}
}
Класс, который обрабатывает параметры класса Data:
namespace Transporter
{
class Handler
{
private Data data = new Data();
public void AddFormat(string format,string path)
{
data.Add(format, path);
}
public void AddPath(string path)
{
data.Path = path;
}
public void DelAllFormats()
{
data.Formats.Clear();
data.RemoveSettingsFile();
}
public string GetParameters()
{
StringBuilder builder = new StringBuilder();
builder.AppendLine("Исходный каталог:");
builder.AppendLine(data.Path == null ? "исходный каталог не указан" : data.Path);
builder.AppendLine();
builder.AppendLine("Форматы файлов и каталоги, куда файлы будут перенесены:");
if (data.Formats.Count == 0)
builder.AppendLine("Форматы файлов и каталоги не указаны");
else
{
foreach (var format in data.Formats)
{
builder.AppendLine(string.Format("{0} => {1}", format.Key, format.Value));
}
}
return builder.ToString();
}
public void MoveFiles(CancellationToken token)
{
while (!token.IsCancellationRequested)
{
try
{
var files = Directory.GetFiles(data.Path);
foreach (string file in files)
{
foreach (var format in data.Formats)
{
var fileInfo = new FileInfo(file);
if (fileInfo.Extension.ToLower().Contains(format.Key.ToLower()))
{
fileInfo.MoveTo(format.Value + @"\" + DateTime.Now.ToString("yyyyMMdd_hhmmss_")
+ fileInfo.Name);
}
}
}
Thread.Sleep(5000);
}
catch (IOException) { }
catch (KeyNotFoundException) { }
}
}
public void SaveSetting()
{
data.Save();
}
public bool checkParametrsForStart()
{
return data.Path != null && data.Formats.Count > 0;
}
}
}
Форма:
namespace Transporter
{
public partial class Transporter : Form
{
private Handler handler = new Handler();
private CancellationTokenSource cancellationTokenSource;
public Transporter()
{
InitializeComponent();
StringBuilder builder = new StringBuilder();
builder.AppendLine("Для начала работы укажите:");
builder.AppendLine("1) исходный каталог - откуда бы Вы хотели перенести файлы;");
builder.AppendLine("2) форматы файлов с каталогами - куда бы Вы xотели переместить
файлы.");
builder.AppendLine();
builder.AppendLine("Кнопка 'Start' запускает перенос файлов.");
builder.AppendLine("Кнопка 'Stop' останавливает перенос файлов.");
builder.AppendLine("Кнопки 'Add' добавлют параметры.");
builder.AppendLine("Кнопка 'Reset' сбрасывает параметры переноса.");
builder.AppendLine("Кнопка 'View' выводит текущие параметры в консоль.");
builder.AppendLine("Кнопка 'Clear' очищает консоль.");
builder.AppendLine("Кнопка 'Save' сохраняет текущие параметры.");
builder.AppendLine();
builder.AppendLine("Автор программы: Rumeet94 (https://vk.com/evgengorb)");
tbMessage.Text = builder.ToString();
}
private void BtnAddDiir_Click(object sender, System.EventArgs e)
{
if (Regex.IsMatch(tbDir.Text, @"^.:\w*", RegexOptions.IgnoreCase))
{
handler.AddPath(tbDir.Text);
tbMessage.Text = "Путь исходного каталога успешно добавлен";
}
else
{
tbMessage.Text = "Путь исходного каталога указан некорректно";
}
}
private void BtnAddFormatAndDir_Click(object sender, System.EventArgs e)
{
if (Regex.IsMatch(tbFormatDir.Text, @"^.:\w*", RegexOptions.IgnoreCase) &&
!tbFormat.Text.Equals(""))
{
handler.AddFormat(tbFormat.Text, tbFormatDir.Text);
tbMessage.Text = "Формат и путь каталога для переноса файлов успешно
добавлены";
}
else
{
tbMessage.Text = "Не указан формат или путь каталога для переноса файлов
указан некорректно";
}
}
private void BtnStop_Click(object sender, System.EventArgs e)
{
tbMessage.Text = "Перенос файлов остановлен.";
cancellationTokenSource?.Cancel();
tbDir.ReadOnly = false;
tbFormat.ReadOnly = false;
tbFormatDir.ReadOnly = false;
btnStart.Enabled = true;
btnAddDiir.Enabled = true;
btnAddFormatAndDir.Enabled = true;
btnResPar.Enabled = true;
}
private void BtnStart_Click(object sender, System.EventArgs e)
{
if (handler.checkParametrsForStart()) {
tbDir.ReadOnly = true;
tbFormat.ReadOnly = true;
tbFormatDir.ReadOnly = true;
btnStart.Enabled = false;
btnAddDiir.Enabled = false;
btnAddFormatAndDir.Enabled = false;
btnResPar.Enabled = false;
cancellationTokenSource = new CancellationTokenSource();
tbMessage.Text = "Выполняется перенос файлов";
Task.Run(() => handler.MoveFiles(cancellationTokenSource.Token));
}
else
{
tbMessage.Text = "Невозможно запустить перенос файлов, т.к. исходный
каталог " +
"или форматы и каталоги для переноса файлов не указаны";
}
}
private void BtnViewPar_Click(object sender, System.EventArgs e)
{
tbMessage.Text = handler.GetParameters();
}
private void BtnResPar_Click(object sender, System.EventArgs e)
{
handler.DelAllFormats();
}
private void BtnClearCon_Click(object sender, System.EventArgs e)
{
tbMessage.Text = "";
}
private void BtnSave_Click(object sender, System.EventArgs e)
{
handler.SaveSetting();
}
}
}
Ответы
Ответ 1
Из того, что мне сразу бросилось в глаза:
Класс, где хранятся все параметры. Можно было работать с app.config и использовать
штатные возможности, предоставляемые языком. Вместо этого используется кастомный XML
файл. Почему бы не начать хотя бы ConfigurationManager.AppSettings["SettingsName"]?
Что-то стояло за этим выбором?
Метод GetParameters в классе Handler одновременно читает настройки (должно быть:
Init в Settings?), валидирует (ValidateConfig) и форматирует текст сообщения об ошибках.
Пустые catch без объяснения -- нехорошо:
catch (IOException) { }
catch (KeyNotFoundException) { }
В конструкторе Transporter можно выделить отдельный метод:
StringBuilder builder = new StringBuilder();
builder.AppendLine("Для начала работы укажите:");
builder.AppendLine("1) исходный каталог - откуда бы Вы хотели перенести файлы;");
builder.AppendLine("2) форматы файлов с каталогами - куда бы Вы xотели переместить
файлы.");
builder.AppendLine();
builder.AppendLine("Кнопка 'Start' запускает перенос файлов.");
builder.AppendLine("Кнопка 'Stop' останавливает перенос файлов.");
builder.AppendLine("Кнопки 'Add' добавлют параметры.");
builder.AppendLine("Кнопка 'Reset' сбрасывает параметры переноса.");
builder.AppendLine("Кнопка 'View' выводит текущие параметры в консоль.");
builder.AppendLine("Кнопка 'Clear' очищает консоль.");
builder.AppendLine("Кнопка 'Save' сохраняет текущие параметры.");
builder.AppendLine();
builder.AppendLine("Автор программы: Rumeet94 (https://vk.com/evgengorb)");
tbMessage.Text = builder.ToString();
Сразу будет понятно, что конструктор делает два действия (InitializeComponent и GetTextForAboutButton),
кому нужны детали как выглядит этот метод -- кликнет и перейдёт.
Кстати, я не вижу никакого обоснования зачем вам понадобился string builder -- у
вас же константа:
const string About = @"Для начала работы укажите:
1) исходный каталог - откуда бы Вы хотели перенести файлы;
2) форматы файлов с каталогами - куда бы Вы xотели переместить файлы.
Кнопка 'Start' запускает перенос файлов.
Кнопка 'Stop' останавливает перенос файлов.
Кнопки 'Add' добавлют параметры.
Кнопка 'Reset' сбрасывает параметры переноса.
Кнопка 'View' выводит текущие параметры в консоль.
Кнопка 'Clear' очищает консоль.
Кнопка 'Save' сохраняет текущие параметры.
Автор программы: Rumeet94 (https://vk.com/evgengorb)";
Это конечно здорово, что вас научили пользоваться StringBuilder для конкатенации
строк, но тут пока оверхед: не надо каждый раз при запуске программы вычислять эту
строку, достаточно один раз при компиляции ПО.
Здесь напрашивается функция с говорящим именем IsDirectoryNameValid(tbDir.Text):
if (Regex.IsMatch(tbDir.Text, @"^.:\w*", RegexOptions.IgnoreCase))
{
handler.AddPath(tbDir.Text);
tbMessage.Text = "Путь исходного каталога успешно добавлен";
}
else
{
tbMessage.Text = "Путь исходного каталога указан некорректно";
}
И уже внутри деталь реализации, что это делается регулярками.
Вот это -- некоторая магическая константа:
Thread.Sleep(5000);
Нужно ей дать имя, хотя бы TaskDelayBetweenFiles и сделать для начала хотя бы константой
класса (хотя лучше сразу в конфиг-файл вынести!), а кроме того, хорошо бы придумать
такое имя, из которого было бы сразу стало понятно, зачем нужна эта задержка. Сейчас
вот -- непонятно.
Вот тут решарпер обычно выдаёт что можно сделать readonly:
private Handler handler = new Handler();
private CancellationTokenSource cancellationTokenSource;
private Data data = new Data();
И у вас в некоторых местах так сделано.
Вот тут .Close() не нужен:
using (FileStream fileStream = new FileStream(fileName, FileMode.Create))
{
...
fileStream.Close();
}
Попробуйте просто кликнуть по Close и посмотреть, как выглядят исходники этого метода:
/// Closes the current stream and releases any resources (such as sockets
and file handles) associated with the current stream. Instead of calling this method,
ensure that the stream is properly disposed.
public virtual void Close()
{
this.Dispose(true);
GC.SuppressFinalize((object) this);
}
/// Releases all resources used by the .
[__DynamicallyInvokable]
public void Dispose()
{
this.Close();
}
У вас блок using который автоматически вызовет .Dispose() и из него - .Close().
Вот это одна из самых типовых ошибок новичков, которые просто не видят в упор эти
пустые строки:
private void BtnAddFormatAndDir_Click(object sender, System.EventArgs e)
{
if (Regex.IsMatch(tbFormatDir.Text, @"^.:\w*", RegexOptions.IgnoreCase) && !tbFormat.Text.Equals(""))
{
handler.AddFormat(tbFormat.Text, tbFormatDir.Text);
tbMessage.Text = "Формат и путь каталога для переноса файлов успешно добавлены";
}
else
{
tbMessage.Text = "Не указан формат или путь каталога для переноса файлов
указан некорректно";
}
}
(посмотрите внимательно между двумя последними фигурными скобками) Таких мест у вас
несколько, вы их визуально не считываете.
Посмотрите например в конец класса и обратите внимание на пустую строку между фигурной
скобкой класса и фигурной скобкой последнего метода. А, показалось: у вас там вообще
конец класса и неймспейса.
Также вы ставите порой две пустых строки между методами - а зачем? Ставьте одну:
}
private void BtnStop_Click(object sender, System.EventArgs e)
{
Ещё в тему отступов: у вас нет отступа внутрь Namespace:
namespace Transporter
{
public class Data
И вероятно, лучше настроить assembly name чтобы неймспейс был CompanyName.ProjectName
или ProjectName хотя бы.
private void BtnStop_Click(object sender, System.EventArgs e)
{
Вот тут не хватает фигурных скобок в if:
if (!formats.ContainsKey(format))
formats.Add(format, path);
else
{
formats.Remove(format);
formats.Add(format, path);
}
if (File.Exists(settingsFile)) File.Delete(settingsFile);
Да, они необязательны. И тут надо чётко понимать, что лучше их поставить сразу. Такие
вещи должны быть зафиксированы в любом виде в кодстайле, чтобы не было пустых споров
"а мне так нравится" и все писали единообразно, даже если этот пишете якобы только
вы один. (Якобы -- потому что через полгода читать код будете не "я текущий", а "я
через полгода", это совсем разные люди) Вот мне лично тоже больше нравится опускать
эти скобки, но в большинстве кодстайлов я видел зафиксированной их обязательность.
Из плюсов: чище будет diff если вы когда-нибудь вынуждены будете добавить что-то
там: при расставленных фигурных скобках будет просто одна строка добавлена.
Вот эти две портянки кода нужно убрать в метод DisableButtons и EnableButtons:
btnStart.Enabled = true;
btnAddDiir.Enabled = true;
btnAddFormatAndDir.Enabled = true;
btnResPar.Enabled = true;
tbDir.ReadOnly = true;
tbFormat.ReadOnly = true;
tbFormatDir.ReadOnly = true;
btnStart.Enabled = false;
btnAddDiir.Enabled = false;
btnAddFormatAndDir.Enabled = false;
btnResPar.Enabled = false;
И ваш код сразу станет чище и понятнее, а также не нужно будет его постоянно в разных
местах так подробно расписывать.
Почему имя класса с маленькой буквы?
public class item
И сюда же:
formats.Select(x => new XElement("item", new XAttribute("format", x.Key)
Зачем вы захардкодили строки? Меняем их на nameof(item) и nameof(item.format).
Ответ 2
Предупреждаю сразу, что я не профессиональный инспектор кода, но знаком с некоторыми
рекомендациями по разработке приложений с публичным API под .NET, их и некоторые другие
опишу ниже.
private static string settingsFile = "settings.xml";
Возможно, стоило добавить модификатор readonly или вообще вынести в константу через
const.
private readonly Dictionary formats;
public Dictionary Formats
{
get { return formats; }
}
Вместо класса Dictionary как тип свойства стоит использовать интерфейс
IDictionary, потому что нужно стараться уходить от конкретных реализаций
к интерфейсам, когда это возможно.
private string path = null;
public string Path
{
get { return path; }
set { path = value; }
}
Имеет смысл заменить на автосвойство, потому что у вас нет никакой дополнительной
логики ни в геттере, ни в сеттере, но это больше дело вкуса, чем рекомендация.
public class item
{
[XmlAttribute]
public string path;
[XmlAttribute]
public string format;
}
Публичные классы и публичные члены классов должны называться в PascalCase, т.е. стоило
бы переименовать класс в Item, а поля в Path и Format. Еще лучше было бы сделать поля
свойствами, потому что если бы вы вдруг отправили API пользователям с публичными полями,
а потом решили бы поменять их на свойства, вы не смогли бы сделать это без потери обратной
совместимости с прошлыми версиями. Несмотря на то, что члены бы имели те же самые имена,
пользовательские приложения не смогли бы работать с вашей библиотекой без повторной
компиляции, потому что поля и свойства для CLR - не одно и то же.
Кржиштоф Цвалина, Брэд Абрамс - Инфраструктура программных проектов (Microsoft
.NET Development)
Хорошая книга, в которой описаны особенности разработки публичных API на CLR-совместимых
языках.
Ответ 3
Коллеги указали на часть ошибок, но есть и другие. Опишу их.
Конструктор класса Data.
if (File.Exists(fileName) && !File.ReadAllText(fileName).Equals("") && File.ReadAllText(fileName)
!= null)
Проверка File.Exists не гарантирует, что файл окажется на месте. Он может быть удалён
уже после проверки, но раньше доступа к нему. Поэтому желательно обернуть код в try-catch
с перехватом исключений. Возможны FileNotFoundException, теоретически UnauthorizedAccessException...
Чтение одного и того же файла выполняется дважды. Это дорогая и медленная операция.
Проверка на null выполняется последней. Следовательно, при вызове .Equals можно было
бы словить NullReferenceException, если бы ReadAllText вернул null.
Потом файл читается в третий раз при вызове XElement.Load(fileName). Ужас!
И если там будет невалидный xml - опять возможны исключения.
Странные действия в методе Add в ветке else. Достаточно:
formats[format] = path;
Метод Save.
Необязательно удалять файл, т. к. перегрузка new FileStream(fileName, FileMode.Create),
по сути, сама это сделает благодаря FileMode.Create.
В надёжном софте (а ваш сервис, как я понимаю, будет работать постоянно) все операции
ввода-вывода должны позаботиться об обработке исключений. Диск в любой момент может
сбойнуть, на нём может закончиться место и т. п.
Атрибуты [XmlAttribute] нужны только для xml-сериализации. Вы не используете XmlSerializer,
а делаете парсинг и составление xml вручную. Следовательно, эти атрибуты не нужны.
Посмотрим класс Handler.
Метод GetParameters.
Я сторонник лаконичности. Переписал бы начало так:
var builder = new StringBuilder()
.AppendLine("Исходный каталог:")
.AppendLine(data.Path ?? "исходный каталог не указан")
.AppendLine()
.AppendLine("Форматы файлов и каталоги, куда файлы будут перенесены:");
В цикле foreach, вместо жутко неэффективного string.Format, написал бы так:
builder.Append(format.Key).Append(" => ").AppendLine(format.Value);
Конечно, это такая оптимизация, что и в микроскоп не разглядишь, но раз уж мы используем
StringBuilder, то нужно использовать его по полной.
Переходим к форме.
В её конструкторе используется StringBuilder для собирания в одно целое строковых
литералов, которые известны заранее, на этапе компиляции. Можно выбросить этот билдер
и просто написать одну большую строку.
Если хочется красивого форматирования, то можно так:
string text = "Для начала работы укажите:" + Environment.NewLine +
"1) исходный каталог - откуда бы Вы хотели перенести файлы;" + Environment.NewLine +
...
Т. к. это литералы, то они будут склеены в одну строку на этапе компиляции. В рантайме
конкатенации не будет.
Для большей краткости можно использовать "\r\n".
Отмечу положительно отключение/включение контролов (Enabled/ReadOnly) перед началом
работы, после её завершения. Защита от дурака - не дать пользователю выполнить запрещённое
действие - лучше всего.
Task.Run(() => handler.MoveFiles(cancellationTokenSource.Token));
Кхгм... Знаю, что это именно я так написал... Но в данном конкретном случае это не
очень хороший код.
Метод Task.Run, запуская задачу, берёт поток из пула. Пул должен использоваться только
для коротких задач. Взяли поток, что-то быстро сделали, вернули поток в пул.
А у вас задача выполняется долго, возможно даже всегда. Для такой цели лучше создать
отдельный поток, не из пула.
Task.Factory.StartNew(() => handler.MoveFiles(cancellationTokenSource.Token), TaskCreationOptions.LongRunning);
LongRunning означает, что задача долгоиграющая и для неё будет создан отдельный поток.
