ООП — это замечательно. За несоблюдение этой парадигмы принято ругать, а знание паттернов зачастую является обязательным. Но даже правильный подход не страхует полностью от ошибок. О том, как сломать программу при помощи обычного шаблонного метода, мы сегодня и узнаем.
Это ещё одна статья, родившаяся из проверки 24-й версии DBeaver статическим анализатором PVS-Studio. В её ходе я нашёл несколько подозрительных мест, которые зацепили меня достаточно для того, чтобы я посвятил им по отдельной статье. Обновляемый список статей из этого цикла:
Не так давно я писал статью о том, как можно применять ООП в своей ежедневной работе. Статья теоретическо-практическая и заглядывает в вопрос шире, чем мы это сделаем сегодня. Интересующихся темой приглашаю в ту статью, но весь необходимый материал мы повторим здесь — об этом можно не беспокоиться. В рамках же этой статьи необходимость использования ООП и следования 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:
Вспомнив об этом, разумеется, сразу же понимаем, что так делать не надо. Вместо этого определим абстрактный метод-сериализатор в классе нашего принтера. Реализация класса тривиальна:
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);
}
....
}json:
{
name: "John"
surname: "Doe"
}xml:
<root>
<name>
John
</name>
<surname>
Doe
</surname>
</root>Вернёмся к диаграмме класса. Её можно перерисовать под наш пример, чтобы было проще понять предыдущую:
Хорошо, затянутое объяснение паттерна закончено. Почти. Только упомяну, что если у вас были мысли, что вместо наследования тут может быть лучше использовать композицию (то есть стратегию) — то вы в своём праве. Я же просто хотел продемонстрировать конкретный паттерн, так что условился использовать именно его.
О шаблонном методе можно сказать как о базовом во многих смыслах, но в нём есть и неочевидные места.
Вот, например, была следующая задача: на веб-странице есть форма с данными, большую часть из которых можно менять. Причём нужно запоминать их версию до и после изменений, чтобы при закрытии страницы уведомлять пользователя о том, что он их не сохранил. Таких форм на разных страницах много, поэтому нужно общее решение.
Пробуем решить задачу:
Проблему во втором пункте надо как-то решать. Для этого вводим переопределяемый метод, в котором можно указывать ненужные поля. Вот немного упрощённое решение, которое я тогда написал:
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;
}
....
}Это не у меня Java сломалась, это С# код :) Прошу воспринимать это как стилистическое отступление, код я постарался упростить.
На резонный вопрос о том, почему во фронтэнд задаче используется C#, отвечу, что это фреймворк Blazor, который активно используется в нашей компании.
К слову, раз мы заговорили о других языках программирования, то у нас есть статья на схожую тему и для C++.
Алгоритм довольно простой: копируем при помощи рефлексии все свойства из обобщённой модели в потомка ParameterBase, игнорируя указанные в самом потомке. Если что-то не сошлось по количеству, то выбрасываем исключение. Собственно, метод GetIgnoredMappingProperties и является частным случаем шаблонного метода, который называется зацепка (hook). Задача решена. Всё хорошо, так ведь?
Всё бы хорошо, но после сборки срабатывает инкрементальный анализ, и PVS-Studio выдаёт следующее сообщение на код выше:
V3068 Calling overrideable class member 'GetIgnoredProperties' from constructor is dangerous. ParameterBase.cs(34)
И вот тут надо сказать, что я не очень люблю анализ на code smell-ы. В большинстве случаев хочется встать в позу, пожурить инструмент и с умным видом подавить предупреждение. И не буду нагнетать ненужной интриги — в данном случае анализатор не прав, никакой проблемы в моём решении нет. И именно так я в своё время и поступил, занеся предупреждение в suppress файл проекта, откуда я его сейчас и достал.
Конечно, можно пофантазировать о возможной проблеме и её исправлении:
Но зачем заниматься этой эквилибристикой только ради того, чтобы анализатор от тебя отстал, если проще подавить предупреждение? В общем, неинтересно.
Об этом случае я и вспомнил, когда, листая 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. Ссылки на другие статьи можно найти в начале, а чтобы не пропустить выход новых статей, предлагаю подписаться на мой Х-твиттер.