Как шаблонный метод может сломать ваш Java код

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

Предисловие

Это ещё одна статья, родившаяся из проверки 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.