Бунтарём себя можно считать только тогда, когда люди на самом деле защищают противоположную вашей позицию. Я не согласен с одной из best practices, недавно представленной в Google Testing Blog . Обычно это очень хороший ресурс, ведь этот пост не случайно попал в мою читалку новостей!
Авторы представили две версии функции и спросили, какая из них более читаема.
Можно сразу заметить, что подача немного пристрастна. В дополнение к очевидному цветовому перекосу авторы заменили часть кода справа многоточиями, сделав обе реализации примерно одного размера, хотя на самом деле правая гораздо длиннее.
Авторы заявляют, что функция справа более читаема, потому что не смешивает уровни абстракции, благодаря чему её структура больше отвечает принципу «сверху вниз». Хорошо, допустим, но функция слева линейно читается сверху экрана донизу, а если вы захотите разобраться, что происходит в правой функции, то вам придётся скакать по всему коду.
Вы можете сказать: да, справедливо, но весь код целиком никогда изучать и не нужно, именно для этого и нужна абстракция! Допустим. Так, а теперь ответьте быстро на вопрос: выпекающая пиццу функция выполняет ли также и разогрев печи, или её нужно предварительно разогреть? Подсказка: там есть две функции. Одна называется bake, она получает на входе Pizza, а вторая называется bakePizza…
Ещё один вопрос: что произойдёт, если передать пиццу этим функциям дважды? Они идемпотентны или в конечном итоге вам придётся есть угли?
Итак, как вы уже могли догадаться, мне не нравится стиль оформления кода справа. Но должен признать, его почему-то проще понимать, чем код слева. Благодаря ли это структуре, разделяющей уровни абстракций? Давайте посмотрим. Как насчёт такой версии?
func createPizza(order *Order) *Pizza {
// Готовим пиццу
pizza := &Pizza{Base: order.Size,
Sauce: order.Sauce,
Cheese: “Mozzarella”}
// Добавляем топпинги
if order.kind == “Veg” {
pizza.Toppings = vegToppings
} else if order.kind == “Meat” {
pizza.Toppings = meatToppings
}
oven := oven.New()
if oven.Temp != cookingTemp {
// Разогреваем печь
for (oven.Temp < cookingTemp) {
time.Sleep(checkOvenInterval)
oven.Temp = getOvenTemp(oven)
}
}
if !pizza.Baked {
// Печём пиццу
oven.Insert(pizza)
time.Sleep(cookTime)
oven.Remove(pizza)
pizza.Baked = true
}
// Кладём в коробку и нарезаем на куски
box := box.New()
pizza.Boxed = box.PutIn(pizza)
pizza.Sliced = box.SlicePizza(order.Size)
pizza.Ready = box.Close()
return pizza
}Узнаёте? Это всего лишь код функции слева, к которой в качестве комментариев добавлены имена функций справа.
Не знаю, как вам, а мне он нравится больше всего. И похоже, что читаемость повысилась только благодаря объяснению того, что мы делаем, а не из-за дополнительных слоёв абстракции и косвенных действий.
В заключение хочу сказать, что придерживаюсь приведённого выше мнения: не стоит выделять мелкие функции из линейного кода, особенно если вы используете их только один раз. Никакие преимущества не перевесят потерю линейности.
И ещё одно
Я не был уверен, стоит ли об этом говорить, но в результате отредактировал пост, потому что в этой функции меня беспокоит работа с печью.
Во-первых, предварительный разогрев печи - это автономная операция и она, вероятно, должна быть методом самой печи (oven). Более того, этот код нелогичен: зачем создавать совершенно новую печь для изготовления пиццы? В реальности мы покупаем печь один раз, а затем печём в ней все пиццы, не выполняя весь цикл нагрева заново.
Я знаю, что это синтетический пример, но подобные проблемы на самом деле возникают и в реальном коде, иногда вызывая проблемы с производительностью. Вероятно, этот код должен получать печь в качестве параметра. Её предоставление —задача вызывающей стороны.
(И после того, как мы поместили пиццу в коробку, нам, скорее всего, нужен интерфейс для возврата коробки, а не пиццы. Ну да ладно.)
Комментарии (138)
16tomatotonns
15.09.2023 12:49+45В правом варианте есть ошибка нейминга, функция box перетрёт класс box.
Конкретно в данном случае, больше нравится левый вариант как раз за линейность. Все линейные рецепты должны быть линейными, если им это позволяет сложность и функция не превышает ~сотню строк (24, если правки смотрят через ssh).
Избыточная декомпозиция как правило вредит, как раз по причинам постоянных прыжков. Недостаточная - сложнее модифицируется при меняющихся условиях задачи. Балансируем, ребят.
alexeibs
15.09.2023 12:49+6Я согласен с автором оригинальной статьи, что левый код читается лучше, т.к. фрагмент кода небольшой, все умещается на одном экране. Однако код в камне не высекается и имеет свойство со временем меняться. Если не меняется, то скорее всего этот код никому не нужен и читабельность его может быть какой угодно, т.к. никто его читать не будет. Если же код живой, то вариант слева со временем может вырасти в нечто нечитаемое строк на 1000, а то и более. Первоначальные авторы кода могут со временем уйти в другие проекты/компании etc. Приходят какие-нибудь стажеры. Или просто люди, которым надо по-быстрому запилить фичу здесь и сейчас. Код справа более устойчив к хаотичным правкам, его читабельность со временем будет деградировать медленнее.
ketzal
15.09.2023 12:49+1"24, если правки смотрят через ssh". Подключился я тут давеча со свежайшего компьютера с pentium II и новехоньким ЭЛТ-монитором к серверу код пиццерии поправить ...
DreamC
15.09.2023 12:49+16Я бы сказал что это очень зависит от проекта. Что если кроме пиццы вы готовите еще что-то? Вам придется дублировать код подогрева печи в каждом блюде. Так что нужно знать где и когда как писать, ане бездумно заворачивать все в отдельные функции по одной строчке.
VanKrock
15.09.2023 12:49+3Если рассмотреть покрытие кода тестами, то на левый сложнее писать, так же больше вероятность конфликтов. В книге "Совершенный код" предлагается разбивать код на методы по 7-12 строк
nin-jin
15.09.2023 12:49-4И что же там сложного в тестировании?
Hardcoin
15.09.2023 12:49+2Как вы планируете для левого кода написать тест на то, правильно ли разогревается печь?
nin-jin
15.09.2023 12:49И при чём тут печь? Задача функции - делать вкусную пиццу. Как она там печку греет и использует ли её вообще совершенно не важно.
Hardcoin
15.09.2023 12:49+4Тестирование бывает разного уровня. Если вы хотите только что-то типа интеграционного теста, то конечно без разницы, как греется печка, главное проверить, что пицца в коробке и готова.
Однако юнит-тестирование имеет свои плюсы.
nin-jin
15.09.2023 12:49-2Какие же?
javalin
15.09.2023 12:49+2Фиксация и проверка фрагментов бизнес логики.
boldape
15.09.2023 12:49+5Я с вами не согласен, бизнес логику на практике (а не в выдуманном примере) можно протестировать только интеграционным тестом, юнит тесты имеют смысл только для библиотечного кода, где бизнес логики нет от слова совсем.
Все те тесты, что мнят себе юнит и пытаются в бизнес логику, по факту тестируют моки. Удаляются пачками без сожалений, но самое главное (зависит от языка конечно) могут уродовать АПИ и усложняеть реализацию, только для того, что бы согласно идеологии, писать моки, без которых не получаются юнит тесты.
Ещё раз юнит тестами не надо тестировать бизнес логику они не для этого. Юнит тестами тестируют настоящую логику, математику/вычисления, универсальные алгоритмы и структуры данных, вот тут юнит тесты незаменимы. Юнит тестами тестируют код который может работать как есть в любой прикладной области.
Для тестирования бизнес логики пользуйтесь интеграционными тестами. Бизнес логика - это оксюморон, этими словами обычно именуется самая не логичная часть(и) кода. Правила, а чаще, просто желания людей, кодируются сразу во многих слоях приложения. Так происходит по многим причинам, девелоперы не понимают предметной области и/или языка бизнеса и/или бизнес сам не способен выразить свои хотелки понятными словами и что чаще бизнес не знает чего он хочет в конечном счёте и постоянно меняет правила и требует новой реализации уже позавчера.
Когда нужны моки? Ну когда написать фэйковые зависимости сильно сложнение чем применить моки и/или фэйки работают недостаточно быстро для теста, хотя я ни того ни другого пока не встречал.
Каюсь, я написал буквально тысячи бессмысленных и беспощадных юнит тестов бизнес логики с моками стабами и вот этим вот всем. Теперь почти все мои немногочисленные тесты это интеграционные тесты.
nin-jin
15.09.2023 12:49А плюсы в чём? Фиксацией реализации вы усложнили себе рефакторинг, не получив ничего взамен.
danilovmy
15.09.2023 12:49+2А вот тут не соглашусь. Фиксацией текущего состояния я помогаю себе найти, что сломалось при рефакторинге кода. Иначе - рефакторнул микро тех должок, на автомате, при пролистывании, и через несколько часов рефакторинга уже непонятно, почему не заводится. Мини юниты здорово спасают нервы на огромном проекте и уменьшают время рефакторинга, как бы странно это не казалось коллегам только начинающим работать в том же проекте.
nin-jin
15.09.2023 12:49+1Тесты, что фиксируют код, падают всегда, когда вы этот код меняете. Система контроля версий с задачей поиска изменившегося кода справляется куда лучше. А главное - делает это автоматически, надёжнее и гораздо быстрее.
danilovmy
15.09.2023 12:49Согласен, что журнал изменений - отличный инструмент. Он беспристрастно фиксирует какое изменение было сделано, кем и когда. Но журнал никогда не скажет - работает ли код с изменениями или нет.
Vladivo
15.09.2023 12:49+2У нас, например, 100% покрытие, включая ветвления. Это дисциплинирует, учит писать структурировано, учит писать только то, что нужно, и не писать то, что не нужно. Тест - это ещё и документ, объясняющий не очевидную логику. А спокойствие, которое испытываешь после масштабных рефакторингов и апдейтов просто бесценно.
boldape
15.09.2023 12:49+4Я с вами согласен, но вопрос был по сути не о том. Человек искренне не понимает, а как вообще такое надо тестировать.
@Hardcoin тест был бы, довольно прост. На вход подаем один из типичных заказов. На выходе проверяем соответствие размера, топинга, соуса заказу и опционально просто наличие сыра. Смотрим, что пица запеклась, как то порезана и не голая (в коробке). Степень запекания, цвет коробки, вид нарезки категорически игнорируем, а любое явное упоминание в коде теста печи и прочих инструментов, настоятельно требуем удаления на код ревью. Никаких моков, но если нужно пишем генератор заказов/кастомера, который тупо всегда возвращает один и тот же типичный заказ, и если нужно пишем официанта который только и умеет что взять в руки эту птицу.
Даже если позже мы вынесем код по нарезке, упаковке, то ничего в тестах менять не нужно. И даже, (берегись ерись!) для функции нарезки/упаковки я бы не писал теста т.к. этот код уже покрыт этим тестом.
Hardcoin
15.09.2023 12:49этот код уже покрыт этим тестом.
Не покрыт, так как разнообразные случаи не рассмотрены. Вы же явно игнорируете степень запекания и вид нарезки. Постоянно пригоревшая пицца - не то, что ждут клиенты, возможно код-ревью придется делать в обход вас.
qw1
15.09.2023 12:49+1Да, действительно, у интеграционных тестов есть проблема: как протестировать все варианты параметров каждого из под-процессов. Получится, что интеграционных тестов надо запускать десятки/сотни, когда для тестирования какой-то конкретной функциональности можно было написать те же десятки/сотни тестов, но гораздо более простых, чем большые тесты, гоняющие весь процесс в сборе.
Kergan88
15.09.2023 12:49>Да, действительно, у интеграционных тестов есть проблема: как протестировать все варианты параметров каждого из под-процессов
А зачем тестировать подпроцессы? Это же уже тестирование белым ящиком, плохая практика. Такие тесты часто вреда приносят больше чем пользы.
qw1
15.09.2023 12:49Чтобы проверить опечатки в ветках, специфичных для определённых процессов, которые не попали в большой интеграционный тест.
boldape
15.09.2023 12:49+1О подгоревшей пице мне скажут кастомеры (именно во множественном числе, суть уточнения в том, что одиночные подгорания меня не обеспокоят, у нас правило 10/10 юзеров/эвентов), я заведу баг и когда придет время его фиксить я напишу ещё один тест который будет воспроизводить условия при которых пица подгорает, таким образом я удостоверюсь, что тест полезный он бес фикса будет красным. Затем исправлю код и забуду об этом.
Вы можете не верить, но код покрытый интеграционными тестами тоже покрыт на ~85%, только это не спасает от багов почему то. Забавно то, что баги возникают в том числе и в том коде который покрыт от сюда и до забора.
Покрытие это не цель, особенно покрытие бессмысленно если оно достигается моками.
Мы все знаем, что зелёные тесты не значат, что в программе нет ошибок, при любом покрытии. Зачем же нам тогда тесты? Ну вам не знаю, а нам, что бы детектировать бизнес проблемы. Не любой баг это проблема. Ну и что что пица подгорает раз в пятилетку, у одного кастомера который заказывает голый корж? Из моего опыта, интеграционные тесты это самый экономичный способ мониторить проблемы бизнеса в коде. Когда то я тоже считал, что корректность кода должна быть пре выше всего, но это не всегда разумно.
Hardcoin
15.09.2023 12:49Грустно быть вашим пользователем. Если ваше руководство это устраивает и оно платит деньги, никаких проблем. Но ведь это вопрос, не относящийся к тому, левый вариант хорош или правый.
Просто вы не проверяете даже очевидные случаи. Не те, что раз в пятилетку, а самые очевидные - не сгорит ли каждая пицца (а в коде нет остановки нагрева, возможно, что каждая сгорит). Или проверяете вручную. Но это не делает код слева лучше. Просто вам и так нормально.
qw1
15.09.2023 12:49Вы слишком идеализируете тесты. Если пицца горит из-за отстутсвия в коде остановки нагрева в определённом случае, а тест это проверяет, значит разработчик знает об этой ситуации, обработал её и в тестах, и в коде.
Чаще всего, разработчик вообще не подозревает, что нагрев надо останавливать. На это не написан тест. Соответственно, все тесты зелёные, а пицца на проде — горит.
boldape
15.09.2023 12:49Изначально, был подход в основе которого были юнит тесты, что выразилось в краш рэйте 70%. Это значит 3 из 10 запусков нашего приложения заканчивались крашем. К этому моменту кодовой базе было более 6 лет. С момента смены акцента в тестах на интеграционные и авто / енд то енд прошло почти 5 лет, за это время краш рэйт стал 99,95 это значит только 1 из 2000 запусков заканчивается крашем + функционал значительно вырос. Я обсалютно убежден, что наши пользователи довольны сменой акцента наших тестов. Юниты остались, как и люди которые все ещё продолжают не понимать когда юниты действительно нужны. Забавно, то что это почему то те же самые люди которые топят за слоистые архитектуры, ну те которые дядя боб описывал, все эти контролёры, модели, одна функция не больше вашего любимого магического числа, инверсии здравого смысла под видом зависимостей и обажатели прыжков по кодо базе. Я лично не фанатик и не трогал такого вида код который работал, он для меня просто не существует, но когда мне приходилось чинить один и тот же код раз за разом, то я выпиливал все это вместе с юнит тестами.
Hardcoin
15.09.2023 12:49Я не понимаю, как приложение может падать. Вероятно, ко мне оно приходит уже в более приличном виде, но мысль, что программист отправит в репозиторий падающий код меня удивляет. За ваш опыт спасибо, вероятно он сильно отличается и ситуация другая.
При чем тут магическое число не ясно, видимо вы хотите приписать мне то, что я не говорил.
boldape
15.09.2023 12:49У нас десктоп + эмбед на с++. Это значит любое не тривиальное приложение на плюсах падает без исключений, таков с++ с этим ничего не поделаешь.
Программист даже не догадывается, что новый код приводит к падениям, это нормально.
Магическое число к вам не имеет отношение, это отсылка к правилу где произвольным образом выбирается "правильная" длина функции в строках кода.
ReadOnlySadUser
15.09.2023 12:49+3Врагу не пожелаю писать код в проекте, состоящем из функций в 7-12 строк :D
vladvul
15.09.2023 12:49+1ваш линейный вариант не асинхронен. Программа блокируется пока пицца не приготовится. Правый вариант типичные асинхронные вызовы с коллбэком
lorc
15.09.2023 12:49+3Это код на С++ и там нет ни одного коллбэека.
Zara6502
15.09.2023 12:49+1это точно не срр, ниже написали что это го
lorc
15.09.2023 12:49+1Да, сорри, туплю. Не выспался, однако. Но коллбеков там все равно нет.
MiraclePtr
15.09.2023 12:49+1добавьте 'await' (или альтернативу из вашего языка) к вызову функций - и вот уже у вас асинхронный код с невидимыми коллбэками :)
axifive
15.09.2023 12:49+1Вероятно, этот код должен получать печь в качестве параметра. Её предоставление —задача вызывающей стороны.
Получать печь в качестве параметра тоже не очень удачная идея, тут синглтон прямо напрашивается
SAPetrovich77
15.09.2023 12:49+12Не согласен. В пиццерии не всегда печь в одном экземпляре. Скорее тут синглтоном должен быть пулл печей
Zara6502
15.09.2023 12:49я бы оформил вот так:
func createPizza(order *Order) *Pizza { // Готовим пиццу pizza := &Pizza{Base: order.Size, Sauce: order.Sauce, Cheese: “Mozzarella”} // Добавляем топпинги if order.kind == “Veg” pizza.Toppings = vegToppings else if order.kind == “Meat” pizza.Toppings = meatToppings oven := oven.New() if oven.Temp != cookingTemp for (oven.Temp < cookingTemp) { // Разогреваем печь time.Sleep(checkOvenInterval) oven.Temp = getOvenTemp(oven) } if !pizza.Baked { // Печём пиццу oven.Insert(pizza) time.Sleep(cookTime) oven.Remove(pizza) pizza.Baked = true } // Кладём в коробку и нарезаем на куски box := box.New() pizza.Boxed = box.PutIn(pizza) pizza.Sliced = box.SlicePizza(order.Size) pizza.Ready = box.Close() return pizza }Это какой-то Си подобный язык? Меня смущает строка
box := box.New()тут присваивание как в Паскале, хотя следом в строках ниже - как в Си.axifive
15.09.2023 12:49+2это Go,
:=- объявление переменной с присвоением, без явного указания типа
https://go.dev/ref/spec#Short_variable_declarations
perfect_genius
15.09.2023 12:49А в Go можно переносить вот так?:
if order.kind == “Veg” pizza.Toppings = vegToppings else if order.kind == “Meat” pizza.Toppings = meatToppingsТак мозгу-глазу удобнее.
Rsa97
15.09.2023 12:49+26Ну, для сферической пиццы в вакууме код подходит. Но если приблизить его к реальности — то сыр закончился, то печь неисправна и не греется или температура гораздо выше нормы, то коробки остались только маленькие, а пицца большая, то клиент попросил порезать не на шесть кусков, а не семь… В результате единая функция распухнет и будет обмазана таким количеством условий, что без стакангенса не разберёшься.
vadimr
15.09.2023 12:49+5В декомпозированной функции это будет ещё хуже. Будут по всему колхозу передаваться многочисленные флаги, что пошло не так на предыдущих этапах. Если, конечно, не принять волевое решение уничтожать любую пиццу, при готовке которой откинулся эксцепшен или что там у неё.
Обрисованная проблема-то фундаментальная – реальный процесс может быть не так линеен, как простейшая модель – а не связанная с оформлением кода.
vvbob
15.09.2023 12:49+2А зачем передавать флаг - "печь неисправна"? Это уже означает что всё, процесс завершен, полуготовую пиццу придется выбросить, сырое тесто вы ведь не повезете клиенту, как и угольки вместо пиццы. Это и есть критикал эксепшен, который должен прервать весь процесс. А что касается всяких особых пожеланий при готовке - их можно передавать функциям в объекте (в структуре, если мы не в объектном стиле пишем), эта информация все равно должна где-то храниться и обрабатываться, глобальные переменные нифига не лучше для этого приспособлены. Эксепшены, которые можно разрешить - типа "есть только маленькие коробки - а пицца большая - при согласии клиента, разложим ее в несколько коробок" так-же можно в отдельных функциях обрабатывать.
Hardcoin
15.09.2023 12:49+4Будут по всему колхозу передаваться многочисленные флаги, что пошло не так на предыдущих этапах
Это если вы пытаетесь писать код в левом стиле, но в отдельных функциях. Не надо так. И флаги не надо. Если у вас печь не работает, вам не нужен такой флаг при упаковке пиццы в коробку, вам вообще пиццу в коробку упаковывать не надо, она же не готова.
vadimr
15.09.2023 12:49-4Может клиенту сырая пицца - лучше, чем никакой?
У вас такой максималистский подход, что в одном месте сломалось - значит всю работу останавливаем. Не везде он приемлем, и в управлении технологическим процессом - точно нет.
Если у вас реальная пиццерия с тысячью заказов, тысячью полуфабрикатов и сломавшейся печью, то видятся, например, такие альтернативы:
выкинуть все полуфабрикаты, аннулировать все заказы (это то, что предлагаете вы), попасть при этом на себестоимость и на штрафы от наиболее недовольных клиентов;
предложить клиентам сырую пиццу за полцены в надежде, что хотя бы кто-то согласится;
отправить сырую пиццу в коробках в другую фирму, которая согласилась допечь, и далее по клиентам.
Hardcoin
15.09.2023 12:49+4Вам хоть раз доставляли/приносили в ресторане полуготовую еду вместо заказанной? Никто так не делает, но вы в своем бизнесе можете быть первым, если хотите.
А по поводу последнего пункта - если у вас контракт с другой фирмой есть, то конечно - пишете другой бизнес-процесс (собираете из готовых функций), где вместо печи - поездка в другую фирму и исполняете его. Но у вас должны быть готовые функции. Встроить подобную логику в линейную лапшу будет крайне проблематично, не правда ли?
vadimr
15.09.2023 12:49Вам хоть раз доставляли/приносили в ресторане полуготовую еду вместо заказанной?
Именно единственная печка, как в вашем примере, в ресторанах ломается очень редко, поэтому с полуготовой едой не сталкивался (хотя предпочёл бы недопечёную пиццу никакой в ряде сценариев). Но другие согласованные отклонения от рецептуры бывали.
А по поводу последнего пункта - если у вас контракт с другой фирмой есть, то конечно - пишете другой бизнес-процесс (собираете из готовых функций), где вместо печи - поездка в другую фирму и исполняете его. Но у вас должны быть готовые функции. Встроить подобную логику в линейную лапшу будет крайне проблематично, не правда ли?
Предусмотрительный программист реализует альтернативные пути сразу. И именно поэтому не будет линейных последовательностей вызовов.
В конечном итоге в такой программе структурно образуется что-нибудь вроде цикла обхода состояний конечного автомата (если программист понимает, что он делает), или просто лапша (если не очень).
vvbob
15.09.2023 12:49+1Недопеченная пица.. ну не знаю, корочка - внутри сырое тесто, не до конца обработанные термически продукты, можно просто отравиться.. Может вам и нормально но ни один уважающий себя бизнес такое клиенту не повезет.
Hardcoin
15.09.2023 12:49Предусмотрительный программист реализует альтернативные пути сразу.
Оверинжиниринг не всегда приветствуется.
И именно поэтому не будет линейных последовательностей вызовов.
Тут мы с вами достигли согласия, кажется. Линейный код слева не очень подходит.
vadimr
15.09.2023 12:49Никто так не делает, но вы в своем бизнесе можете быть первым, если хотите.
В моём бизнесе неполные технологические процессы – обычная практика жизни. Причин, препятствующих выполнению отдельных операций, очень много, а условный полуфабрикат – дорогой, не разбросаешься.
Поэтому логически каждая операция имеет, как практический минимум, три возможных результата, которые необходимо обрабатывать: операция выполнилась успешно; операция выполнилась неуспешно; во время операции возникло внешнее событие, отменяющее её выполнение. В нашем примере это будут, например: пицца испеклась; пицца постояла в печке, но не испеклась; в печку вместе с пиццей забрался кот.
Alexeyslav
15.09.2023 12:49Не ясно как себя поведут компоненты при попытке второй раз приготовить, гарантировать при этом БЕЗОПАСНОСТЬ продукта никто не возьмётся. И во вторых, чисто логистически разрабатывать и применить систему решений что-делать-дальше-если-.... дороже чем изготовление одной единицы продукта. А ещё нужна система датчиков чтобы ПОНЯТЬ ЧТО С ПИЦЦЕЙ НЕ ТАК, чтобы система принятия решений смогла принять решение. Проработать все варианты возможные, отладить это всё и протестировать.... тоже желательно во всех возможных вариантах, а не на производстве уже. Итого, система автоматической "доготовки" пиццы просто разорит предприятие. Проще выкинуть неудачный экземпляр, даже если это будет одна из сотни.
И кстати, один только факт что на выходе есть недоготовленные изделия может нести репутационные риски с последующим банкротством.
Vcoderlab
15.09.2023 12:49+14Присоединяюсь: в голосовалке нет варианта "ни один не нравится". Какой-то винегрет из ООП и процедурного программирования
Конкретно мне кажутся нелогичными такие места, как например
oven.Temp = getOvenTemp(oven)Если печь у нас объект, то у неё должен быть метод
oven.GetTemp(), возвращающий температуру. Странно узнавать состояние объекта с помощью сторонней функции. Также в моём понимании процесса у печи должен быть блокирующий методoven.Heat(Temp), нагревающий печь до нужной температуры и только потом завершающийся, и асинхронный методoven.SetTemp(Temp), задающий новую уставку температуры и завершающийся немедленно.ReadOnlySadUser
15.09.2023 12:49getOvenTemp(oven) тестировать легче, чем oven.GetTemp().
Но сама по себе функция имеет плохое имя и дизайн, тут согласен)
Vcoderlab
15.09.2023 12:49+4Хм...
Вот у нас есть объектoven. У этого объекта есть свойствоoven.Temp,где хранится его температура. Однако чтобы это свойство имело актуальное значение, необходимо записать это значение в него вручную (oven.Temp = getOvenTemp(oven)), вызвав некую функциюgetOvenTemp(), в которую в качестве параметра передаётся сам объектoven. И здесь у меня возникает несколько вопросов именно с программистской точки зрения:раз уж мы передали в функцию
getOvenTemp()сам объектoven, почему эта функция не может сама установить этому объекту значение свойстваoven.Temp? Почему мы должны делать это вручную?Как это реализовано технически? Откуда функция
getOvenTemp()берёт значение температуры? Если из самого объектаoven, то почему мы не можем сделать то же самое без вызова этой функции? Если откуда-то извне, то как так получилось, что свойство объекта определяется какими-то внешними условиями?
Я не считаю себя профессиональным программистом, но даже на мой непрофессиональный взгляд это не код, а какая-то каша. ИМХО нельзя выставлять такое в роли образца для подражания. Впрочем я допускаю, что такое моё мнение - как раз следствие моего непрофессионализма. В таком случае буду рад разъяснениям, почему в обсуждаемом коде всё сделано правильно.
ReadOnlySadUser
15.09.2023 12:49+1почему эта функция не может сама установить этому объекту значение свойства oven
Потому что функция хочет быть чистой. Чистые функции предсказуемы и крайне надёжны. Но от этого эпизодически страдает производительночть.
Если из самого объекта oven, то почему мы не можем сделать то же самое без вызова этой функции
Потому что это модификация состояния, что не всегда требуется.
Де факто, код справа переписан в функциональном стиле (хотя и не очень удачно), что позволяет получить определенные преимущества, особенно в контексте асинхронной/параллельной обработки пиццы.
Однако чтобы понять суть этих преимуществ, нужно хотя бы поверхностно окунуться в функциональные языки, на этом конкретном примере продемонстрировать их не получится.
nin-jin
15.09.2023 12:49+1Чистые функции так и норовят вызвать ООМ. Очень надёжно, да.
ReadOnlySadUser
15.09.2023 12:49+1На моей памяти такого никогда не случалось (я имею ввиду, чтобы в погоне за чистотой, случился ООМ).
Написание кода в функциональном стиле - это хороший подход в очень многих ситуациях, я крайне его рекомендую и всячески пропагандирую. В более или менее приличных языках, имеющих хоть какую-то внятную (не обязательно хорошую) систему типов, компилятор способен оптимизировать оверхед.
Тем не менее функциональные языки мне не нравятся, на них жутко неудобно писать обработку ошибок и просто поразительное количество костылей нужно, если хочется повозиться с состоянием.
Поэтому мой совет - во всём нужно знать меру) Второй совет - используйте языки, позволяющие смешивать императивщину и функциональщину и будете счастливы)
nin-jin
15.09.2023 12:49Пример из моей практики: хеш-таблица на 1М записей. Надо добавить ещё одну запись - изволь скопировать несколько десятков мегабайт и надеяться, что в памяти есть непрерывная область достаточного размера. Про нейронки с гигабайтными моделями, которые даже в однократном экземпляре не всегда влезают в память, я даже не заикаюсь.
Kergan88
15.09.2023 12:49Так надо иммутабельный хеш юзать
nin-jin
15.09.2023 12:49Это и есть иммутабельный хеш. Вы возможно имеете ввиду какой-нибудь btree, но у него асимптотика чтения сильно хуже.
Kergan88
15.09.2023 12:49>Это и есть иммутабельный хеш.
Тогда зачем он десятки мегабайт копирует при вставке?
qw1
15.09.2023 12:49А смысл такого "хеша", если у него сложность добавления элемента O(N)? Чтобы вставить 1M элементов, нужно 1T времени.
Это самописный контейнер или из какой-то библиотеки?
Kergan88
15.09.2023 12:49Обычно иммутабельные хеши делают на hamt, и его целиком копировать не требуется.
DirectoriX
15.09.2023 12:49Очень интересный функциональный стиль, когда почти все функции мутируют подаваемые на вход данные вместо создания новых...
Да и выполняться это должно в строго фиксированном порядке (если, конечно, вы не хотите иметь сырые овощи поверх коробки, или разогревать печь уже после "выпекания"), что подразумевает сайд-эффекты / монады, которые... ах да, которые позволяют в функциональщину добавить императивщину.
ReadOnlySadUser
15.09.2023 12:49+1Как я и сказал - реализация и дизайн - оч плохи. Я бы предпочел код слева тому, что справа. Но задумка была в этом)
Tony-Sol
15.09.2023 12:49+6Конкретно здесь, в примере, оба варианта плохи, но я бы предпочел увидеть скорее правый, чем левый.
Левый вариант нарушает, как мне кажется, базовое правило декомпозиции - все делается в одном месте, вместо того, чтобы иметь отдельные маленькие, понятные и тестируемые функции.
Это, кстати, можно видеть и в правом варианте (bake создает oven - почему?, зачем?), но в куда меньшей степени.
ikonvpalto
15.09.2023 12:49+1Для меня правый читается сложнее
... в первый раз
Но, если я в ближайшее время продолжу работать с этим кодом, то я буду приблизительно помнить ответы на перечисленные выше и любые другие вопросы по поводу функций."А если кто-то поменяет код, и твои ответы устареют?" - Я либо на этапе код ревью увижу изменения, либо и при работе с левым кодом ответы устареют, и я столкнусь ровно с такой же проблемой
michael_v89
15.09.2023 12:49+6В статье от Google высказана правильная мысль "Avoid mixing different abstraction layers into a single function", только их пример неправильно ей следует.
Действительно, не должно быть смешивания низкоуровневой логики и высокоуровневых абстракций. Из этого например следует, что одна публичная функция не должна вызывать другую публичную функцию, они обе должны вызывать приватную. Но следует так же и избегать большого количества промежуточных уровней абстракции. Следует стремиться к 2-3 уровням — высокоуровневое название публичного метода, высокоуровневые шаги для выполнения действия в виде вызовов приватных методов, технические детали реализации шагов. Во многих случаях высокоуровневый шаг всего один — вызвать реализацию действия, и его можно не указывать.
В примере с пиццей их несколько. Если мы хотим сделать максимально независимые шаги, которые можно переиспользовать в разных методах, то надо делать так.
func createPizzaInBox(order *Order) *Box { pizza = createNewPizza(); addTopings(pizza, order); if !pizza.Baked { oven := getOvenDevice(); heatUpOven(oven); bakePizza(oven, pizza); } box := takeNewBox(); packPizza(pizza, box); slicePizzaInBox(box, order); closeBox(box); markAsReady(box.pizza); return box; }Все шаги на верхнем уровне, представляют собой только вызовы методов и их условия. Условия выполнения шагов, связанные с бизнес-логикой, надо тоже писать на верхнем уровне, а не прятать в детали реализации.
Если мы хотим сделать на верхнем уровне более крупные шаги, то можно сделать так.
func createPizzaInBox(order *Order) *Box { pizza = createNewPizza(order) if !pizza.Baked { bakePizza(pizza) } box := packPizza(pizza) return box }Но тогда в этих методах надо сразу указывать детали реализации, не разбивая дальше. Иначе, как правильно заметил автор статьи, программисту, который не писал этот код, будет сложнее разбираться, как он работает, надо будет держать в голове контексты нескольких мелких функций и перемещаться между ними туда-сюда.
func bakePizza(pizza *Pizza) { oven := getOven() if oven.Temp != settings.cookingTemp { for (oven.Temp < cookingTemp) { time.Sleep(settings.checkOvenInterval) oven.Temp = getOvenTemp(oven) } } oven.Insert(pizza) time.Sleep(settings.cookTime) oven.Remove(pizza) pizza.Baked = true }Напрашивается мысль, что прогрев печи все-таки лучше вынести в отдельный метод. Если поместить все такие методы в этот же класс, то появится виртуальная сущность "Группа методов, связанных с печью". Это как раз признак того, что класс перегружен, и правильнее вынести этот код в отдельный компонент.
func bakePizza(pizza *Pizza) { oven := getOven() oven.heatUp(settings.cookingTemp); oven.bakePizza(pizza); pizza.Baked = true }Тут тоже можно сказать, что другому программисту будет сложнее разбираться, но в данном случае детали реализации находятся в отдельном компоненте. Его проще изучать, он может использоваться и в другом коде, а также программист может решить "А, это печка, мне точно не нужны эти детали". Что нельзя сделать если это оформлено как детали приготовления пиццы, которые программисту как раз и нужны.
ImagineTables
15.09.2023 12:49одна публичная функция не должна вызывать другую публичную функцию, они обе должны вызывать приватную
В чём смысл? Если у нас есть, например, сахарная Window::Display(), которая выводит окно в центр экрана и гарантирует, что оно будет отображаться, что мешает в её имплементации вызвать публичные Window::Show() и Window::Move()? Обязательно городить Window::ShowImpl()/Window::MoveImpl()?
michael_v89
15.09.2023 12:49Ну представьте, что во всех 3 функциях вызывается какое-нибудь логирование в начале и в конце. Для Display() нам надо сделать работу Show() и Move(), но залогировать только "Display begin / Display end". Это относится к любой дополнительной работе, которую нежелательно повторять или вкладывать — начало/коммит транзакции, установка/снятие мьютексов, подсчет количества вызовов отдельно для каждой функции и т.д.
Исключения бывают везде, но для поддерживаемости лучше начинать с такого подхода, и менять его если есть причины.
ImagineTables
15.09.2023 12:49Ну представьте, что во всех 3 функциях вызывается какое-нибудь логирование в начале и в конце. Для Display() нам надо сделать работу Show() и Move(), но залогировать только "Display begin / Display end".
По-моему, это логическая ошибка «бесконечного регресса»: когда объяснение неявно ссылается на само себя.
Если нормально относиться к тому, что публичная функция может вызывать публичную функцию, откуда возьмётся странное требование Для Display() нам надо сделать работу Show() и Move(), но залогировать только "Display begin / Display end"? В логах будут «скобки» и «скоупы», это совершенно нормально. Я такие логи разбирал, ничего страшного в них нет. Ненормальным это кажется, только если взять за аксиому изначальное правило одна публичная функция не должна вызывать другую публичную функцию.
Это относится к любой дополнительной работе, которую нежелательно повторять или вкладывать — начало/коммит транзакции
А как транзакционность связана с обсуждаемым правилом? По-моему, это частный случай проектирования, особенные функции, и из них нельзя вывести универсальное правило, связанное с видимостью.
michael_v89
15.09.2023 12:49откуда возьмётся странное требование
От бизнеса или от техлида.
Я такие логи разбирал, ничего страшного в них нет.
Я не сказал, что в них есть что-то страшное. Я сказал, что бывают ситуации, когда надо логировать внешние вызовы и не логировать внутренние. Логи бывают не только в файлы для программиста, но и в базу для бизнеса. И если там будет в 2 раза больше каких-нибудь записей о платежах чем фактически, вряд ли его это устроит.
если взять за аксиому изначальное правило
Это не аксиома, а практическое улучшение поддерживаемости кода. Поддерживаемость подразумевает адаптацию кода к изменяющимся требованиям. Сегодня не надо было логировать, завтра надо. Сегодня не было транзакций, завтра появились. Ну и упомянутое смешение уровней абстракций, что усложняет понимание для того, кто не писал этот код. Собственно, появление новых требований как раз и проявляет разницу уровней абстракции. Если не хотите улучшать поддерживаемость, это дело ваше.
А как транзакционность связана с обсуждаемым правилом?
Так, что это пример дополнительной работы, которую нежелательно повторять или вкладывать, из-за которой лучше следовать этому правилу.
Или вот такой пример.
Display() { Show(); Move(); } Show() { this.ensureScreenNotNull(); data = this.screen.getData(); ... } Move() { this.ensureScreenNotNull(); data = this.screen.getData(); ... } ensureScreenNotNull() { if (this.screen === null) { this.screen = System.getScreen(); } }В вашем варианте при вызове Display() функция ensureScreenNotNull() вызывается 2 раза, хотя достаточно 1. В каком-то другом коде в такой функции может быть тяжелая инициализация. Которая могла понадобиться не сразу, а через некоторое время после написания первоначального кода. Чтобы избежать лишних вызовов, придется рефакторить.
Если придерживаться этого правила, код будет более понятным, и придется меньше рефакторить при изменениях требований.
qw1
15.09.2023 12:49Видел я такой код, где все низкоуровневые действия обёрнуты в функции, зачастую в функции-однострочники:
void ShowWindow(Window window) { window.Show(); } void SetGlobalState(int state) { g_State = state; }Может, это чем-то и оправдано, но мне показалось это лишней когнитивной нагрузкой, возникшей на пустом месте. Мало того, что надо помнить стандартную библиотеку и назначение переменных, теперь ещё и запоминать название и действия всех этих функций-однострочников, чтобы, когда их видишь, лишний раз не листать куда-то вниз, посмотреть, что они на самом деле делают.
michael_v89
15.09.2023 12:49А где тут публичные функции, вызывающие ненужные приватные? Это немного не то, о чем я говорю. Я говорю, что например в данном случае другая публичная функция для установки состояния должна вызывать не
SetGlobalState(), аg_State = state. Вдруг вы завтра сделаете там проверки нового состояния с выбросом исключения, а в другой функции надо на время ее работы поставить невалидное состояние, которое она в конце работы превращает в валидное.qw1
15.09.2023 12:49Это буквальное следование принципу
правильная мысль "Avoid mixing different abstraction layers into a single function"
Не говорю, что это неправильно. Но читать это было… кхм… трудновато.
michael_v89
15.09.2023 12:49Не могу сказать, что в этом коде есть буквальное следование этому принципу. window.Show() не является низкоуровневым действием для ShowWindow(), это один уровень абстракции.
На самом деле, если у нас есть функция ShowWindow(), она не должна вызывать window.Show(). Это может сделать и сам вызывающий код. То есть или мы имеем ShowWindow() с внутренними шагами, или window.Show() с теми же внутренним шагами. Это можно сделать по техническим причинам для совместимости (например, для упрощения перехода между мажорными версиями библиотек), но не как самоцель.
qw1
15.09.2023 12:49Значит, автор кода неверно понял принцип. Но в его функциях я не видел смешивания обычных присваиваний (типа
g_state = newState) и вызова других функций. Я подумал, что это требование гугловского принципа.
ImagineTables
15.09.2023 12:49От бизнеса или от техлида.
А почему требования какого-то конкретного техлида выдаются за основания для соблюдения универсального правила о невызове публичных функций из публичных? ;)
Я всё подводил-подводил к упоминанию волшебных слов «API» или «библиотека», но, видимо, стоит раскрыть карты и написать прямо.
Я давно обратил внимание, что библиотечный код (включая сюда разные API) и прикладной код — это два совершенно разных вида кода. А ещё я обратил внимание, что когда их путают, получаются разные конфузы.
Самый большой конфуз, на мой взгляд, случился, когда Степанов объяснил, что он старался сделать свою библиотеку максимально дружественной к тем, кто пишет алгоритмы (в сиплюсплюсном понимании этого слова). Но большинство программистов не пишет алгоритмы (в сиплюсплюсном понимании этого слова), а только использует их. А вот к тем, кто использует алгоритмы, его библиотека, ставшая стандартной, повёрнута задом. В отличие от дотнетной, например. Или того же qt.
В данном же случае я думаю, что эту идею можно сформулировать так. Если мы заменим слова «публичная функция» на «библиотечная функция» или «функция API», то я, пожалуй, соглашусь, что есть причины не вызывать их друг из друга. Потому, что там чёткая граница «прикладной код» — «библиотека/API» (и логгирование пересечений этой границы, например) действительно имеет смысл.
А если мы говорим про обычный класс в прикладном коде, то общего смысла в этом нет. Есть только набор частных случаев типа «потому, что техлид так сказал» (довольно плохое объяснение для правил проектирования, не так ли?).
michael_v89
15.09.2023 12:49А почему требования какого-то конкретного техлида выдаются за основания для соблюдения универсального правила
Потому что это не требования конкретного техлида, а часто встречающаяся ситуация с требованиями от разных техлидов или представителей бизнеса.
Если мы заменим слова «публичная функция» на «библиотечная функция»
Нет, эта рекомендация работает в рамках любого класса с публичными функциями. Если вы пишете реализацию — пишите реализацию, не вызывайте абстракцию из реализации.
Есть только набор частных случаев
Любой код это частный случай. Из набора частных случаев складывается статистика. Вот как раз с моей точки зрения вы придумали какой-то частный случай, где это необязательно, и пытаетесь с его помощью доказать, что эта рекомендация неправильная для большинства случаев.
Рекомендация правильная, даже если из нее есть исключения. Это ведь не только я говорю, вон даже Google это в блоге написали — "Avoid mixing different abstraction layers". Без кода функций Display(), Show() и Move() нет смысла спорить о деталях. Если дадите ссылку на конкретную реализацию, можем обсудить.
qw1
15.09.2023 12:49Ок. Если требуется реализовать интерфейс с функциями Move, Show и т.д., мы сразу пишем заготовку
void Show() { ShowImpl(); } void Move() { MoveImpl(); }и т.д., и уже спокойно вызываем MoveImpl из ShowImpl.
Но эти однострочные функции нарушают принцип YAGNI.
Вот когда потребуется логирование, тогда и отрефакторим. А заранее забивать себе голову этими Impl… Наверное, можно к этому привыкнуть, если "техлид" так хочет.michael_v89
15.09.2023 12:49мы сразу пишем заготовку
А заранее забивать себе голову этими Impl…Обычно бывает наоборот, у нас уже есть публичная функция без всяких Impl, мы делаем новую, и хотим вызывать существующую публичную. Если сейчас вынести во внутреннюю функцию, то это будет не заранее, а вовремя.
Функции типа Display(), которые вызывают публичные функции в нужном порядке, часто помещают в отдельный класс Builder/Manager/Utils. А внутри класса лучше работать с деталями реализации. То есть например вызывать
this.property = valueвместоthis.setProperty(value). Публичные функции предполагают, что состояние объекта до и после вызова является валидным, что может быть не так если такой вызов это часть логики другой публичной функции.
michael_v89
15.09.2023 12:49Обязательно городить Window::ShowImpl()/Window::MoveImpl()?
Иногда имеет смысл скопировать в Display() внутренние шаги Show() и Move() со специфичными для Display() параметрами. Тогда добавление новой функциональности в Show() и Move() не повлияет на Display(). Зависит от того, подходит ли нам такое поведение, или нам надо любую новую функциональность повторять в Display().
ImagineTables
15.09.2023 12:49Это будет нарушением DRY (нормализации), а DRY это очень фундаментальный принцип, который хорошо обоснован. Конкретно: опасность, что в Show()/Move() при вызове «внутренних шагов» появится какое-нибудь важное условие, а мы его забудем добавить в Display(), гораздо больше, чем та, что новая функциональность всё сломает. Тем более, Display() упомянута как «сахарная».
michael_v89
15.09.2023 12:49Вот как раз если не хотите нарушать DRY, то надо сделать ShowImpl(). Но на самом деле так редко бывает. Обычно в функциях типа Show() есть больше одного шага, из которых в Display() надо вызвать только главный.
опасность, что в Show()/Move() при вызове «внутренних шагов» появится какое-нибудь важное условие, а мы его забудем добавить в Display()
Неа) Если мы поменяем внутренний шаг, который вызывается и из DIsplay() и из Show(), то поведение поменяется в обоих. Просто очень часто со временем в Show() появляются свои требования, которые не нужны для Display(), отсюда и рекомендация не вызывать одну из другой.
SergeiMinaev
15.09.2023 12:49+5Узнаёте? Это всего лишь код функции слева, к которой в качестве комментариев добавлены имена функций справа.
Комментарии легко теряют актуальность, если при изменении кода про них забыли. Поэтому лучше сводить использование строк в коде к минимуму. Это как при работе с апи надёжнее писать что-то вроде api.order.get(id), чем request(f'/api/odrer/{id}'), потому что если во втором случае будет допущена опечатка, то она может быть замечена не сразу.
antonkrechetov
15.09.2023 12:49+1Комментарии легко теряют актуальность, если при изменении кода про них забыли.
Иногда вижу это замечание, когда речь захоит о коммитах, и оно всегда кажется мне диким. Да, блин, комментарии — это важная часть разработки, и нужно поддерживать и в актуальном состоянии. Такая же важная, как написание и поддержка документации. Как и составление правильных и полезных комментариев коммитов в гите. Как написание кода, наконец.
Vladivo
15.09.2023 12:49Насчёт коммитов вы абсолютно правы, но коммиты не устаревают, в отличие от комментариев. Для меня комментарий - это всегда "запах" и повод задуматься над тем, достаточно ли выразителен сам код. Как правило, я предпочту что-то вроде вериЛонгВарайаблНэйм, потому что комментарий, как вы сами сказали, это дополнительные расходы на поддержку. Не хочу дополнительных расходов там, где можно обойтись без них.
DirectoriX
15.09.2023 12:49Я бы сказал, что нужность / оправданность комментариев сильно зависит от самих комментариев и как они используются.
Например, комментарии к модулям, классам и заголовкам функций (типа JSDoc / Doxygen / Rustdoc / etc.) часто используются IDE для показа подсказок при автодополнении или для автогенерации онлайн-документации - их определённо стоит поддерживать, хоть это и дополнительная работа.
С другой же стороны, что-то вроде
// convert inches to millimeters length *= 25.4;действительно лучше было бы выделить в функцию с говорящим названием, тогда и комментарий стал бы не нужен.
Vladivo
15.09.2023 12:49Вы знаете, я бы выделил докблоки в отдельную категорию. Да, пишутся как комментарии и почти всегда содержат некоторое описание на человеческом языке, но интеграция с IDE делает их скорее надстройкой над языком и чем-то большим, чем просто "инфа здесь и сейчас".
antonkrechetov
15.09.2023 12:49+1
Это опечатка, я имел в виду «когда речь заходит о комментариях»)Иногда вижу это замечание, когда речь захоит о коммитах
Насчёт коммитов вы абсолютно правыДля меня комментарий — это всегда «запах» и повод задуматься над тем, достаточно ли выразителен сам код.
Каким бы выразительным ни был код, он достаточно хорошо документирует, в основном, поведение, то есть «что в этом месте происходит». Состояние (например, после очередного шага алогритма) из кода вывести гораздо сложнее, потому нужно по сути самому проделать в уме необходимые операции. Наконец, отдельные аспекты невозможно понять из кода иногда абсолютно никак. Например, назначение — «зачем этот код вообще нужен в этом месте, какой цели он служит».
Эти вещи и нужно документировать. А комментарии, повторяющие код, в духе
, конечно, не нужны.// Increment the counter. ++counter;Vladivo
15.09.2023 12:49Я высказался, наверное, чересчур категорично. Просто мы тут джейсоны гоняем, без всяких алгоритмов, и в 99% случаев можно написать понятно, не используя комментарии. Совершенно согласен с вами. Если смысл кода не очевиден (особенности чужой библиотеки, сложный алгоритм, редкий случай), то без комментария всё становится похожим на магию, что, на мой взгляд, гораздо хуже, чем избыточный комментарий.
Такой вопрос: а как вы относитесь к комментариям типа @todo?
SergeiMinaev
15.09.2023 12:49Да, блин, комментарии — это важная часть разработки, и нужно поддерживать и в актуальном состоянии
Согласитесь, было бы проще, если бы компилятор мог автоматически проверять актуальность комментариев и выдавать ошибку, если они устарели.
LeshaRB
15.09.2023 12:49+7Вы когда-нибудь видели методы по 100500 строк?
vassabi
15.09.2023 12:49+8я почему-то уверен что автор оригинального текста - не просто видел, а еще и с удовольствием сам пишет такие методы :)
iuabtw
15.09.2023 12:49+1Мой рекорд увиденного - около трёх тысяч строк на питоне; эдакий огромный switch-case от переданного аргумента
boldape
15.09.2023 12:49Я видел код мобильных игр на уже давно мертву(на телефонах) j2me. Вся довольно не маленькая игра типа Соника была ровно в 1 файле и ровно 2 функциях. Первая очень короткая функция вычитывала все события от пользователя и помешала их в главный цикл. Вторая собственно главный цикл, свитч на ~70 евентов, каждое плечо в среднем ~700 строк кода. И это была типичная структура для игр тех времён на Яве. Ну т.е. метод длинною ~50К строк я видел.
Моя задача была портирование игр, с Ява на с++ и/или с телефона одного размера экрана/вида кнопок на другой, называется пост продакшн.
mikronavt
15.09.2023 12:49+6Когда приходиться разбираться с логикой кода, отлаживать, вносить изменения, то в правом примере это сделать проще. Сначала выделяется общая логика метода, и нужно ли что-то в ней менять. Потом если с общей логикой все хорошо, то можно смотреть только на один метод и разбираться уже с ним.
Этот пример достаточно простой, поэтому левый код может читаться не хуже правого. Но если у нас будет добавляться какая-то новая логика, появляться больше опциональности, переиспользование шагов, добавится логирование, обработка исключительных ситуаций, потом появится необходимость в тестах, то все равно придется менять запись в сторону правого варианта иначе у нас станет лютое нечитаемое спагетти.
Kelbon
15.09.2023 12:49+2А чего удивляться, да, в гугле не умеют писать код. Это же не новость
Начиная с названия функции - createPizza, вообще-то createPizza это первая строчка с созданием (вызов конструктора), функция на самом деле обрабатывает заказ, а не создаёт пиццу
Далее про "деление на абстракции", чтобы на них делить они... должны быть и к тому же быть осмысленными. Разделение кода как справа на бездумные куски текста плодит множество занимающих место функций с громадным количеством прекондишнов, которые нигде не написаны. Ничего не остаётся, кроме как никогда не использовать такую функцию нигде(либо прочитать что там внутри, понять какие прекондишны у функции и написать их комментарием)
DirectoriX
15.09.2023 12:49+3В комментариях пишут, что код слева/справа легче читается, но как будто никто не прочитал код полностью И слева, и справа написано по 28 строк, но слева на "обёртки" (заголовок + фигурные скобки - границы блока) функции ушло ровно 2 строки, а справа - 11 (немногим меньше половины, Карл!), причём по меньшей мере 3 функции там объявлены, но не реализованы.
Я понимаю, почему некоторые считают правый вариант более удобным/понятным, но есть же граница, когда дробление кода приносит больше синтаксического мусора, чем пользы. Тем более, очень это странное дробление:
bake()создаётovenс параметрами по умолчанию, но настройка = разогрев вынесен в отдельную функцию? Вы уж или factory / builder используйте, или настраивайте в этом же месте, а то ни туда, ни сюда. И почемуpizzaинициализируется нужными значениями Base, Order и Cheese сразу, а не в 3 отдельных функциях?
zoonman
15.09.2023 12:49+5func makePizza(order *Order) *Pizza { return pack(bake(prepare(order))) }Надеюсь, что мозг сломал всем.
А если без шуток, то вариант слева прекрасен своей лаконичностью, но практически нерасширяем. Так джуны пишут в стартапах. А еще так пишут сеньоры код на выброс.
Вариант справа расширяем, для него можно писать тесты и т.д. Можно сменить тип духовки или добавить двухуровневый разгрев и т.д. Так обычно пишут опытные разработчики успевшие хлебнуть всякого и им просто хочется написать нормальный код и пойти домой. А когда надо что-то поправить, не пытаться вкурить 20 строк превратившиеся в 2000, а просто исправить одну функцию не нарушая интерфейсов.
А по большому счету оба фрагмента воняют индусятиной за версту. Странный выбор наименований и плохая семантика в целом.
mikronavt
15.09.2023 12:49В условной джаве и подобных языках было бы что-то типа
return order.prepare().bake().pack();и читается достаточно хорошо, правда пришлось бы нагородить кучу классов со своими методами и сильно увеличить количество строк кода.
zVadim
15.09.2023 12:49+1Если у пиццерии летом поставят столики и понадобится отдавать пиццу без упаковки, то левый код придётся копипастить почти весь, а в правом только добавить один метод из трех строк createPizzaWithoutBox()
qw1
15.09.2023 12:49Обычно это решается введением параметра "без коробки".
Тут тоже холиварный вопрос: что хуже — функция-монстр с 10 параметрами, или 250 простых функций, которые делают ровно то, что написано у них в заголовке. Второе проще читать, но если потребуется внести изменение в одном параметре, которое затронет сразу 60 вариантов...
KMiNT21
15.09.2023 12:49Оххх. Классный пример попинать ООП. :) Типичные проблемы
oven.Insert(pizza) или pizza.ПолезайВ(oven)И забавные абстракции
pizza.Boxed = box.PutIn(pizza) -- т.е. внутри пиццы теперь у нас будет лежать еще и упакованная пицца. :) А потом еще и .Boxed , и Sliced и .Ready... оййоой....; условный лиспоподобный псевдокод без синтаксиса отступов ; для базовой ситуации, где одна печь defn produce-pizza [order] ; включаем нагрев печки oven/start-heating our-pizza-cooking-temperature ; берем нашу стандартную Т ; "создаем" новую пиццу let [pizza (create-new-pizza order "Mozarella")] ; я указал как в примере ; ожидаем полного нагрева oven/wait-for-temperature our-pizza-cooking-temperature ; ....... ; а дальше конвеер операций c pizza (-> pizza (add-toping (get order :kind)) (oven/bake our-pizza-cooking-time) put-in-a-box (slice (get order :size)) close-the-box) ; Нам же важен порядок манипуляций? ; Значит вполне логично выразить это в последовательности операций ; Каждая следущая работает с результатом предыдущей ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ; ну и локальные функции для работы с pizza: defn add-toping [pizza kind] ... defn slice [pizza size] ... defn put-in-a-box [pizza] ... defn close-the-box [pizza] ...А если у нас печей много, то должно быть что-то вроде такого, т.е. операции в контексте печи с этим ID:
def cooking-params {:temp 221, :time 20} ...... defn produce-pizza [order] with-open [current-oven (oven/find-free-oven)] ; передаем команду "подготовить и начать нагрев до температуры X" (oven/prepare current-oven (get cooking-params :temp)) ; внутри with-open для того, чтобы уже пошел нагрев, пока мы начинаем делать пиццу :) let [pizza-template (create-new-pizza order "Mozarella")] (oven/wait-for-temperature (get cooking-params :temp)) (-> pizza (add-toping (get order :kind)) (oven/bake current-oven (get cooking-params :time)) put-in-a-box (slice (get order :number-of-slices)) close-the-box)В этом, на мой взгляд, такой код не сломает мозги непонятными метаниями между лишними абстракциями.
Или вот даже в такую сторону можно еще продвинуться:
(def cooking-params {:temp 221, :time 20}) ... ... ... ... ... (defn produce-pizza [order] ... with-open [current-oven (oven/find-and-program-oven cooking-params)] (-> order (create-new-pizza "Mozarella") (add-toping (get order :kind)) (oven/bake current-oven) (put-in-a-box) (slice (get order :number-of-slices)) (close-the-box))
firnind
15.09.2023 12:49На мой взгляд ни один вариант не подходит. Если речь про ФП, то скорее нужно что-то в духе pipe(prepare, bake, box)(pizza), если про ООП, то либо реально разносить по «энтерпрайзному» с блекджеком и фабриками, если логики реально много, либо просто применить паттерн builder, что будет достаточно читаемо.
Насчет проваливается в функции для понимания деталей реализации, например если реально важно, будет ли bake второй раз печь уже испеченную пиццу, то можно либо сделать метод с каким-то очевидным названием, который будет сбрасывать в 0 требуемое время выпечки — это даст нужную информацию, или же использовать более длинные названия методов, типа bakeIfIsNotBaked().
antonkrechetov
15.09.2023 12:49+2Всем, кто пишет комметарии в духе "но левый подход не работает с функциями из тысячи строк" — а что, если я вам скажу, что можно функции на тысячу строк декомпозировать, а относительно небольшие оставлять, как есть?
Люди очень любят придумывать или находить эвристические правила, вроде тех же 7-12 строк, а потом пытаться применять их везде, не задумываясь о том, подходят они к данному случаю или нет. Не надо так.vassabi
15.09.2023 12:49Люди очень любят придумывать или находить эвристические правила, вроде тех же 7-12 строк, а потом пытаться применять их везде, не задумываясь о том, подходят они к данному случаю или нет. Не надо так.
а как вы думаете - в чем цель некоторых статей?
чтобы в чем-то разобраться (есть ли преимущества у А или Б - и прийти к выводу что у каждого есть свои слабые и сильные стороны) или вызвать поток комментариев от "я за А" против "я за Б", чтобы они спорили между собой - у кого вера крепче?
buriy
15.09.2023 12:49Вот только если поменять порядок функций в правом варианте и чуть-чуть изменить использование переменных, код станет не сильно отличаться от левого варианта.
Тяжело скакать по функциям, когда они написаны в рандомном порядке, а расположите их одинаково, поставьте все управляющие конструкции внизу -- и проблем не будет.
(Спагетти код -- это плохо, но не потому, что кусочки слишком маленькие -- это вывод, к которому пришёл автор -- а потому что они перемешаны!)
А, соответственно, в левом коде есть обратная проблема: как только куски кода станут больше, ваша монолитная функция превратится в кашу, и, чтобы перескочить с одной части логики на другую, вам придётся читать весь код функции!
Apxuej
Автор, понимаю, что это перевод, но не могли бы Вы добавить голосование в конец статьи кому какая версия кода больше нравится. Было бы интересно посмотреть результаты.
PatientZero Автор
Спасибо за совет, добавил
Zara6502
а где вариант - никакой?
PatientZero Автор
Добавил. Конечно, перекос будет в сторону двух первых вариантов, но лучше, чем ничего.
FriLiHaGo
В голосование хорошо бы еще добавить вариант: левый с комментариями
Zara6502
Тогда уж "авторский", так как я не сразу понял о чем речь, воспринял это как левый вариант но я напишу в комментария почему именно.