
Был поздний вечер.
Мой коллега только что записал в репозиторий код, над которым работал целую неделю. Мы делали тогда графический редактор, а в свежем коде были реализованы возможности по изменению фигур, находящихся в рабочем поле. Фигуры, например — прямоугольники и овалы, можно было модифицировать, перемещая небольшие маркеры, расположенные на их краях.
Код работал.
Но в нём было много повторяющихся однотипных конструкций. Каждая фигура (вроде того же прямоугольника или овала) обладала различным набором маркеров. Перемещение этих маркеров в разных направлениях по-разному влияло на позицию и размер фигуры. А если пользователь, двигая маркеры, удерживал нажатой клавишу Shift, нам, кроме того, надо было сохранять пропорции фигуры при изменении её размера. В общем — в коде было много вычислений.
Код, о котором идёт речь, выглядел примерно так:
let Rectangle = {
resizeTopLeft(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
resizeTopRight(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
resizeBottomLeft(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
resizeBottomRight(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
};
let Oval = {
resizeLeft(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
resizeRight(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
resizeTop(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
resizeBottom(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
};
let Header = {
resizeLeft(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
resizeRight(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
}
let TextBlock = {
resizeTopLeft(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
resizeTopRight(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
resizeBottomLeft(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
resizeBottomRight(position, size, preserveAspect, dx, dy) {
// 10 однотипных строк вычислений
},
};
Меня все эти вычисления, все эти почти одинаковые строки, сильно зацепили.
Код не был чистым.
Основной объём однотипных строк наблюдался в тех местах, где задавалось перемещение фигур в одном и том же направлении. Например, в методе
Oval.resizeLeft()
был код, похожий на тот, который можно было найти в Header.resizeLeft()
. Дело было в том, что оба эти метода отвечают за изменения фигуры, выполняемые при перемещении маркера влево.Похожими были и методы фигур, имеющих одну и ту же форму. Например,
Oval.resizeLeft()
был похож на все остальные методы фигуры Oval
. Все эти методы работали с овалами — отсюда и их сходство. Дублирующийся код можно было найти в объектах Rectangle
, Header
и TextBlock
, так как текстовые блоки представляли собой прямоугольники.У меня появилась идея.
Можно избавиться от дублирующихся конструкций, по-другому сгруппировав код. Например — так:
let Directions = {
top(...) {
// 5 уникальных строк вычислений
},
left(...) {
// 5 уникальных строк вычислений
},
bottom(...) {
// 5 уникальных строк вычислений
},
right(...) {
// 5 уникальных строк вычислений
},
};
let Shapes = {
Oval(...) {
// 5 уникальных строк вычислений
},
Rectangle(...) {
// 5 уникальных строк вычислений
},
}
Затем можно прибегнуть к композиции и собрать из этих базовых методов то, что нужно:
let {top, bottom, left, right} = Directions;
function createHandle(directions) {
// 20 строк кода
}
let fourCorners = [
createHandle([top, left]),
createHandle([top, right]),
createHandle([bottom, left]),
createHandle([bottom, right]),
];
let fourSides = [
createHandle([top]),
createHandle([left]),
createHandle([right]),
createHandle([bottom]),
];
let twoSides = [
createHandle([left]),
createHandle([right]),
];
function createBox(shape, handles) {
// 20 строк кода
}
let Rectangle = createBox(Shapes.Rectangle, fourCorners);
let Oval = createBox(Shapes.Oval, fourSides);
let Header = createBox(Shapes.Rectangle, twoSides);
let TextBox = createBox(Shapes.Rectangle, fourCorners);
То, что у меня получилось, было в два раза меньше того, что написал коллега. В моём варианте программы полностью отсутствовали повторяющиеся фрагменты! Чистейший код. Если нужно было изменить поведение системы, относящееся к конкретному направлению, или к конкретной фигуре, можно было сделать это в одном месте, а не переписывать несколько методов.
Была уже поздняя ночь (я увлёкся). Я влил результаты рефакторинга в ветку
master
и отправился спать, гордый тем, как я «причесал» неопрятный код коллеги.Следующее утро
…прошло не так, как ожидалось.
Руководитель пригласил меня на разговор за закрытыми дверями, в ходе которого вежливо попросил меня откатить мои изменения. Меня это потрясло. Ведь старый код — это же сплошной бардак. А мой код был чистым.
Я нехотя исполнил просьбу, но мне понадобились годы для того, чтобы понять правоту руководителя и коллеги.
Это — одна из ступеней развития программиста
Зацикленность на «чистом коде» и стремление к удалению дубликатов — это одна из ступеней развития программиста, через которую проходили многие из нас. Когда мы не чувствуем уверенности в своём коде, возникает желание привязать наше восприятие самооценки и профессиональной гордости к чему-то такому, что можно измерить. Набор строгих правил линтинга, схема именования сущностей, структура файлов проекта, минимизация дублирующегося кода.
Нельзя автоматизировать борьбу с повторяющимся кодом, но эта борьба, совершенно точно, становится легче с практикой. Обычно после каждого изменения можно сказать о том, больше или меньше одинаковых конструкций стало в проекте. В результате избавление от дублирующихся фрагментов программы воспринимается как улучшение некоего объективного показателя качества кода. И, что хуже, это искажает самосознание программиста: «Я из тех, кто пишет чистый код». Это так же сильно влияет на человека, как и любой другой род самообмана.
Как только программист узнаёт о том, как создавать абстракции, он вполне может этим увлечься. Он, видя дублирующийся код, будет находить абстракции там, где их нет. А после нескольких лет такой практики дублирующийся код будет обнаруживаться абсолютно везде. Абстрагирование станет новым талантом программиста. Если кто-то скажет ему, что абстрагирование — это добродетель, он это примет. И он начнёт осуждать тех, кто не преклоняется перед «чистотой».
Теперь я понимаю, что у моего «рефакторинга», и у того, как я к нему подошёл, было два серьёзных недостатка:
- Во-первых, я не поговорил с тем, кто написал код. Я переписал код и залил его в репозиторий, не обсудив изменения с автором кода. Даже если мои изменения улучшали бы программу (я больше так не считаю), это — демонстрация совершенно неправильного подхода к подобным делам. В основе здоровой команды программистов лежит постоянное укрепление доверия. А если переписать код одного из членов команды и с ним не посоветоваться — это нанесёт удар по возможности эффективной совместной работы над проектом.
- Во-вторых, за всё надо платить. Мой код пожертвовал возможностью менять требования в угоду сокращения объёма дублирования. Цена этой жертвы была слишком высока. Например, позже нам понадобилось обрабатывать множество особых условий и вариантов поведения для различных маркеров разных фигур. Мою абстракцию для поддержки подобных требований пришлось бы основательно усложнить. А вот в исходную «неаккуратную» версию кода подобные изменения вносились легче лёгкого.
Говорю ли я о том, что вам нужно писать «грязный» код? Нет. Я предлагаю лишь хорошо подумать над тем, что имеют в виду под понятиями «чистый» и «грязный». Что-то приводит вас в негодование? Вы чувствуете в чём-то правильность, красоту, изящество? Уверены ли вы в том, что можете применять подобные понятия, описывая конкретные результаты работы программистов? Каким образом эти понятия влияют на то, как пишут и модифицируют код?
Я, конечно, о таких вещах тогда не думал. Я много размышлял о том, как выглядел код — но не о том, как он развивался вместе с командой, состоящей из людей, которым не чуждо ничто человеческое.
Программирование — это путь. Подумайте о том, как далеко вы продвинулись от написанной вами первой строчки кода, до того места, где находитесь сейчас. Я полагаю, что вы, в первый раз извлекая функцию или подвергая класс рефакторингу, с восхищением наблюдали за тем, как эти приёмы упрощают сложный код. Если вы чувствуете гордость за своё дело, то у вас появится стремление к преследованию чистоты кода. Поддайтесь на некоторое время этому стремлению.
Но не останавливайтесь на этом. Не становитесь фанатичным приверженцем чистого кода. Чистый код — это не цель. Это — попытка как-то осмыслить огромную сложность систем, с которыми мы имеем дело. Это защитный механизм, который программист применяет тогда, когда ещё не уверен в том, как некое изменение повлияет на кодовую базу проекта, но ищет ориентиры в море неизвестности.
Пусть идея чистого кода станет вашим ориентиром. А потом — отпустите эту идею.
Уважаемые читатели! Как вы относитесь к «чистому коду»?

DMGarikk
Мне кажется или реальноая проблема статьи не в чистом/грязном коде, а в том что ктото пушит в мастер без mr/pr и кодревью?
aleksandy
Нет, не кажется. В обсуждениях оригинальной статьи в твитере к это тоже был первый комментарий.
dididididi
Пуш с ревью — как секс в презервативе)) ощущения не те!
konraddd
Работа она ради денег, а секс ради денег — лучше в презервативе делать)) Я не в теме, но таковы мои предположения
dididididi
А вы докопались до истины))) Те кто прогают для удовольствия, те просто пушат в мастер, а кто для денег — для тех кодревью, аджайлы и прочая))
Zenitchik
Ну да. В пэт-проекте у меня только мастер.
А на работе — тыща веток, только в одну из которых (по имени develop) можно пушить.
Dink
Причем пушат похоже все: и автор оригинальной статьи, и его коллега (иначе чистоту кода можно было обсудить еще на ревью первоначальных изменений)
nckma
А как не пушить в мастер, если менеджер говорит, через неделю сделать бету, хотя проекту только неделя и прошла?
В былые времена я бы мог ожесточенно спорить с менеджером и говорить, что это тупо и не возможно. Сейчас я говорю «это будет не просто сделать, но мы сделаем все, что будет в наших силах». Вроде бы и не соврал и позицию обозначил.
DMGarikk
если вы один программист на проекте то можно и в мастер конечно, в таких условиях
во всех остальных случаях деплой когда в мастере непонятно что и в каком статусе — это хождение по минному полю
wiwrel
Похоже что так и есть. Ибо первая проблема — пуш в мастер без согласования с автором, вторая — модификация кода без code review. Этих проблем можно было бы избежать — обсуди автор заранее изменения в коде
shuron
Чисто интересно "пуш в мастер без согласования с автором"
это единстевенный подход и парадигма? Trunk-based никак?
MIKEk8
+ пуш на прод в ночь. Ещё бы в пятницу или перед праздниками и было бы кккомбо.
PsyHaSTe
ИСЧХ судя по статистике голосования за статью почти 80% прочитавших согласны с приведенными "фактами".
Я понимаю, что человек старался и переводил, но кмк текст "не стоит чернил которыми написан".
TheShock
Много людей ищут оправдание своему говнокоду и стараются его возвести в идеологию.
v2kxyz
Проблема в том, что есть ненулевое количество разработчиков, которые подвержены карго-культу. Т.е. вместо того, чтобы уловить причинно-следственные связи в статье, они уловят факт — не нужно плодить «лишние» абстракции, надо использовать Ctrl+C/Ctrl+v N раз.
P.S. Нужно признать, что понимать — когда абстракция лишняя, а когда крайне полезная нужная — очень сложно.
it_pm
Лепят не только по причине карго-культа.
Профессионально пишущие на ФП часто жалуются на то, что приходится разбирать код в который кто-то из предшественников что-то налепил просто чтобы потренироваться в ФП.
Flammar
Вообще, профессионально пишущим нередко приходится разбирать код, в который кто-то из предшественников что-то налепил просто чтобы потренироваться. Издержки профессии…
VolCh
Потренироваться ещё ладно. Есть вероятность, что перед тренировкой доки почитал и прочая теория есть. А вот когда человеку вне контекста дают задачу на проекте и он делает "работает же", то разгребать гораздо больше обычно
DarthVictor
Я не плюсовал статью, но скорее по причине бесполезности перевода статей Даниила. Он и сам их вполне успешно переводит.
Но не соглашаться со статьей тоже трудно.
Ну да, не прав. Странно, что у них не было код-ревью, но судя по тому что Redux-у уже пять лет, самой истории легко может быть и все десять. У меня на работе в это время VSS использовался вместо системы контроля версий. А ревью было добровольное и производилось путем подзывания другого разработчика за свой стол.
То же вроде верно.
Можно еще пошутить, подход Redux'а это прямо идеал кода «ДО».
PsyHaSTe
Да не верно. Вы пишете одну функцию, которую используете из разных мест. Если вдруг вам приходят требования которые требуют засунуть 15 коллбек-аргументов чтобы одна эта функция выполняла нужные действия вот тогда и надо брать и разделять функцию. А если не приходят — то не надо.
В общем, как и с преждевременной оптимизацией, история что пытаются предвидеть то, чего не произойдет, и в итоге только ухудшают поддержку. Хорошая статья на тему. Приведу цитату:
DarthVictor
Вы как-то исходите из того, что юный Абрамов реально код укоротил и упростил.
Я вот рассказ понял наоборот, что он ввел в код новую абстракцию, причем выдуманную на ходу, не имеющую какого-то математического смысла. Для реального упрощения там можно было пытаться найти какой-то фитзический смысл этих операция, вроде предложенного TheShock'ом здесь.
Вместо этого у него вышла просто фабрика кусков функций и странных подобранных констант вроде fourCorners и twoSides, который не пойми как вычисляются.
Код в духе Хаскельного матчига по печкам/фигурам там был по-ходу до этого. А Абрамов начал его превращать в абстрактную фабрику
шаблонов бобов-одиночекгеометрических фигур. «Сначала берем рецепт сторон, потом углов, потом понимаем, что у заголовка сторон, не четыре, а две (sic!) и все пишем заново.» И из-за большого числа геометрических объектов стало формально короче. На деле нечитаемое говно. Человек-архиватор.У меня есть еще вариант, что математический смысл в его действиях всё-таки был, но его никто не понял. Тогда опять же стоило сначала спросить у того, кто писал в начале.
vsb
Интересно, какой процент программного кода проходит review перед публикацией? Я вот ни разу не работал в подобной компании и при взаимодействии с другими компаниями тоже ни разу не видел подобной практики (как и повсеместного использования юнит-тестирования, функционального тестирования и прочих "священных коров"). Это мне так повезло или в интернете некий карго-культ качества, который в реальности встретить надо ещё постараться?
PsyHaSTe
Только когда был зеленым джуном работал в компании где не было ревью. После этого сменил 5 мест, и всегда любые изменения просматриваются другим разработчиком. В одной компании таже была заморочка что был написан свой анализатор с помощью АПИ компиляторы чтобы некоторые гайдлайны компании (вроде запрета использования из модуля для клиента А функций для клиента Б) энфорсились и их нарушение приводило к ошибке компиляции.
Kanut
Всеобщей статистики у меня нет, но code review в том или ином виде последние лет десять по моему на всех фирмах был. Под "в том или ином виде" я имею ввиду что иногда он был обязательным и без него было вообще не закоммитить, а иногда ты сам мог решать надо кому-то дать код ещё раз глянуть или нет.
Юнит-тесты тоже были по моему почти везде, но в большинстве фирм это было скорее для алиби и кто хотел делал, а кто не хотел не делал. В одной единственной фирме с этим было очень серьёзно.
0xd34df00d
Работал в одной Большой компании, которой лет сорок, у которой есть довольно много клинетов, и которая славится тем, что не падает. В общем, в ней внезапно решили, что теперь код-ревью становятся обязательными. И был у меня среди коллег один товарищ с ну очень интересным ЧСВ. В общем, он создавал бранч, пушил туда, создавал PR, назначал себя ревьювером, сам писал LGTM, сам мержил,
сам хлопал себя по плечу. И так несколько лет. Может, до сих пор так делает, не знаю.Впрочем, это не лучшее его качество было.
Koderka
А я, наоборот, не работала на проектах, где бы ни было code review в том или другом виде. А сейчас при всем желании в битбакете не могу смержить pull request с продовым бранчем без одобрения кого-то из коллег.
Dolios
У нас в компании запрещено мержить бранч в мастер без ревью.
DMGarikk
это кажется карго культом до тех пор пока вы не пушните в мастер и задеплоите фикс на две строчки который завалят очень ответственный сервис с потерей данных и простоем на пару суток… потому что документации тоже нет (а зачем?) а программист который писал большую часть модулей уволился лет 7 назад и у нес с собой многие ценные знания как оно работает.
после этого начнёт появятся понимание что качество — оно нужно и строится оно на очень многих параметрах. Это как в каске по стройке ходить, если взять статистику падения кирпичей на головку — каска с вашей точки зрения это каргокульт
StepEv
Это вам повезло. У нас невозможно смерджить pull request без отметки ревьювера. И знаю множество компаний с таким же подходом.
Whuthering
За последние лет 10 не встречал приличной компании, где не было бы обязательного code review хоть в каком-то виде (чаще всего настроено полиси pre-commit, иногда вручную). Тесты писали примерно в половине случаев.
Места, где нет ни того, ни другого, иногда на этапе собеседований или взаимодействия со смежниками тоже попадались, но в тех командах в целом с инженерной культурой было очень и очень плохо, и это были либо проекты которые писали вчерашние студенты, либо наоборот деды с полусоветских предприятий.
DarthVictor
Просто переход на обязательные ревью я стал встречать лет пять назад. Нормальные (когда на ревью 1000 строк ревьюер убивает день, проверяет решение целиком, а не тупо работает линтером, проводит минимальное тестирование) я и сейчас не везде вижу. Ну и нормальные ревью, а не «10 lines of code = 10 issues, 500 lines of code = looks fine» это не дёшево.
Firsto
За пять лет был только один такой проект — который полностью писали с нуля вдвоём с коллегой. Когда команда больше — куда ж без ревью.