Статический анализатор, который изменит вашу архитектуру
Статический анализатор обычно помогает поддерживать выбранный стиль кода. Иногда он находит нетривиальные шаблонные проблемы. Но сегодня посмотрим на то, как статический анализатор заставляет менять всю архитектуру.
Tribore Menendez from Final Space
Я решал задачу получения текстового контента из объекта, представляющее
электронное письмо. Оказывается, задача непростая, ведь тело письма
может состоять из разных частей: вложения, html, альтернативный текст и другие
(RFC-2045). Вот ссылка на код,
который JakartaMail любезно предложил
мне использовать для этой задачи:
How do I find the main message body in a message that has attachments?
Я вставил этот метод в свой класс Mail
, где мне нужно было написать получение
контента из MimeMessage
:
import javax.mail.MessagingException;
import javax.mail.Multipart;
import javax.mail.Part;
import java.io.IOException;
public final class Mail {
private boolean textIsHtml = false;
/**
* Return the primary text content of the message.
*/
private String getText(Part p) throws
MessagingException, IOException {
if (p.isMimeType("text/*")) {
String s = (String)p.getContent();
textIsHtml = p.isMimeType("text/html");
return s;
}
if (p.isMimeType("multipart/alternative")) {
// prefer html text over plain text
Multipart mp = (Multipart)p.getContent();
String text = null;
for (int i = 0; i < mp.getCount(); i++) {
Part bp = mp.getBodyPart(i);
if (bp.isMimeType("text/plain")) {
if (text == null)
text = getText(bp);
continue;
} else if (bp.isMimeType("text/html")) {
String s = getText(bp);
if (s != null)
return s;
} else {
return getText(bp);
}
}
return text;
} else if (p.isMimeType("multipart/*")) {
Multipart mp = (Multipart)p.getContent();
for (int i = 0; i < mp.getCount(); i++) {
String s = getText(mp.getBodyPart(i));
if (s != null)
return s;
}
}
return null;
}
}
и запустил статический анализатор youshallnotpass (этот инструмент написан мной, его цель - удостовериться в некоторой степени в том, что код использует подход ElegantObjects):
> Task :youshallnotpass FAILED
nullfree
Mail.getText(Mail.java:24) > null
Mail.getText(Mail.java:28) > null
Mail.getText(Mail.java:33) > null
Mail.getText(Mail.java:44) > null
Mail.getText(Mail.java:49) > null
allfinal
Mail(Mail.java:8) > textIsHtml = false
Mail.getText(Mail.java:16) > String s = (String) p.getContent()
Mail.getText(Mail.java:23) > Multipart mp = (Multipart) p.getContent()
Mail.getText(Mail.java:24) > String text = null
Mail.getText(Mail.java:25) > int i = 0
Mail.getText(Mail.java:26) > Part bp = mp.getBodyPart(i)
Mail.getText(Mail.java:32) > String s = getText(bp)
Mail.getText(Mail.java:41) > Multipart mp = (Multipart) p.getContent()
Mail.getText(Mail.java:42) > int i = 0
Mail.getText(Mail.java:43) > String s = getText(mp.getBodyPart(i))
Mail.getText(Mail.java:13) > Part p
allpublic
Mail.getText(Mail.java:13) > private
nomultiplereturn
Mail.getText(Mail.java:13)
Гхм… С чего бы начать?…
Тут четыре типа ошибок:
NullFree
- нужно писать код без использованияnull
(Почему?)AllFinal
- нужно, чтобы везде стояло ключевое словоfinal
, чтобы поддерживать иммутабельность (Как?)AllPublic
- нужно, чтобы все методы и классы были с модификатором доступаpublic
(Почему?)NoMultipleReturn
- нужно, чтобы в методах был только один операторreturn
(Почему? Потому что много операторовreturn
мешают восприятию кода метода. Это не моя мысль, во многих статических анализаторах присутствуют ограничения на количество управляющих операторов в методах, я лишь сделал это ограничение бескомпромиссным.)
Ошибки статического анализатора нужно исправлять, иначе сборка в CI не соберется.
Ошибок достаточно много, поэтому придется исправлять поэтапно. Обычно, проще
всего решать AllFinal
ошибки. Это места, которые нужно пометить ключевым
словом final
. С них и начнем.
Хотя вот прямо сейчас нужно сделать отступление. Если вы нормальный программист,
то “Что, блин, происходит?!” уже звучит в вашей голове. Потому что для чего
нужно пытаться писать программу без null
-ов - не понятно, да и возможно ли
это? А все методы public
- это зачем? Ведь часто нужно сделать метод,
необходимый только для одного конкретного класса. А также ссылочки там были на
этого противоречивого господина и его
сомнительный подход ElegantObjects.
Короче, далее будет еще не одно действие, после которого будет начинаться жжение
под копчиком. Поэтому предлагаю считать все происходящее в этой статье
мысленным экспериментом.
AllFinal 1-ый этап
Вернемся к final
. Где там в самом первом месте
у нас final
?
Mail(Mail.java:8) > textIsHtml = false
Да, поле класса, которое будет изменено в зависимости от того, какой тип
текстового контента нашел алгоритм: если text/html
, то textIsHtml == true
,
если text/[not html]
, то, соответственно, textIsHtml == false
. Тут надо
объяснить, для чего это нужно, если вы не парсили почту в своих программах
ранее. Текстовый контент письма может быть представлен в разных форматах, чаще
всего это text/html
и text/plain
. Почтовый клиент должен выбрать наилучший
формат отображения текстового контента, исходя из особенностей окружения, и,
возможно, спросив пользователя (RFC 2046).
Поэтому, JakartaMail FAQ решила,
что приоритетнее text/html
, ну а если все же html
не был найден, то нужно
оставить флажок, чтобы разработчик понимал, что это найден не html
контент.
Очевидно, если просто поставить private final textIsHtml
,
то это не решит проблему. И вот тут на помощь приходит концепция объектов.
Задекларируем следующее поведение:
public interface TextContent {
String asString();
}
И, как вы уже, наверное, догадались, сделаем две реализации этого интерфейса.
Для не html
текста (скорее всего там будет просто text/plain
):
public final class SimpleTextContent implements TextContent {
private final String text;
public SimpleTextContent(final String text) {
this.text = text;
}
@Override
public String asString() {
return text;
}
}
И для html
текста:
public final class HtmlTextContent implements TextContent {
private final String text;
public HtmlTextContent(final String text) {
this.text = text;
}
@Override
public String asString() {
return text;
}
}
Да, да, да. Я уже чувствую запах летящего помидора: “Да это же две одинаковые
реализации!”. Верно, реализации пока одинаковые. Достаточно того, что у нас есть
две реализации одного поведения и они соответствуют разным классам объектов.
Просто мы еще не придумали бизнес логику, в которой они различны. Но ничего,
специально для этого вот вам бизнес случай: нужно извлечь текст из email и
сделать его цитирование (например, для того, чтобы генерировать автоматические
ответы на входящие письма). Различие в том, что для text/html
цитирование будет
выглядеть так:
<blockquote>
...
</blockquote>
А для text/plain
так:
> ...
> ...
> ...
Не зная других особенностей бизнес логики, можно реализовать данный сценарий так:
public interface TextContent {
...
String asQuote();
}
public final class HtmlTextContent implements TextContent {
private final String text;
...
@Override
public String asQuote() {
final StringBuilder quoteBuilder = new StringBuilder();
quoteBuilder.append("<blockquote>");
quoteBuilder.append(text);
quoteBuilder.append("</blockquote>");
return quoteBuilder.toString();
}
}
public final class SimpleTextContent implements TextContent {
private final String text;
...
@Override
public String asQuote() {
final StringTokenizer textLines = new StringTokenizer(text, "\n");
final StringBuilder quoteBuilder = new StringBuilder();
while (textLines.hasMoreTokens()) {
quoteBuilder.append('>');
quoteBuilder.append(' ');
quoteBuilder.append(textLines.nextToken());
quoteBuilder.append('\n');
}
return quoteBuilder.toString();
}
}
Хорошо, хорошо. Как там у нас теперь будет выглядеть метод получения текстового контента электронного письма?
Код без внешнего флажка textIsHtml
Посмотреть
Кстати, я дополнительно расставил final
-ы везде, где это было возможно, и у нас
осталось всего три места, где нужно поставить final
, но пока нельзя:
TextContent text = null;
...
for (int i = 0; i < mp.getCount(); i++)
...
for (int i = 0; i < mp.getCount(); i++)
“В for
-иках то зачем final
?” - спросите вы.
Там объявляется переменная—индекс int i = 0
, и она имеет возможность быть переназначенной, как,
собственно, и происходит в блоке действия, выполняемого после каждой итерации.
И, я уверяю вас, мы сможем избавиться от этого не final
поля через некоторое время. Но
пока, перейдем к другим ошибкам.
NoMultipleReturn
В нормальной жизни вас это бы не волновало, но
сейчас у нас мысленный эксперимент. Помните, да? Поэтому нужно попробовать
обойтись одним return
. Обычно, когда мне приходится решать подобные проблемки,
и переделывать множественный return
в одинарный, я поступаю следующим образом.
Я выделяю переменную final TextContent result;
, затем вместо каждого
return value;
делаю result = value;
:
private TextContent getText(final Part p) {
final TextContent result;
if (p.isMimeType("text/*")) {
final String s = (String)p.getContent();
if (p.isMimeType("text/html")) {
result = new HtmlTextContent(s);
} else {
result = new SimpleTextContent(s);
}
} else if (p.isMimeType("multipart/alternative")) {
...
} else if (p.isMimeType("multipart/*")) {
...
} else {
result = null;
}
return result;
}
Сложные случаи с циклами сейчас разберем отдельно. Возьмем из двух сложных
циклов тот, что попроще, и на нем научимся делать один return
, да еще и
final
декларацию не сломать.
} else if (p.isMimeType("multipart/*")) {
final Multipart mp = (Multipart)p.getContent();
for (int i = 0; i < mp.getCount(); i++) {
final TextContent s = getText(mp.getBodyPart(i));
if (s != null)
return s;
}
}
Что тут происходит? Итерируются по частям multipart
-а, и первый не пустой
ответ будет являться результатом. Достаточно ввести еще один список — список
непустого контента, найденного в частях multipart
-а и проблема будет решена:
} else if (p.isMimeType("multipart/*")) {
final Multipart mp = (Multipart)p.getContent();
final List<TextContent> foundTexts = new ArrayList<>();
for (int i = 0; i < mp.getCount(); i++) {
final TextContent s = getText(mp.getBodyPart(i));
if (s != null) {
foundTexts.add(s);
}
}
if (!foundTexts.isEmpty()) {
result = foundTexts.get(0);
} else {
result = null;
}
}
Итоговый результат с одним return
:
Посмотреть
И текущие ошибки анализатора:
nullfree
Mail.getText(Mail.java:37) > null
Mail.getText(Mail.java:50) > null
Mail.getText(Mail.java:57) > null
Mail.getText(Mail.java:64) > null
Mail.getText(Mail.java:67) > null
allfinal
Mail.getText(Mail.java:29) > int i = 0
Mail.getText(Mail.java:55) > int i = 0
allpublic
Mail.getText(Mail.java:13) > private
NullFree
Так так. null
используется в этом коде для обозначения того, что текстовый
контент не найден. Обычно, в таких ситуациях есть несколько вариантов:
- бросать исключение, если текстовый контент не найден (тогда придется
отлавливать его вместо проверки на
null
) - сделать еще одну реализацию
interface TextContent
для обозначения отсутствия текста - можно попробовать приравнять отсутствие текста к пустой строке
Вот третьим путем мы и пойдем, так как ограничения нашей несуществующей бизнес
логики говорят, что, в случае отсутствия текстового контента в письме, можно
считать, что письмо просто состоит из пустой строки. Это значит, что все
присваивания null
-ов можно заменить на new SimpleTextContent("")
, а все
проверки на null
можно заменить на
textContent.isEmpty()
(этого метода там еще нет,
но ввести его не сложно, так как необходимая информация уже инкапсулирована в
классы SimpleTextContent
, HtmlTextContent
).
Довольно просто!
Код без null
-ов
Посмотреть
Однако ошибки анализатора все еще присутствуют:
allfinal
Mail.getText(Mail.java:29) > int i = 0
Mail.getText(Mail.java:55) > int i = 0
allpublic
Mail.getText(Mail.java:13) > private
AllFinal 2-ой этап
for (final int i = 0; i < mp.getCount(); i++) {
- вот так, как вы понимаете,
сделать нельзя. Вот если бы можно было проитерироваться по элементам
multipart
😌:
for (final Part part: mp) {
...
}
Однако, Multipart
не реализует Iterable
.
Что ж, не беда, сделаем свою обертку! Для начала, нужно реализовать
Iterator
:
import javax.mail.MessagingException;
import javax.mail.Multipart;
import javax.mail.Part;
import java.util.Iterator;
import java.util.NoSuchElementException;
import java.util.concurrent.atomic.AtomicInteger;
public final class PartsIterator implements Iterator<Part> {
private final Multipart multipart;
private final AtomicInteger position;
public PartsIterator(
final Multipart multipart
) {
this(multipart, new AtomicInteger(0));
}
public PartsIterator(
final Multipart multipart,
final AtomicInteger position
) {
this.multipart = multipart;
this.position = position;
}
@Override
public boolean hasNext() {
try {
return position.intValue() < multipart.getCount();
} catch (final MessagingException e) {
throw new IllegalStateException(e);
}
}
@Override
public Part next() {
if (!this.hasNext()) {
throw new NoSuchElementException(
"The iterator doesn't have any more items"
);
}
try {
return multipart.getBodyPart(position.getAndIncrement());
} catch (final MessagingException e) {
throw new IllegalStateException(e);
}
}
}
Теперь можно и Iterable
. Для скорости, воспользуемся библиотекой
cactoos
, в частности классом
IterableOf
:
new IterableOf<>(new PartsIterator(mp))
Почти конечный вариант c final
везде
Посмотреть
Осталась одна ошибка анализатора:
allpublic
Mail.getText(Mail.java:15) > private
AllPublic
Мысленный эксперимент продолжается, и нам нужно, казалось бы, просто взять и
сделать этот метод public
, чего сложного то? Сложно тут то, что если просто
сделать этот метод публичным, то обновится поведение у класса Mail
, владеющего
этим методом. Поэтому, как объяснялось в
статье:
Приватный метод - повод для нового класса
Логика в методе Mail.getText
получилась непростой, она однозначно требует
тестирования. Скорее всего, она может быть переиспользована в других участках
кода. Вынести этот код в новый класс - значит решить целых три проблемы:
- анализатор больше не будет ругаться
- получение текстового контента можно будет тестировать
- получение текстового контента можно будет переипользовать
Остается только выделить поведение такого класса, задеклалировать интерфейс,
и реализовать его. Интерфейс наверняка должен быть простым, от интерфейса нам
требуется только метод, возвращающий TextContent
. Или можно воспользоваться
поведением существующего TextContent
, написав реализацию, которая будет
принимать Part
в конструкторе.
Конечный вариант без ошибок анализатора
Посмотреть
Там довольно много кода, но я выделю главные изменения:
public final class PartTextContent implements TextContent {
private final Unchecked<TextContent> text;
public PartTextContent(final Part p) {
this(new Unchecked<>(() -> {
final TextContent result;
if (p.isMimeType("text/*")) {
...
} else if (p.isMimeType("multipart/alternative")) {
...
for (final Part bp: parts) {
final TextContent foundText = new PartTextContent(bp);
...
}
...
} else if (p.isMimeType("multipart/*")) {
...
for (final Part bp: parts) {
final TextContent s = new PartTextContent(bp);
...
}
...
} else {
...
}
return result;
}));
}
public PartTextContent(final Unchecked<TextContent> text) {
this.text = text;
}
...
}
Видите, да? Там конструктор PartTextContent
вызывается в конструкторе
PartTextContent
. Вы можете подумать, что
это такой особый вид ректальных неудовольствий тонкого искусства. Потому что
вызывать конструктор рекурсивно - это уж совсем неоптимально, непроизводительно
и т.д. Но если немного подумать, то можно понять, что в email почти никогда нет
вложенных друг в друга multipart
-ов, поэтому рекурсия не будет глубокой. Если
вы переживаете, что при вызове методов asString
или isEmpty
будет каждый раз
создаваться новая гора объектов, то не стоит - там все достаточно легко и просто
закешировать с помощью декоратора
Solid
для поля final Unchecked<TextContent> text
:
Посмотреть
Кстати, вам может показаться, что код конструктора получился громоздким (мне тоже так кажется).
Поэтому можно выделить еще пару классов class MultipartAlternativeTextContent implements TextContent
и
class MultipartTextContent implements TextContent
,
тогда итоговый конструктор class PartTextContent
будет выглядеть совсем просто:
Посмотреть
Исходники всех созданных классов в конечном варианте можно посмотреть тут.
Спасибо большое! Вы смогли дочитать до конца и не потеряли достаточно нудную нить
моих размышлений, это очень приятно.
Мысленный эксперимент закончился. Мы просто попробовали переписать код так,
чтобы в нем все переменные были final
, чтобы там не использовались null
, чтобы
был только один return
и чтобы все методы были public
. На самом деле в
анализаторе youshallnotpass
есть еще несколько
правил. Например, не использовать getter-ы/setter-ы, не использовать ключевое
слово static
. И вы, может быть, удивитесь, но
оказывается можно писать код, который соответствует всем этим правилам, и такой
код даже будет решать бизнес задачи. Это и была основная цель статьи - показать,
что так тоже можно. Я не буду утверждать, что код, соответствующий таким
правилам, лучше или хуже того, что мы привыкли видеть каждый день (хотя уже
есть некоторые исследования на тему
использования null).
Однако, мы попробовали соответствовать некоторым формальным правилам подхода
ElegantObjects с помощью автоматического инструмента.
Это заставило нас поменять архитектуру, и в процессе чего мы смогли
декомпозировать изначальную непростую логику на составляющие, зафиксировать
поведение этих компонентов, и, в конце концов, даже упростили (с точки зрения
чтения и восприятия) алгоритм.