Блог

В настоящее время мне поручено очистить, исправить ошибки и оптимизировать форму в winforms (3000 строк кода в одном .cs файле, это становится немного уродливым!). Я уже заметил несколько очевидных плохих практик и несколько избыточных вызовов, с которыми я мог бы относительно легко разобраться.

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

 private void datePicker_DateChanged(object sender, EventArgs e)
{
  tabControl_SelectedIndexChanged(sender, e);
}

private void comboBox_SelectedIndexChanged(object sender, EventArgs e)
{
  tabControl_SelectedIndexChanged(sender, e);
}
  

Моя первая проблема заключается в том, что у метода будет sender объект, который является средством выбора даты или полем со списком, но имеет ли это значение? Я спросил себя, для чего существует объект sender? Возможно, именно поэтому он там? Я также считаю EventArgs , что сам по себе он довольно бесполезен (насколько мне известно), если класс не унаследован.

Я знаю, что ни sender или EventArgs не используются в tabControl_SelectedIndexChanged методе, поэтому код работает нормально. Как насчет возможных будущих последствий при изменении некоторого кода?

Должен ли я изменить их на 3 разных обработчика событий, которые все указывают на простой void loadCurrentTab() метод? Или, возможно, я должен заставить все 3 элемента управления вызывать один и тот же обработчик событий, например loadCurrentTab(sender, e) ? Или просто оставить все как есть? Это так важно?

Должен ли я изменить их на 3 разных обработчика событий, которые все указывают на простой метод void loadCurrentTab()?

На самом деле это было бы моим предпочтением в этом сценарии. Это делает намерение очень ясным — все три обработчика событий перенаправляются на один набор логики, который (по замыслу) не обращает внимания на отправителя или EventArgs .

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

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

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

Предпочтительным подходом было бы иметь несколько обработчиков, вызывающих специальный метод (не eventhandler).

 private void datePicker_DateChanged(object sender, EventArgs e)
{
   // tabControl_SelectedIndexChanged(sender, e);
   RefreshCurrentTabControl();
}
  

вызов другого обработчика событий, очевидно, не является хорошей практикой. Потому что, если tabControl_SelectedIndexChanged изменение во времени и запуск зависят от отправителя, весь код завершается с ошибкой или запуск не прекращается..

Я разделю ту же логику в третьем методе, как вы сказали .. loadCurrentTab() . Вы также можете рассмотреть возможность использования некоторого шаблона команды для обработки событий -> хороший способ отделить бизнес-логику от графического интерфейса.

Например:

 interface ICommand {
    void Execute();
}

interface IEventCommand : ICommand {
    void Execute(object sender, EventArgs e); 
}


class CommandAttribute : Attribute {
     ...
}

class MyCommand : IEventCommand {
     ...
}
  

внедрите некоторый менеджер, исполнитель, присоедините команду с использованием отражения и т.д…

И затем вы можете присоединить команду следующим образом:

 [Command(typeof(MyCommand), new [] {"Click"})]
private Button m_oButton = new Button();
  

Все необходимые параметры автоматически передаются в command ;).

Да, это плохая практика, вы возитесь с одним и нарушаете другое по незнанию, классическая ошибка была бы такой же простой, как ((ComboBox)sender).SomeProperty, потому что «его можно вызвать только из списка со списком».

Зависит от того, насколько далеко вы можете позволить себе провести рефакторинг, но моей мгновенной реакцией было бы, по крайней мере, «RefreshTabControl», чтобы выполнить работу, а затем заставить обработчики событий вызвать ее. Если вам нужно расширить это, т. Е. Имеет значение Отправитель или EventArgs, то некоторая форма делегирования, как предложено, или не такая умная, но намного лучше, чем у вас, была бы перегрузкой.

например, аннулировать RefreshTabControl(Combobox argBox, EventArgs e), а затем выполнить приведение и нулевую проверку в eventhandler, по крайней мере, это было бы заявлением о намерениях.

То, что у вас есть сейчас, — это ошибка, ожидающая своего появления.