Комментарии 37
Очень сомнительный подход.
null free - котлин с таким подходом породил кучку костылей. В реальном мире так обычно и получается. Расставить аннотации над параметрами публичных методов и возвращаемым результатом есть смысл. Тому кто будет вызывать код так проще. В остальном скорее зло, которое замусоривает код.
final где угодно, кроме полей классов, замусоривает код и не несет какого-либо смысла. Тут даже консенсус есть. Область видимости локальных переменных должна быть достаточно небольшой чтобы было понятно что с ними происходит.
1 return - порождает переменную result, и монструозные иногда вложенные блоки if. Часто гораздо проще писать и читать кучку блоков if() {return} if() {return}
Без приватных методов тоже как-то странно. Хочу я вынести кусок логики в отдельный метод. Этот кусок точно не нужен нигде больше в любой разумной перспективе. И почему я не могу это сделать? Зачем я должен изобретать странные конструкции для такого простого действа?
Наоборот, отказ от null делает код проще и понятнее, потому что не надо проверять на null
if (a != null) Этот код просто исчезает, и это прекрасно. Котлин просто хочет усидеть на двух стульях, одновременно сохранить совместимость с Java библиотеками, поэтому они добавили новый синтаксис, усложнив язык. Вот и получились костыли.
Аннотации тоже не выход, потому что все усложняют. В результате появляются параметры с аннотациями и без, про которые надо думать как-то по-разному.
В результате появляются параметры с аннотациями и без
Задайте поведение по умолчанию через package-info.java
https://medium.com/square-corner-blog/non-null-is-the-default-
58ffc0bb9111
Котлин просто хочет усидеть на двух стульях, одновременно сохранить совместимость с Java библиотеками
Котлин без джава библиотек резко сократит свой потенциальный рынок. Весь бекенд сразу исчезнет. Почему они такой выбор сделали понятно, плата за него тоже понятна.
Вам в вашем проекте совместимость джава библиотеками сделанными по разному тоже нужна.
В результате появляются параметры с аннотациями и без, про которые надо думать как-то по-разному.
Разработка это вообще не очень просто. С аннотациями вам среда разработки будет помогать всеми силами. У нее это неплохо получается.
Ну да, конечно. Вместо проверок a != null у нас везде будут проверки !a.isBlank(). Плюс еще придется придумывать, как различать случаи, когда текст не найден от текст имеется, но он пустой.
Я так понимаю, статья это реклама нового набора Java-заположняков от латентных функциональщиков.
AllPublic - мне это кажется ужасной рекомендацией.
И приведенная для объяснения статья раскрывает ее лишь отчасти, но в целом не говорит о таком требовании. Это уж не говоря про то, что с самой статьей можно поспорить.
Действительно приватные методы и приватные статические методы - это кандидаты на рефакторинг и вынесение в отдельные классы, но это не всегда оправданно. Они могут быть слишком специфичными и не иметь достаточно хорошей обобщенной функциональности, чтобы выносить их. Кроме того само вынесение и обобщение - не бесплантая штука, она потребует дополнительных усилий на поддержание, на обеспечение обобщенности, на понимание работы родительского класса.
Зато это требование, исполняемое бездумно, уводит от понимания необходимости контроля публичного API объекта и самой идеи организации кода через объекты как изолированные части стейта.
Объект может быть сколь угодно сложен внутри, но его пользователь взаимодействует с ним через публичный интерфейс и для хорошей абстракции нужно что-бы он был понятным и простым. То есть его публичные методы должны быть операциями, переводящими объект из одного корректного состояния в другое и не допускающие неверных состояний.
В противовес этому внутренняя структура объекта, его поля и алгоритмы внутри могут допускать потерю консистентности на время - это их работа, сделать что-то с объектом.
Обязать делать все приватные поля публичными - это аналогично рекомендации распотрошить объект, буквально сделать обратное тому, на что направлено ООП.
Вынести все приватные части наружу - по сути аналогичное требование разнести объект и его логику слишком сильно, что противоречит логике High Cohesion.
Отдельной нитью тут идут unit и тесты. Если ты тестируешь объект так, что тебе требуется его внутреннее состояние - ты точно правильно спроектировал объект?
Хотя можно согласиться, что иногда хорошо тестируемый дизайн того требует. Как и многие другие штуки, он не бесплатен и может ухудшать дизайн в принципе и делает код хуже.
А можно ли пояснить начальный фрагмент кода, для большего понимания происходящего? Почему именно так реализовано? Почему не существует иерархии классов с виртуальной функцией, которая сразу возвращает нужное (в зависимости от содержимого письма)? И т.д. и т.п.
хе-хе. ха-ха-ха. Как же нам вставить "final" в обьявление "int index" в итераторе? А очень просто: "final AtomicInteger index = new AtomicInteger();". Ха-ха, о май гад.
этот инструмент написан мной, его цель - удостовериться в некоторой степени в том, что код использует подход ElegantObjects
И один из деклариремых принципов ElegantObjects это "No code in constructors",
а в результате рефакторинга кода почти вся логика в конструктор и переехала,
нет ли здесь каких-либо противоречий?
Нет противоречий. В моем конструкторе создается абстрактный класс (лямбда в данном случае), в котором и есть код, выполняющий действие. Это нельзя назвать "код в конструкторе", так как он не выполняется при инстанцировании объекта
Казуистика какая-то. Жирноватая лямбда получается, но может в EO такое норма
АЗдесь в целом и правда нет проблем как минимум на первый взгляд, ведь код внутри лямбды не исполнится внутри конструктора мгновенно. Разумеется для большей наглядности можно было вынести его в отдельный класс тем самым разгрузив конструктор, но в целом в рамках примера в этом нет необходимости.
Однако полагаю можно было порефакторить и дальше, например сделать более гибким и настраиваемым выбор стратегии для определённого типа контента и т.д. что возможно даже не менее важно чем все переделки в статье
https://www.yegor256.com/2015/05/07/ctors-must-be-code-free.html
No, it’s not. It’s a bad idea for one reason: It prevents composition of objects and makes them un-extensible.
Вместо простого решения мы используем сложное. За это мы получаем решение проблемы, которой у нас нет и вроде как не планируется в ближайшем будущем.
Я читал. Легко чинится добавив ещё конструктор с supplier в аргументах, о чем я собственно упомянул, когда говорил про вынос в отдельный класс.
Так что не убедили, увы
Upd. Если присмотреться, в финальном примере объект создаётся через другой конструктор, у которого в аргументах основной класс обёрнутый в unchecked, поэтому даже мое предложение снимается, потому что уже существует необходимый конструктор.
Мой комментарий про кнонтекст развития данного кода. Парсинг писем достаточно стабильная тема и я не ожидаю какоги-нибудь развития. В этом ключе этот код черезмерно усложнен.
Давайте представим, что это поле будте еще развиваться, и сложность имеет смысл для будущего.
Конструктор с суплаером, в виде интерфейса это отлично.
А вот первый конструктор это что-то интересное.
- Его сложно читать, там огромная вложенность.
- В нем есть вычисления (создаётся лямбда с логикой), что противоречит подходу элегантных объектов.
- Реализация интерфейса знает о других реализациях интерфейса. Иерархия наследования состоит из двух уровней (интерфейс и реализаций), а вот в иерархии зависимостей все реализации равны, но некоторые ровнее.
Этот код напрашивается в отдельный класс. Например отнаследоваться от Scalar и поместить код в value().
в финальном примере объект создаётся
Извините за педантичность, но примеров использования этого кода я не вижу, это абстрактные объекты в вакуме. Каким именно конструктор вызывается первым нам приходится только гадать.
NullFree- нужно писать код без использования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 блоках. Нужно вынести ее в отдельну функцию и она существенно улучшит восприятие
Вы увлеклись такими продвинутыми вещами как Элегантные объекты, но пропустили очень простую и важную метрику: цикломатическую сложность кода https://en.wikipedia.org/wiki/Cyclomatic_complexity.
if (p.isMimeType("text/*")) {
result = ...
} else if (p.isMimeType("multipart/alternative")) {
result = ...Это можно сделать в ООП стиле.
AbstractHandler handler = GetTypeHandler(p) // теже if-else но выбирается класс обработчика
return handler.process() Суммарно кода будет чуть больше, зато каждый кусочек будет маленьким и простым.
Какие-то крайности, каждый пункт спорный... Писать свою реализацию итератора, только потому что все объявления должны быть имутабельны?
NullFree- не очень совет: если выкидывать исключения каждый раз это просто чудовищная просадка по производительности будет, если объект создавать то тоже не так эффективно и удобно. Для явности можно писать такие методы с названием типа *orNull(). А если еще и на котлине писать то для null там много чего придумано типа явных nullable типов, операторов ?:, функций ?.let, методов isNullOrEmpty итд отказываться от такого удобства ради сомнительной выгоды странноAllFinal- хорошая штука, но на java очень громоздко получается к каждому классу/методу/переменной писать final, такое количество доп кода может отвлекать от того что он делает. Но опять таки если вы используете котлин проблемы просто нет: там все методы и классы по умолчанию final, а переменные final просто пишутся как val.AllPublic- это вообще грязь какая то, просто инкапсуляцию на помойку выкидываемNoMultipleReturn- часто наверное имеет смысл, но если правильно применять паттерн early return, то это больше повышает читабельность кода на самом деле. Пара проверок аргументов в начале метода лучше чем тройная вложенность if
На удивление получившийся код читается намного хуже, чем исходный, 64 строки в одном методе, метод выполняет как минимум три функции, миллион локальных переменных наподобие листов или итераторов(хотя нам прекрасно жилось без итераторов с c-like фором или с нефайнал переменными вместо листов размером 1, что не является назначением листа), наряду с result, от которого ты не знаешь, чего ожидать в конце метода(или в любой его части ввиду размера самого метода), ещё большей вложенностью if-else, логикой в конструкторе, а не в фабричном методе, который бы делегировал обязанности нужным классам, и разработчику бы вообще не приходилось думать о внутренней реализации.
А мне нравится как про AllPublic ссылка ведёт на сайт автора, причем нерабочий. Такое потому что потому 😁
NullFree - а если проект это проброс json'ин по обменникам, где null вполне себе валидное значение. Эксепшены заставят делать обертки или воротить лишнюю логику с try catch во всех местах.
NoMultipleReturn - ну прям перекрывается всякими stop using else in your program. Обоснованно удобнее читать в начале метода всякие шляпные быстрые early return без ненужных else if. Особенно это помогает в чужом коде, видно что человек обдумал крайние условия и вот они пожалуйста сверху.
Имхо, убеждённое подчинение явно неоднозначным и спорным правилам больше свойственно религии, а не рабочей этике в разработке ПО.
Спасибо! Поправил ссылку.
Ага, фанатичное подчинение NoMiltipleReturn (мне кажется, это пошло из сишных проектов, где в конце каждой функции проводилось освобождение выделенных в этой функции ресурсов, поэтому держать несколько точек завершения функции было неудобно, доводы за это правило применительно к java при наличии gc, try-with-resource - дело вкуса) и AllFinal привели к натуральным костылям - использованию списков не по назначению. Дальше читать не стал просто.
Автору, видимо, нужен другой язык, где будут синтаксические конструкции, позволяющие обойтись без костылей, соблюдая эти правила (например, в питоне у циклов есть else clause (правда, это не совсем то, что нужно)), но в java это смотрится и читается не очень. На code review первая мысль ревьювера может быть такой: зачем в и без того нетривиальной функции нагружать мозг лишними, не имеющими отношения к логике функции сущностями?
Надо понимать стиль изложения и особенности написания кода Егором https://www.yegor256.com/.
Он излагает рекомендации для конкретных случаев, как аксиомы для любого кода.
Концепции заложенные в элегантных объектах решают много проблем, но и добалвяют некоторую толику сложности. Если у вас нет проблем которые они решают, то они не нужны.
Егор пишет много кода, в том числе библиотек. Элегантные объекты выигрывают на длинных дистанциях и при большом объеме кода.
Очень рекомендую посмотреть его видео они бодрые и провакационные.
nullFree - ужасное, высосанное из пальца требование. null существует с одной целью - показать, что объекта нет. Не текст пустой, а нет объекта текста, по разным причинам.
"Только ситхи возводят все в абсолют".
Верно. И местами оно полезно. Например, никогда не передавать null вместо коллекции. Всегда передавать пустую коллекцию.
Но именно местами. Везде это только вред и дополнительные костыли.
"Нет объекта" это некорректный случай в ОО языке. Не может быть такого что переменная есть а объекта нет. Если объекта нет не объявляйте переменную.
"Нет объекта" это как раз конструкция из языков с указателями, там такое может быть, переменная есть, а область памяти куда она указывает - нет.
В ОО языках null быть не должно. Если есть переменная со ссылкой на объект то должен быть и объект. В ОО языках все объект.
NoMultipleReturn- нужно, чтобы в методах был только один операторreturn
(Почему? Потому что много операторов return мешают восприятию кода метода.
Это не моя мысль, во многих статических анализаторах присутствуют ограничения на количество управляющих операторов в методах, я лишь сделал это ограничение бескомпромиссным)
Обычно, когда мне приходится решать подобные проблемки, и переделывать множественный
returnв одинарный, я поступаю следующим образом.
Я выделяю переменнуюfinal TextContent result;, затем вместо каждогоreturn value;делаюresult = value;.
А разве становится хоть на сколько-нибудь лучше?
Было несколько
returnс возможностью быстрого прерывания циклов, причёмreturnподсвечиваются даже простыми текстовыми редакторами с подсветкой синтаксиса не говоря уж об полноценныхIDE.Стало как минимум столько же присвоений в переменную, причём быстрый выход из цикла приходится городить отдельно что сильно усложняет понимание условий и смысла цикла, плюс нужен ещё и возврат вычисленного значения, и эти присвоения ещё и никем не подсвечиваются.
Требование выполнено, да, но ведь стало хуже.
Какой ужас, сначала прочитал и не понял, зачем кто-то нагородил очередной статический анализатор для Джава да еще с такими кривыми и безумными инспекциями. А потом увидел бейдж “Elegant Objects”…
Статический анализатор, который изменит вашу архитектуру