Вот как бывает, пишешь программу, и вдруг видишь, что какой-то кусочек повторяется, и даже не второй раз. Начинаешь понимать, что это кандидат на новую функцию. Не потому, что логика обосабливаемая, а просто потому, что оно повторяется.
Этот кусочек нужно скопипастить, выбрать ему место, подровнять, дать имя этой новорожденной функции, и всем ее параметрам и переменным. Ведь теперь это самостоятельная функция, а не какой-то там временный тестовый код.
И вот получилась маленькая красивенькая функция, которая имеет всего одну функцию, один смысл, одну аккуратную страницу кода. Рекурсия блестит и сверкает своей стройностью и логичностью. В ней ничего лишнего, к ней ничего не нужно добавлять. Пока...
Но проходит день, другой, и начинаешь понимать, что она не так уж и совершенна, что в глубине этой функции нужно добавить еще одно условие. Добавить параметр вызова с лямбдой, в которой бы то условие проверялось бы, обрабатывалось, и т.д. В общем немножко добавить, что бы функция стала немножко более совершенной.
Ну не дублировать же целую страницу кода ради дополнительных двух строк кода? Если на каждое условие делать копии функций, это будут мегабайты однотипного кода.
И теперь в ней появляется дополнительное условие, добавляющее ей универсальности. Казалось бы теперь это само совершенство.
Но время идет, и аппетиты растут. Теперь программа хочет иметь новые свойства, новые условия, которые нужно где-то разместить, где-то поселить...
Ну в самом деле, не копировать же каждый раз функции, ради одного нового маленького-маленького условия? Ему всего то нужно места, две, а может и одна короткая строчка. Может даже всего один оператор умножающий на коэффициент какой-нибудь смысл или возводящий в степень.
И так прошло время, недели или даже месяцы.
Теперь это уже не та маленькая и красивая функция. Теперь это монстр, который может делать все и вся, потрясающий своей красотой в сложных хитросплетениях взаимодействий.
Она не так уж и велика, всего страниц на пять-десять кода. Но в ней все так взаимосвязано, все параметры и переменные так задействованы, каждое написанное слово имеет столько смыслов, тщательно продуманных... и благополучно забытых.
Вокруг, словно пояс астероидов, расположились маленькие функции вспомогательного функционала. Они жмутся к этому монстру, кроме него они никому не нужны, без него их сразу удалят.
Между строк уныло бледнеют старые комментарии, на которые уже давно не смотришь, их словно нет. При попытке их читать, слова кажутся бессвязными и не имеющими отношение к окружающему коду. Но они все равно остаются, в надежде пролить хоть какой-нибудь свет на происходящее.
Все это страшно менять, дабы ничего не рухнуло. Сложно переосмыслить, ведь каждое изменение может откликнуться во многих других местах. Сложно избавиться от всего этого, ведь от этого монстра теперь многое зависит.
И сложно дальше делать программу, этого монстра теперь никуда не подвинуть. Теперь, что бы что-то изменить, нужно убить этого монстра. Пока он не пожрал всю программу.
Убить, мясо разделить на кусочки, и накормить ими новые маленькие функции, новых подрастающих монстриков.
PS: Прототипом к описанию была функция, которая занимает ~3 страницы, с кучей вспомогательных функций, которые все мельче, но суммарно их много.
Комментарии (34)
pankraty
22.09.2021 06:21+1Хехе, у этого хоррора есть еще сиквел про маленький переиспользуемый пакет, в который вынесены несколько полезных функций, используемых в нескольких проектах. И теперь мало того что изменения логики могут аукнуться в каком-то далеком месте, и тесты этого не покажут; мало того что любые изменения, ломающие API, становятся дикой головной болью... Так еще у этого пакета образовалось несколько внешних зависимостей, и все проекты его использующие обязаны выравнивать версии своих зависимостей, попутно исправляя ломающие изменения, привнесенные их обновлением.
Sazonov
22.09.2021 06:49+2Мне это всё напомнило вот такой хороший сайт. Периодически делюсь этим с младшими коллегами: https://refactoring.guru/ru/smells/long-method
victor79 Автор
22.09.2021 06:58Очень полезное описание, спасибо. Вроде написанное там все и так понятно, но оно все хорошо описано и собрано в одном месте.
mvv-rus
23.09.2021 00:22+1На этом хорошем сайте все далеко не так однозначно.
Например, в истории по ссылке попытка избавиться от слишком длинного метода простым извлечением метода чревато тем, что просто расплодятся вспомогательные методы, которые не имеют смысла без основного и сильно связаны с ним и друг с другом. В результате «слишком длинный» (якобы) метод, который занимал целую страницу (но всего одну, которую можно окинуть одним взглядом, разползется на кучу, занимающую, из-за бойлерплейта, далеко не одну страницу, которую уже не окинуть одним взглядом. А вот сложность при этом никуда не денется.victor79 Автор
23.09.2021 00:38Я с этим очень согласен, но выход то какой? Делать функции на 12 страниц?
Нужно как-то все же обосабливать хоть какие-нибудь блоки, даже если это приводит к некоторому увеличению кода, и если обособления в них даже не видишь.
Просто потому, что когда читаешь код, его пытаешься располжить в свою быструю память, а она ограничена. Пыешься охватывать взглядом блоки, объемы которых могут быть схвачены за один взгляд. И т.д.
И вот нужно как-нибудь разбивать на подфункции, или как-либо по другому делать разметку, что бы было удобно читать такой код. Однозначных рецептов нет, но если не пытаться, то вообще не будет разбивки.mvv-rus
23.09.2021 01:08+2Я с этим очень согласен, но выход то какой?
Действовать по обстоятельствам. Использовать свой опыт и здравый рассудок. Общие принципы их заменить не могут, хотя и могут послужить подспорьем.
Собственно, вы и сами об этом в конце написали.
barabacka
22.09.2021 08:21Бывает следующая стадия. Когда эта функция начинет уменьшаться и даже иногда исчезат вовсе.
aragaer
22.09.2021 09:04+2Почему-то в рабочих проектах происходит именно так, да. А вот в собственных у меня уже несколько раз случалась следующая красота: Некоторая логика несколько раз используется в разных местах. По идее всю эту логику можно было бы инкапсулировать в отдельный класс. Причем простой и наивный класс. И вот в один день я сажусь этот класс писать. Это ему знать не нужно, это тоже. И вообще, вот у меня тесты, как я буду его использовать.. И вдруг он уже готов. 100 строк тестового кода, который полностью проверяет поведение класса. Сам класс от силы строчек 30. Прохожусь по основному коду и дублирую существующую логику с использованием нового класса и проверками, что результат одинаковый. Нахожу ошибки в старой логике, которые с новым классом оказались на поверхности. Выпиливаю старый код. 100 строк удалено, 30 новых вставлено.
Ndochp
22.09.2021 09:23+2Так вы описали стадию
Рекурсия блестит и сверкает своей стройностью и логичностью. В ней ничего лишнего, к ней ничего не нужно добавлять. Пока...
Просто собственные проекты изменяются реже. Расширяются, растут и дт так же, а вот вернуться к старому функционалу и сказать: "а теперь ты будешь работать иначе" случается реже, так как заказчик — вы. А когда заказчик это новый человек на старой должности и он хочет новые свистелки, но только в одном месте из трех, откуда выносили класс — вот тут и начинается хоррор из статьи.
AlexSkvortsov
22.09.2021 11:57По моему опыту уже только наличие тестов почти всегда предотвращает дальнейшие негативные этапы.
Да и вообще, я если пишу функцию длиной более 20 строк, это уже звоночек, что что-то не так.
Serg10
22.09.2021 20:14Объясните начинающему, как написать, например, маппер на овермного полей в 20 строк кода? Или вот ещё, работа с экселем, когда нужно вставить много полей. Наверняка есть ещё куча примеров, но я не припомню
AlexSkvortsov
23.09.2021 00:05Если у Вас есть структура на 20+ полей, то это уже само по себе обычно означает необходимость рефакторинга и совершенные ранее ошибки.
Если полей менее 20, но у маппинга сложная логика, то выносим всю логику в отдельные малые методы и вспомогательные мапперы.
Ну и наконец 20 строк - это звоночек, а не жесткое правило. Просто в 9 случаях из 10 метод на 20+ строк уже необходимо рефакторить.
Ndochp
25.09.2021 01:18Как наличие тестов предотвратит изменения в требованиях?
Вот был у вас какой-то вынесенный кусочек с проверкой, допустим неких данных. Ок — ок, не ок — выдать сообщение пользователю (одно и то же). И был он такой одинаковый в пяти местах.
Потом оказалось, что нужно в одном из мест сделать сообщение синеньким. Потом в другом — что на иврите. Потом, в третьем — выдать пользователю "ок" и тихонько сообщить в СБ.
Да в первом случае можно добавить цвет со значением по умолчанию в параметры. Потом встроить функцию перевода, третий случай наверное все-таки нужно менять на вызов нового блока, но 90% кода — проверка собственно — будет совпадать. Ясно, что проверка скорее всего вынесена в отдельную функцию, но все равно будет блок-обертка над куском в целом. и про эту обертку надо будет думать, или делать 5 (или 3) штуки, отличающиеся одной строкой, или копить условия в одной обертке.AlexSkvortsov
25.09.2021 13:43+2Тест в какой-то момент станет очень лсожно доработать и проще окажется задублировать метод. Тест в данном случае просто заставляет лишний раз подумать.
aragaer
26.09.2021 22:38Тесты никак не связаны с изменением требований. Собственно если меняются требования, то надо идти и менять тесты. А если тесты нормально не меняются, то, как уже было сказано, это повод задуматься, нужно ли все это пихать в ту же самую функцию.
v1000
22.09.2021 10:19+1а еще бывает забавно, когда в самом языке есть баг, который много раз отлавливали в реальных программах и научились обходить. но когда встал вопрос исправить этот баг в самом языке, оказалось, что теперь это может повлиять на тысячи работающих программ, у которых вдруг сломается логика, основанная на этом баге. причём не понятно где, когда и как. и этот баг оставляют в самом языке.
WASD1
22.09.2021 13:44+1Причём в первом же абзаце обозначена проблема. Недавно сознательно написал такой код:
res = sync_and_do_something(data)
if (is_err(res)) {
close_and_release(data);
return 1;
}
close_and_release(data);
return 0;
Потому, что совпадение низкоуровневого поведения при успехе и неуспехе - это так получилось. При любом техническом или функциональном изменении они разойдутся и появится описанная выше проблема, когда содержательно разный код "продолжить работу vs закрыть работу" описывается одной функцией со 100500 параметров.
ПС
К стати мне кажется, что функциональное программирование (при существующих его недостатках) частично решает описанную проблему, т.к. там принято в более явном виде разделять "основную логику" от "побочной, передаваемой как лямбды".
Мне сложно это описать более явно, но вот есть ощущение, что в ФП такой стиль \ привычки, что этой проблемы скорее не возникнет.
johnfound
22.09.2021 15:16+1res = sync_and_do_something(data) if (is_err(res)) { close_and_release(data); return 1; } close_and_release(data); return 0;
А почему не что-то вроде:
res = sync_and_do_something(data); close_and_release(data); return iserr(res);
mvv-rus
22.09.2021 23:53Ну, в ООП, при минимально грамотном его использовании, такой проблемы не вызникает:
там обычно есть конструкция try… finally, которая позволяет вызвать код очистки вне зависимости ни от чего. Или — ее фунциональный заменитель, типа автоматического вызова деструктора локальной переменной при ее выходе из области видимости в C++.
Но вот в старых, процедурных языках, типа C, оригнинального Паскаля (не Delphi) и т.п. это действительно проблема.
PS Я бы все-таки, если расхождение веток пока что не просматривается на горизонте, слил бы пока эти две ветки в одну — но с прицелом на разделение, как только они разойдутся (для этого и комментарий можно оставить).
Shatun
22.09.2021 13:47+1PS: Прототипом к описанию была функция, которая занимает ~3 страницы, с кучей вспомогательных функций, которые все мельче, но суммарно их много.
Когда я пришел в программирование в первые полгода мне надо было пофиксить один мелкий баг. Баг казался несложным, со стэктрейсом с nullpointer в определенной строке. Я не видел подвоха пока не открыл исходники-там вызывалась функция на 3000 строк с десятками входных параметров и обращений к базе с ифами! И ошибка была где-то в середине...
В общем поиска причины мне хватило чтобы усвоить на практике зачем нужны правила типа SOLID, KISS и как ненадо писать код.
Yaris
22.09.2021 20:313 страницы - это ещё не так много. Как насчёт функции, которая занимает 12 страниц? При этом она рекурсивно вызывает себя и напрямую, и через другие функции. Человек, который её написал, говорил, что должен "помедитировать" прежде чем что-то в этой функции поменять. Он уволился 2 года назад, а тот код до сих пор никто не трогает, потому что его даже протестировать толком нельзя.
victor79 Автор
22.09.2021 21:47Меким шрифтом? Ну что тут скажешь, оставили художника без присмотра. Зато теперь знаете, к чему это может привести.
AzatJ
22.09.2021 21:48Не потому, что логика обосабливаемая, а просто потому, что оно повторяется.
Вот она причина, повторяется она по чистому совпадению. Если нельзя разбить на логические утилитнные функции, я всегда против переиспользования
victor79 Автор
22.09.2021 21:49Не все ситуации однозначны. И в рассказе не без преукрашательства.
AzatJ
23.09.2021 10:26Ну понятно, в программировании вообще нет абсолютных правил, только гайдлайны.
Когда мы выносим в одну функцию код, который не связан логикой мы нарушаем Single Responsibility Principle: каждый класс должен иметь только одну причину для изменения. Все дальнейшие проблемы от этого
victor79 Автор
23.09.2021 13:44каждый класс должен иметь только одну причину для изменения
Смысл этого предложения несколько кривой, наверное ты подразумевал «иметь одну ответственность». И дальше следует категоричное «Все дальнейшие проблемы от этого».
Согласно этому принципу, если мне в функцию размером в страницу кода нужно добавить новое условие, то я должен сделать копию этого кода и менять в копию. Ведь это уже другой смысл — «А» это не «А+Б». Когда понадобиться восемь доп.условий, то это уже 256 потенциальных страниц копий кода. И потом еще на каждый вызов иметь switch/case то же на 256 позиций — это что бы не заморачиваться, которые варианты из этих 256 все же нужны для текущего случая.AzatJ
23.09.2021 15:06Смысл этого предложения несколько кривой, наверное ты подразумевал «иметь одну ответственность».
Uncle Bob настаивает именно на этом определении: https://blog.cleancoder.com/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html
Если функция выполняет «А», то причина изменения может быть, то что бизнес требование «А» поменялось. Если есть другая бизнес задача «Б» , которая по совпадению имеет такой же код как и «А», то, согласно принципу они должны лежать в разных местах, и если требование к «А» (но не «Б») поменялись, то можно без проблем поменять код в одном месте
https://blog.cleancoder.com/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html
mamento
Это не новелла, это хоррор! Спать 3 дня теперь не смогу!
avkor2021
Вот, а надо было написать, что этому чудовищу нужна красавица Рефакторинг, выделить функцию и прижавшиеся к ней функции вспомогательного функционала, которые никому больше не нужны, в отдельный класс, выделить интерфейсы, написать тесты, и была бы сказка, а не хоррор ;)