Плохой код

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

Итак, что имеется:

1. Контроллер, который обрабатывает события от UI.
2. Окошко, события которого обрабатывает контроллер, может находиться в нескольких режимах. В зависимости от режима, одно и то же событие может обрабатываться по разному — в одном режиме щелчок по объекту приводит к переходу на экран с подробной информацией об объекте, в другом режиме — к удалению объекта и т.п. Всего режимов 3 штуки.
3. Чтобы обработчики событий не превратились в спагетти от условной логики, завязанной на режим, код обработки событий выносится в отдельные классы (state-классы); по одному классу на каждый режим.
4. Есть события, которые во всех режимах обрабатываются одинаково, они реализованы в базовом state-классе.
5. В некоторых режимах некоторые действия не поддерживаются. Соответственно, на UI эти действия запрещаются (контролы делаются недоступными), а в обработчиках кидаются исключения.

Собственно говоря, ничего ракетного. Такая штука называется по научному паттерном «State» и замечательно описана в книжке Банды Четырёх[1]. Там даже пример похожий приводится, с программой для рисования, выбранным инструментом (кисть, карандаш, ластик) и обработкой событий мыши.

Реализовано всё это было примерно так:

public sealed class Controller
{
    private StateBase currentState;

    public void Action1()
    {
        currentState.Action1();
    }

    public void Action2()
    {
        currentState.Action2();
    }
}

public abstract class StateBase
{
    public void Action1()
    {
        // Тут код, общий для всех режимов
    }

    // Реализацию предоставляют наследники
    public abstract void Action2();
}

public sealed class ConcreteState1 : StateBase
{
    public override void Action2()
    {
        // В этом режиме действие Action2 не поддерживается,
        // метод вызываться не должен
        throw new NotSupportedException();
    }
}

public sealed class ConcreteState2 : StateBase
{
    public override void Action2()
    {
        // В этом режиме действие Action2 не поддерживается,
        // метод вызываться не должен
        throw new NotSupportedException();
    }
}

public sealed class ConcreteState3 : StateBase
{
    public override void Action2()
    {
        // Логика выполнения действия Action2 для этого режима
    }
}

В один прекрасный день один из коллег увидел всё это дело и решил избавиться от дублирования кода — в двух из трёх наследников код в Action2 одинаковый. Получилось такое:

public abstract class StateBase
{
    // ...

    public virtual void Action2()
    {
        // Общий код из ConcreteState1 и ConcreteState2
        // перенесли сюда
        throw new NotSupportedException();
    }
}

public sealed class ConcreteState1 : StateBase
{
    // ...
}

public sealed class ConcreteState2 : StateBase
{
    // ...
}

public sealed class ConcreteState3 : StateBase
{
    public override void Action2()
    {
        // Логика выполнения действия Action2 для этого режима,
        // реализация Action2 из базового класса не вызывается.
    }
}

На самом деле, действий, таких как Action2, которые поддерживаются только в одном из state-классов, было несколько; ко всем были применены аналогичные преобразования. Результат оказался компактнее по размеру, но хуже по качеству кода. Что тут есть плохого?

Проблема №1: формально ConcreteState3 продолжает оставаться наследником StateBase, фактически — нет. После изменений StateBase превратился из «базового класса для всех состояний» в «базовый класс для всех состояний, в которых Action2 не поддерживается», но притворяется, что он остался всё тем же «базовым классом для всех состояний».
Внести эти изменения в StateBase можно было только зная реализацию его наследников (плохо). Вносить изменения/добавлять новых наследников StateBase можно только зная реализацию StateBase (плохо). В частности, базовый класс разрешает переопределить Action2 в наследнике, но вызывать при этом Action2 базового класса наследник не должен.
StateBase после изменений стал, по терминологии Эрика Липперта, «хрупким базовым классом» (brittle base class). От таких классов тяжело наследоваться, в них тяжело вносить изменения из-за необоснованно большого сцепления между базовым классом и наследниками.

Проблема №2: состояния, которые не поддерживали Action2, не просто так бросали исключения. Это средство для обнаружения других ошибок в коде. В этом конкретном примере исключение NotSupportedException говорит об одном из двух:

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

Вывод из всего этого простой: иерархия наследования из-за сильной зависимости между базовыми и производными классами — крайне ломкая штука; прежде чем вносить в неё изменения, нужно семь раз отмерить. Даже незначительные изменения могут внести ошибки в код, и вы этого не заметите.