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

Предисловие

Это ещё одна статья, родившаяся из проверки 24-й версии DBeaver статическим анализатором PVS-Studio. В её ходе я нашёл несколько подозрительных мест, которые зацепили меня достаточно для того, чтобы я посвятил им по отдельной статье. Обновляемый список статей из этого цикла:

• Ловушки volatile, DCL и синхронизации в Java

• Как шаблонный метод может сломать вашу программу (вы находитесь здесь)

ООП? Шаблонный метод?

ООП!

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

Шаблонный метод!

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

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

Сразу отдаём дань GoF и берём классическую схему этого паттерна из их книги:

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

Допустим, нам надо выводить в консоль (хотя это может быть и файл, и что угодно ещё) содержимое класса Person, который содержит в себе только имя и фамилию.

public class Person {
    private final String name; 
    private final String surname;

    public String getName() {
        return name;
    }
    public String getSurname() {
        return surname;
    }

    public Person(String name, String surname) {
        this.name = name;
        this.surname = surname;
    }
}

Просто так содержимое класса мы вывести не можем, нам надо его сериализовать. Для интереса представим, что формата сериализации два: json и xml. Мы бы, конечно, могли сделать всё в одном классе, а нужный тип сериализации выбирать посредством какого-нибудь перечисления, но тогда мы бы нарушили два принципа SOLID:

• SRP: совместив логику разных сериализаторов в одном, мы нарушим принцип единственной ответственности;

• OCP: добавляя новые типы сериализации в будущем, мы нарушим принцип открытости-закрытости, так как придётся менять уже существующий класс.

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

public abstract class AbstractPersonPrinter {
    protected abstract String serialize(Person person);
    public void print(Person person) {
        System.out.println(serialize(person));
    }
}

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

public class JsonPersonPrinter extends AbstractPersonPrinter {
    @Override
    public String serialize(Person person) {
        var sb = new StringBuilder();
        var s = System.getProperty("line.separator");
        sb.append("{").append(s);
        sb.append("  name: \"").append(person.getName())
                               .append("\"")
                               .append(s);
        sb.append("  surname: \"").append(person.getSurname())
                                  .append("\"")
                                  .append(s);
        sb.append("}");
        return sb.toString();
    }
}

И для xml:

public class XmlPersonPrinter extends AbstractPersonPrinter {
    @Override
    public String serialize(Person person) {
        var sb = new StringBuilder();
        var s = System.getProperty("line.separator");
        sb.append("<root>").append(s);
        sb.append("  <name>").append(s);
        sb.append("    ").append(person.getName())
                         .append(s);
        sb.append("  </name>").append(s);
        sb.append("  <surname>").append(s);
        sb.append("    ").append(person.getSurname())
                         .append(s);
        sb.append("  </surname>").append(s);
        sb.append("</root>");
        return sb.toString();
    }
}

Вуаля. Теперь мы можем сконфигурировать какой-нибудь логгер и получать вывод в желаемом формате. Если вам когда-то вообще хотелось получать вывод в json или xml.

public class ConsoleLogger {
    private final AbstractPersonPrinter printer;
    public ConsoleLogger(AbstractPersonPrinter printer) {
        this.printer = printer;
    }

    public void logPerson(Person person) {
        printer.print(person);
    }
    ....
}

{
  name: "John"
  surname: "Doe"
}

<root>
  <name>
    John
  </name>
  <surname>
    Doe
  </surname>
</root>

Вернёмся к диаграмме класса. Её можно перерисовать под наш пример, чтобы было проще понять предыдущую:

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

Реальная практика

Задача и решение

О шаблонном методе можно сказать как о базовом во многих смыслах, но в нём есть и неочевидные места.

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

Пробуем решить задачу:

1. Делаем абстрактный обобщённый класс ParameterBase, который заключает в себе вышеописанную логику. Копирование из DTO и других объектов происходит через рефлексию, а логика сохранения и обновления состояния реализована с помощью паттерна хранитель (memento);

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

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

public abstract class ParameterBase<TEntity>
    : ObservableValidator where TEntity : class
{
    protected virtual List<string> GetIgnoredProperties()
        => new List<string>();

    public ParameterBase(TEntity entity)
    {
        var ignore = GetIgnoredProperties();
        var sourceProp = GetType().GetProperties();

        var colNumber = sourceProp.Length - ignore.Length;
        var colCounter = 0;

        foreach (var prop in sourceProp)
            if (!(ignore.Contains(prop.Name)))
            {
                prop.SetValue(this, typeof(TEntity)
                    .GetProperty(prop.Name, Consts.PropertyBindingFlags)
                    .GetValue(entity, null), null);
                colCounter++;
            }

        if (colNumber != colCounter)
            throw new InvalidOperationException(
                "Not every parameter field got its value");

        this.PropertyChanged += Change;
    }

    ....
}

Алгоритм довольно простой: копируем при помощи рефлексии все свойства из обобщённой модели в потомка ParameterBase, игнорируя указанные в самом потомке. Если что-то не сошлось по количеству, то выбрасываем исключение. Собственно, метод GetIgnoredMappingProperties и является частным случаем шаблонного метода, который называется зацепка (hook). Задача решена. Всё хорошо, так ведь?

Внезапное предупреждение

Всё бы хорошо, но после сборки срабатывает инкрементальный анализ, и PVS-Studio выдаёт следующее сообщение на код выше:

V3068 Calling overrideable class member 'GetIgnoredProperties' from constructor is dangerous. ParameterBase.cs(34)

И вот тут надо сказать, что я не очень люблю анализ на code smell-ы. В большинстве случаев хочется встать в позу, пожурить инструмент и с умным видом подавить предупреждение. И не буду нагнетать ненужной интриги — в данном случае анализатор не прав, никакой проблемы в моём решении нет. И именно так я в своё время и поступил, занеся предупреждение в suppress файл проекта, откуда я его сейчас и достал.

Конечно, можно пофантазировать о возможной проблеме и её исправлении:

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

2. Для исправления можно было бы сделать конструктор приватным, инициализацию вынести в отдельный метод, а созданием объектов управлять через фабрику.

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

У нас проблемы

Опасность близко

Об этом случае я и вспомнил, когда, листая DBeaver, наткнулся на аналогичное для Java сообщение PVS-Studio:

V6052 Calling overridden 'isBinaryContents' method in 'TextWithOpen' parent-class constructor may lead to use of uninitialized data. Inspect field: binary. TextWithOpenFile.java(77), TextWithOpen.java(59)

Вспоминая прошлый опыт, я намеревался пройти дальше, но всё же решил изучить код. Итак, метод isBinaryContents находится в классе TextWithOpenFIle:

public class TextWithOpenFile extends TextWithOpen {
    private final boolean binary;
    ....

    @Override
    protected boolean isBinaryContents() {
        return binary;
    }
    ....
}

Не выглядит захватывающе. Но давайте посмотрим на единственное место, где он используется:

public TextWithOpen(Composite parent, boolean multiFS, boolean secured) {
    super(parent, SWT.NONE);

    ....
    if (!useTextEditor && !isBinaryContents()) {
        ....
        
        editItem.setEnabled(false);
    }

    ....
}

Анализатор указал на огромный конструктор, который я для удобства сократил. Вышеупомянутый isBinaryContents используется в условии, в теле которого я вырезал около 40 строк. Имейте в виду, что сейчас мы находимся в классе-родителе TextWithOpen. Теперь бы посмотреть, что находится внутри родительского isBinaryContents:

protected boolean isBinaryContents() {
    return false;
}

О, та самая зацепка, о которой мы говорили ранее. Получается, разработчики хотели, чтобы в родительском классе второе условие всегда равнялось true (не забываем про отрицание в нём). Так, а о чём говорит документация к диагностике?

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

Выходит, надо проверить конструктор класса TextWithOpenFIle:

public TextWithOpenFile(
    Composite parent,
    String title,
    String[] filterExt,
    int style,
    boolean binary,
    boolean multiFS,
    boolean secured
) {
    super(parent, multiFS, secured);   // <=
    this.title = title;
    this.filterExt = filterExt;
    this.style = style;
    this.binary = binary;              // <=
}

Ого. Ошибка. Сначала мы вызываем конструктор TextWithOpenFile, тот в свою очередь вызывает конструктор TextWithOpen, где вызывается isBinaryContents, читающий значение binary, которое по умолчанию является false. И только после этого поле binary инициализируется в конструкторе TextWithOpenFile.

Самое неприятное, что сходу непонятно, как это исправить. Просто взять и переместить вниз вызов super, к сожалению, не выйдет :)

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

Хорошей альтернативой было бы использовать порождающие паттерны:

• Любая из фабрик, как я уже упоминал, поместила бы процесс создания объектов в отдельное место. Тогда рядом хотя бы будут напоминания о необходимости инициализации и её способе;

• Для данного случая также поможет строитель — классическое решение для ситуаций, когда инициализация становится слишком сложной. С помощью этого паттерна можно разбить её на несколько более простых шагов, тем самым упростив дальнейшее расширение.

Мораль

Кажется, где-то здесь надо признать, что мой скептицизм был посрамлён, а предупреждения анализатора игнорировать нельзя. Но я всё же останусь при своём мнении. В конце концов, даже в нашей документации написано:

В случае, если вы точно уверены, что вам нужно описанное поведение при инициализации объекта и вы хотите скрыть предупреждение от анализатора, пометьте сообщение как ложнопозитивное.

В моём случае я оценивал и оцениваю вероятность возможной ошибки как невероятно малую, хотя и без гарантий. Здесь же мы имеем значительно более сложную инициализацию с большим количеством параметров, и в этих обстоятельствах опасный приём закономерно привёл к ошибке. В общем, здравая оценка рисков — главная вещь при принятии решений. Тоже не звучит захватывающе, я знаю :)

Послесловие

И на этом хочется перейти к заключению. Надеюсь, вам было интересно узнать, как следование правильному подходу может привести к обидной ошибке. И если вы будете лишний раз всё перепроверять, используя переопределяемые методы в конструкторах, то я своё дело сделал :)

Если вам хочется поискать эту или другие ошибки в своём проекте, то вы можете бесплатно попробовать PVS-Studio, перейдя по этой ссылке.

А ещё я напомню, что это не все интересные вещи, которые были найдены в DBeaver, и к выходу планируется ещё одна статья. После публикации я добавлю ссылки сюда, а пока предлагаю подписаться на мой Х-твиттер, чтобы не пропустить её выход.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Konstantin Volohovsky. How template method can ruin your Java code.