Статический анализатор обычно помогает поддерживать выбранный стиль кода. Иногда он находит нетривиальные шаблонные проблемы. Но сегодня посмотрим на то, как статический анализатор заставляет менять всю архитектуру.
Я решал задачу получения текстового контента из объекта, представляющее электронное письмо. Оказывается, задача непростая, ведь тело письма может состоять из разных частей: вложения, 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
Посмотреть
import javax.mail.MessagingException;
import javax.mail.Multipart;
import javax.mail.Part;
import java.io.IOException;
public final class Mail {
/**
* Return the primary text content of the message.
*/
private TextContent getText(final Part p) throws
MessagingException, IOException {
if (p.isMimeType("text/*")) {
final String s = (String)p.getContent();
final TextContent content;
if (p.isMimeType("text/html")) {
content = new HtmlTextContent(s);
} else {
content = new SimpleTextContent(s);
}
return content;
}
if (p.isMimeType("multipart/alternative")) {
// prefer html text over plain text
final Multipart mp = (Multipart)p.getContent();
TextContent text = null;
for (int i = 0; i < mp.getCount(); i++) {
final Part bp = mp.getBodyPart(i);
if (bp.isMimeType("text/plain")) {
if (text == null)
text = getText(bp);
continue;
} else if (bp.isMimeType("text/html")) {
final TextContent s = getText(bp);
if (s != null)
return s;
} else {
return getText(bp);
}
}
return text;
} 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;
}
}
return null;
}
}
Кстати, я дополнительно расставил 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
, и она имеет возможность быть переназначенной, как, собственно, и происходит в блоке действия, выполняемого после каждой итерации. Но, правило - есть правило, надо избавляться от ошибки. Не делать же @SuppressWarnings
в этом месте, да? И, я уверяю вас, мы сможем избавиться от этого не 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:
Посмотреть
import javax.mail.MessagingException;
import javax.mail.Multipart;
import javax.mail.Part;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
public final class Mail {
/**
* Return the primary text content of the message.
*/
private TextContent getText(final Part p) throws
MessagingException, IOException {
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")) {
// prefer html text over plain text
final Multipart mp = (Multipart)p.getContent();
final List<TextContent> foundPlain = new ArrayList<>();
final List<TextContent> foundHtml = new ArrayList<>();
final List<TextContent> foundOthers = new ArrayList<>();
for (int i = 0; i < mp.getCount(); i++) {
final Part bp = mp.getBodyPart(i);
final TextContent foundText = getText(bp);
if (bp.isMimeType("text/plain")) {
foundPlain.add(foundText);
} else if (bp.isMimeType("text/html")) {
foundHtml.add(foundText);
} else {
if (foundText != null) {
foundOthers.add(foundText);
}
}
}
if (!foundHtml.isEmpty()) {
result = foundHtml.get(0);
} else if (!foundOthers.isEmpty()) {
result = foundOthers.get(0);
} else if (!foundPlain.isEmpty()) {
result = foundPlain.get(0);
} else {
result = null;
}
} 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;
}
} else {
result = null;
}
return result;
}
}
И текущие ошибки анализатора:
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-ов
Посмотреть
import javax.mail.MessagingException;
import javax.mail.Multipart;
import javax.mail.Part;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
public final class Mail {
/**
* Return the primary text content of the message.
*/
private TextContent getText(final Part p) throws
MessagingException, IOException {
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")) {
// prefer html text over plain text
final Multipart mp = (Multipart)p.getContent();
final List<TextContent> foundPlain = new ArrayList<>();
final List<TextContent> foundHtml = new ArrayList<>();
final List<TextContent> foundOthers = new ArrayList<>();
for (int i = 0; i < mp.getCount(); i++) {
final Part bp = mp.getBodyPart(i);
final TextContent foundText = getText(bp);
if (bp.isMimeType("text/plain")) {
foundPlain.add(foundText);
} else if (bp.isMimeType("text/html")) {
foundHtml.add(foundText);
} else {
if (!foundText.isEmpty()) {
foundOthers.add(foundText);
}
}
}
if (!foundHtml.isEmpty()) {
result = foundHtml.get(0);
} else if (!foundOthers.isEmpty()) {
result = foundOthers.get(0);
} else if (!foundPlain.isEmpty()) {
result = foundPlain.get(0);
} else {
result = new SimpleTextContent("");
}
} 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.isEmpty()) {
foundTexts.add(s);
}
}
if (!foundTexts.isEmpty()) {
result = foundTexts.get(0);
} else {
result = new SimpleTextContent("");
}
} else {
result = new SimpleTextContent("");
}
return result;
}
}
Однако ошибки анализатора все еще присутствуют:
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 везде
Посмотреть
import org.cactoos.iterable.IterableOf;
import javax.mail.MessagingException;
import javax.mail.Multipart;
import javax.mail.Part;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
public final class Mail {
/**
* Return the primary text content of the message.
*/
private TextContent getText(final Part p) throws
MessagingException, IOException {
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")) {
// prefer html text over plain text
final Multipart mp = (Multipart)p.getContent();
final IterableOf<Part> parts = new IterableOf<>(
new PartsIterator(mp)
);
final List<TextContent> foundPlain = new ArrayList<>();
final List<TextContent> foundHtml = new ArrayList<>();
final List<TextContent> foundOthers = new ArrayList<>();
for (final Part bp: parts) {
final TextContent foundText = getText(bp);
if (bp.isMimeType("text/plain")) {
foundPlain.add(foundText);
} else if (bp.isMimeType("text/html")) {
foundHtml.add(foundText);
} else {
if (!foundText.asString().isEmpty()) {
foundOthers.add(foundText);
}
}
}
if (!foundHtml.isEmpty()) {
result = foundHtml.get(0);
} else if (!foundOthers.isEmpty()) {
result = foundOthers.get(0);
} else if (!foundPlain.isEmpty()) {
result = foundPlain.get(0);
} else {
result = new SimpleTextContent("");
}
} else if (p.isMimeType("multipart/*")) {
final Multipart mp = (Multipart)p.getContent();
final IterableOf<Part> parts = new IterableOf<>(
new PartsIterator(mp)
);
final List<TextContent> foundTexts = new ArrayList<>();
for (final Part bp: parts) {
final TextContent s = getText(bp);
if (!s.asString().isEmpty()) {
foundTexts.add(s);
}
}
if (!foundTexts.isEmpty()) {
result = foundTexts.get(0);
} else {
result = new SimpleTextContent("");
}
} else {
result = new SimpleTextContent("");
}
return result;
}
}
Осталась одна ошибка анализатора:
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/*")) {
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")) {
// prefer html text over plain text
final Multipart mp = (Multipart)p.getContent();
final IterableOf<Part> parts = new IterableOf<>(
new PartsIterator(mp)
);
final List<TextContent> foundPlain = new ArrayList<>();
final List<TextContent> foundHtml = new ArrayList<>();
final List<TextContent> foundOthers = new ArrayList<>();
for (final Part bp: parts) {
final TextContent foundText = new PartTextContent(bp);
if (bp.isMimeType("text/plain")) {
foundPlain.add(foundText);
} else if (bp.isMimeType("text/html")) {
foundHtml.add(foundText);
} else {
if (!foundText.asString().isEmpty()) {
foundOthers.add(foundText);
}
}
}
if (!foundHtml.isEmpty()) {
result = foundHtml.get(0);
} else if (!foundOthers.isEmpty()) {
result = foundOthers.get(0);
} else if (!foundPlain.isEmpty()) {
result = foundPlain.get(0);
} else {
result = new SimpleTextContent("");
}
} else if (p.isMimeType("multipart/*")) {
final Multipart mp = (Multipart)p.getContent();
final IterableOf<Part> parts = new IterableOf<>(
new PartsIterator(mp)
);
final List<TextContent> foundTexts = new ArrayList<>();
for (final Part bp: parts) {
final TextContent s = new PartTextContent(bp);
if (!s.asString().isEmpty()) {
foundTexts.add(s);
}
}
if (!foundTexts.isEmpty()) {
result = foundTexts.get(0);
} else {
result = new SimpleTextContent("");
}
} else {
result = new SimpleTextContent("");
}
return result;
}));
}
public PartTextContent(final Unchecked<TextContent> text) {
this.text = text;
}
@Override
public String asString() {
return text.value().asString();
}
@Override
public String asQuote() {
return text.value().asQuote();
}
@Override
public boolean isEmpty() {
return text.value().isEmpty();
}
}
Там довольно много кода, но я выделю главные изменения:
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
:
Посмотреть
public PartTextContent(final Part p) {
this(
new Unchecked<>(
new Solid<>(() -> {
final TextContent result;
...
return result;
})
)
);
}
Кстати, вам может показаться, что код конструктора получился громоздким (мне тоже так кажется). Поэтому можно выделить еще пару классов class MultipartAlternativeTextContent implements TextContent
и class MultipartTextContent implements TextContent
, тогда итоговый конструктор class PartTextContent
будет выглядеть совсем просто:
Посмотреть
public final class PartTextContent implements TextContent {
public PartTextContent(final Part p) {
this(new Unchecked<>(() -> {
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")) {
// prefer html text over plain text
final Multipart mp = (Multipart) p.getContent();
result = new MultipartAlternativeTextContent(mp);
} else if (p.isMimeType("multipart/*")) {
final Multipart mp = (Multipart) p.getContent();
result = new MultipartTextContent(mp);
} else {
result = new SimpleTextContent("");
}
return result;
}));
}
...
}
Исходники всех созданных классов в конечном варианте можно посмотреть тут.
Спасибо большое! Вы смогли дочитать до сюда и не потеряли достаточно нудную нить моих размышлений, это очень приятно. Мысленный эксперимент закончился. Мы попробовали переписать код так, чтобы в нем все переменные были final
, чтобы там не использовались null
, чтобы был только один return
и чтобы все методы были public
. На самом деле в анализаторе youshallnotpass
есть еще несколько правил. Например, не использовать getter-ы/setter-ы, не использовать ключевое слово static
. И вы, может быть, удивитесь, но оказывается можно писать код, который соответствует всем этим правилам, и такой код даже будет решать бизнес задачи. Это и была основная цель статьи - показать, что так тоже можно. Я не буду утверждать, что код, соответствующий таким диким правилам, лучше или хуже того (по разным критериям, от поддерживаемости до производительности), что мы привыкли видеть каждый день (хотя уже есть некоторые исследования на тему использования null). Однако, мы попробовали соответствовать некоторым формальным правилам подхода ElegantObjects с помощью автоматического инструмента. Это заставило нас поменять архитектуру, и в процессе чего мы смогли декомпозировать изначальную непростую логику на составляющие, зафиксировать поведение этих компонентов, и, в конце концов, даже упростили (с точки зрения чтения и восприятия) алгоритм.
Комментарии (25)
SadOcean
25.06.2022 13:32+8AllPublic - мне это кажется ужасной рекомендацией.
И приведенная для объяснения статья раскрывает ее лишь отчасти, но в целом не говорит о таком требовании. Это уж не говоря про то, что с самой статьей можно поспорить.
Действительно приватные методы и приватные статические методы - это кандидаты на рефакторинг и вынесение в отдельные классы, но это не всегда оправданно. Они могут быть слишком специфичными и не иметь достаточно хорошей обобщенной функциональности, чтобы выносить их. Кроме того само вынесение и обобщение - не бесплантая штука, она потребует дополнительных усилий на поддержание, на обеспечение обобщенности, на понимание работы родительского класса.
Зато это требование, исполняемое бездумно, уводит от понимания необходимости контроля публичного API объекта и самой идеи организации кода через объекты как изолированные части стейта.
Объект может быть сколь угодно сложен внутри, но его пользователь взаимодействует с ним через публичный интерфейс и для хорошей абстракции нужно что-бы он был понятным и простым. То есть его публичные методы должны быть операциями, переводящими объект из одного корректного состояния в другое и не допускающие неверных состояний.В противовес этому внутренняя структура объекта, его поля и алгоритмы внутри могут допускать потерю консистентности на время - это их работа, сделать что-то с объектом.
Обязать делать все приватные поля публичными - это аналогично рекомендации распотрошить объект, буквально сделать обратное тому, на что направлено ООП.
Вынести все приватные части наружу - по сути аналогичное требование разнести объект и его логику слишком сильно, что противоречит логике High Cohesion.
Отдельной нитью тут идут unit и тесты. Если ты тестируешь объект так, что тебе требуется его внутреннее состояние - ты точно правильно спроектировал объект?
Хотя можно согласиться, что иногда хорошо тестируемый дизайн того требует. Как и многие другие штуки, он не бесплатен и может ухудшать дизайн в принципе и делает код хуже.
OlegZH
25.06.2022 14:24А можно ли пояснить начальный фрагмент кода, для большего понимания происходящего? Почему именно так реализовано? Почему не существует иерархии классов с виртуальной функцией, которая сразу возвращает нужное (в зависимости от содержимого письма)? И т.д. и т.п.
nikialeksey Автор
25.06.2022 14:53Для него нет пояснения, он взят из официальной документации JakartaMail. Получается, что по официальной документации нет нормального подхода для выуживания текста из электронного письма
Cdracm
25.06.2022 14:26+12хе-хе. ха-ха-ха. Как же нам вставить "final" в обьявление "int index" в итераторе? А очень просто: "final AtomicInteger index = new AtomicInteger();". Ха-ха, о май гад.
apro
25.06.2022 15:28этот инструмент написан мной, его цель - удостовериться в некоторой степени в том, что код использует подход ElegantObjects
И один из деклариремых принципов ElegantObjects это "No code in constructors",
а в результате рефакторинга кода почти вся логика в конструктор и переехала,
нет ли здесь каких-либо противоречий?
nikialeksey Автор
25.06.2022 15:33-1Нет противоречий. В моем конструкторе создается абстрактный класс (лямбда в данном случае), в котором и есть код, выполняющий действие. Это нельзя назвать "код в конструкторе", так как он не выполняется при инстанцировании объекта
MentalBlood
25.06.2022 16:00Казуистика какая-то. Жирноватая лямбда получается, но может в EO такое норма
ren-san
28.06.2022 08:31АЗдесь в целом и правда нет проблем как минимум на первый взгляд, ведь код внутри лямбды не исполнится внутри конструктора мгновенно. Разумеется для большей наглядности можно было вынести его в отдельный класс тем самым разгрузив конструктор, но в целом в рамках примера в этом нет необходимости.
Однако полагаю можно было порефакторить и дальше, например сделать более гибким и настраиваемым выбор стратегии для определённого типа контента и т.д. что возможно даже не менее важно чем все переделки в статье
APXEOLOG
26.06.2022 00:18+4NullFree
- нужно писать код без использованияnull
(Почему?)Null это спорная вещь, однако и она помогает в некоторых случаях. Например, возврат
Boolean
вместоboolean
даст Вам три состояния вместо двух (и давайте оставим вопросы боксинга/анбоксинга за кадром)AllFinal
- нужно, чтобы везде стояло ключевое словоfinal
, чтобы поддерживать иммутабельность (Как?)Иммутабельность это здорово, но в некоторых ситуациях она можно негативно влиять на производительность. Да и в целом она зачастую может просто усложнить жизнь, если ее бездумно применять. И да, `final List<TextContent>` не делает вашу коллекцию иммутабельной
AllPublic
- нужно, чтобы все методы и классы были с модификатором доступаpublic
(Почему?)Нет, не нужно
NoMultipleReturn
- нужно, чтобы в методах был только один операторreturn
(Почему? Потому что много операторовreturn
мешают восприятию кода метода. Это не моя мысль, во многих статических анализаторах присутствуют ограничения на количество управляющих операторов в методах, я лишь сделал это ограничение бескомпромиссным)Это вкусовщина. fail-fast и умньшение вложенности улучшает восприятие гораздо лучше, чем мешанина из блоков if-else
Нет никаких "нужно". Есть "подходит" и "не подходит". А некоторые рекомендации (вроде ставить везде
final)
вообще идут из нулевых и давно уже не актуальны. Если Вам так нравится иммутабельность - я бы посоветовал использовать KotlinЧто касается Вашего кода - я бы порекомендовал другой набор правил:
Не использовать одно(двух)буквенные переменные
JEP 305: Pattern Matching for instanceof, Java 14 (https://openjdk.org/jeps/305). В Ваше случае, кстати, можно схлопнуть 2 и 3 блок, если просто делать проверку через `instanceOf Multipart`
Переиспользовать свой же код. Логика первого блока повторяется и во 2 и в 3 блоках. Нужно вынести ее в отдельну функцию и она существенно улучшит восприятие
MentalBlood
26.06.2022 12:25instanceof
По EO это рефлексия и плохо
APXEOLOG
26.06.2022 14:24+1А непроверенный каст может кинуть ошибку в рантайме. Видимо по EO лучше падать с ошибками, правильно получается?
mayorovp
26.06.2022 16:00На самом деле в EO приведения типов также под запретом. Только вот не всегда получается их избежать (и особенно не получается когда иерархия классов на самом деле является типом-суммой).
Andrey_Solomatin
26.06.2022 12:42Вы увлеклись такими продвинутыми вещами как Элегантные объекты, но пропустили очень простую и важную метрику: цикломатическую сложность кода https://en.wikipedia.org/wiki/Cyclomatic_complexity.
Andrey_Solomatin
26.06.2022 12:54-3if (p.isMimeType("text/*")) { result = ... } else if (p.isMimeType("multipart/alternative")) { result = ...
Это можно сделать в ООП стиле.
AbstractHandler handler = GetTypeHandler(p) // теже if-else но выбирается класс обработчика return handler.process()
Суммарно кода будет чуть больше, зато каждый кусочек будет маленьким и простым.
MyraJKee
26.06.2022 21:47+1Какие-то крайности, каждый пункт спорный... Писать свою реализацию итератора, только потому что все объявления должны быть имутабельны?
Aprsta
27.06.2022 10:02NullFree
- не очень совет: если выкидывать исключения каждый раз это просто чудовищная просадка по производительности будет, если объект создавать то тоже не так эффективно и удобно. Для явности можно писать такие методы с названием типа *orNull(). А если еще и на котлине писать то для null там много чего придумано типа явных nullable типов, операторов ?:, функций ?.let, методов isNullOrEmpty итд отказываться от такого удобства ради сомнительной выгоды странноAllFinal
- хорошая штука, но на java очень громоздко получается к каждому классу/методу/переменной писать final, такое количество доп кода может отвлекать от того что он делает. Но опять таки если вы используете котлин проблемы просто нет: там все методы и классы по умолчанию final, а переменные final просто пишутся как val.AllPublic
- это вообще грязь какая то, просто инкапсуляцию на помойку выкидываемNoMultipleReturn
- часто наверное имеет смысл, но если правильно применять паттерн early return, то это больше повышает читабельность кода на самом деле. Пара проверок аргументов в начале метода лучше чем тройная вложенность if
MediaNik5
27.06.2022 10:13+2На удивление получившийся код читается намного хуже, чем исходный, 64 строки в одном методе, метод выполняет как минимум три функции, миллион локальных переменных наподобие листов или итераторов(хотя нам прекрасно жилось без итераторов с c-like фором или с нефайнал переменными вместо листов размером 1, что не является назначением листа), наряду с result, от которого ты не знаешь, чего ожидать в конце метода(или в любой его части ввиду размера самого метода), ещё большей вложенностью if-else, логикой в конструкторе, а не в фабричном методе, который бы делегировал обязанности нужным классам, и разработчику бы вообще не приходилось думать о внутренней реализации.
pavelsc
28.06.2022 01:01А мне нравится как про AllPublic ссылка ведёт на сайт автора, причем нерабочий. Такое потому что потому ????
NullFree - а если проект это проброс json'ин по обменникам, где null вполне себе валидное значение. Эксепшены заставят делать обертки или воротить лишнюю логику с try catch во всех местах.
NoMultipleReturn - ну прям перекрывается всякими stop using else in your program. Обоснованно удобнее читать в начале метода всякие шляпные быстрые early return без ненужных else if. Особенно это помогает в чужом коде, видно что человек обдумал крайние условия и вот они пожалуйста сверху.
Имхо, убеждённое подчинение явно неоднозначным и спорным правилам больше свойственно религии, а не рабочей этике в разработке ПО.
BugM
Очень сомнительный подход.
null free - котлин с таким подходом породил кучку костылей. В реальном мире так обычно и получается. Расставить аннотации над параметрами публичных методов и возвращаемым результатом есть смысл. Тому кто будет вызывать код так проще. В остальном скорее зло, которое замусоривает код.
final где угодно, кроме полей классов, замусоривает код и не несет какого-либо смысла. Тут даже консенсус есть. Область видимости локальных переменных должна быть достаточно небольшой чтобы было понятно что с ними происходит.
1 return - порождает переменную result, и монструозные иногда вложенные блоки if. Часто гораздо проще писать и читать кучку блоков if() {return} if() {return}
Без приватных методов тоже как-то странно. Хочу я вынести кусок логики в отдельный метод. Этот кусок точно не нужен нигде больше в любой разумной перспективе. И почему я не могу это сделать? Зачем я должен изобретать странные конструкции для такого простого действа?
pin2t
Наоборот, отказ от null делает код проще и понятнее, потому что не надо проверять на null
Этот код просто исчезает, и это прекрасно. Котлин просто хочет усидеть на двух стульях, одновременно сохранить совместимость с Java библиотеками, поэтому они добавили новый синтаксис, усложнив язык. Вот и получились костыли.
Аннотации тоже не выход, потому что все усложняют. В результате появляются параметры с аннотациями и без, про которые надо думать как-то по-разному.
Andrey_Solomatin
Задайте поведение по умолчанию через
package-info.java
https://medium.com/square-corner-blog/non-null-is-the-default-
58ffc0bb9111
BugM
Котлин без джава библиотек резко сократит свой потенциальный рынок. Весь бекенд сразу исчезнет. Почему они такой выбор сделали понятно, плата за него тоже понятна.
Вам в вашем проекте совместимость джава библиотеками сделанными по разному тоже нужна.
Разработка это вообще не очень просто. С аннотациями вам среда разработки будет помогать всеми силами. У нее это неплохо получается.
kpmy
Я так понимаю, статья это реклама нового набора Java-заположняков от латентных функциональщиков.