Комментарии 107
Наводите порядок в коде, с которым работаете.
Сомнительный совет.
Любая правка — это автоматически ненулевой шанс внести ошибку. Люди, знаете ли, не автоматы, и довольно часто ошибаются в простых казалось бы вещах.
А то, что затронутый код не нужен для решения вашей текущей задачи, автоматически означает, что и тестировать его вы вряд ли будете так же качественно, как основные правки. И вот тут — внимание, большой вопрос: стоит ли заниматься таким «улучшайзингом» в системе, если от нее зависит прибыль всей конторы (или жизни людей, или и то, и другое)?
Разумеется, заниматься «улучшайзингом» стоит с учетом покрытия тестами, зоны ответсвенности, и того, насколько код горячий, либо грешит неочевидными последствиями
Сам процесс рефакторинга очень помогает разобраться в деталях, ведь что бы все пересобрать «как надо» и очистить, нужно понять, как оно вообще работает.
Далее, пересобранный и очищенный код легче понять, следовательно в нем меньше багов, его легче поддерживать и вносить изменения.
стоит ли заниматься таким «улучшайзингом» в системе, если от нее зависит прибыль всей конторы (или жизни людей, или и то, и другое)?
Ну если такой код не покрыт тестами на 1000%, то это как с бэкапами, «люди делятся на 2 типа, на тех кто еще не делает бэкап...» и так далее.
основная причина багов, это то, что программист не совсем понимает код, вторая причина — невнимательность, вследствие усталости.Отсюда, имхо, можно сделать логичный вывод. Важнее понять код, чем рефакторить его. Не надо тратить время на необязательный рефакторинг, что бы не устать на момент когда дойдет дело до обязательной работы:)
Сам процесс рефакторинга очень помогает разобраться в деталях, ведь что бы все пересобрать «как надо» и очистить, нужно понять, как оно вообще работает.Тому кто рефакторит — безусловно помогает. Т.к. рефакторинг даже будучи сделанный по гайд-лайнам, все равно несет в себе индивидуальный отпечаток.
Помогает и новичкам, при условии что после рефакторинга код действительно стал именно лучше, а не просто стал другим — такое тоже бывает.
А вот старой команде, которой был привычен старый код — необходимость разбираться в новом, пусть и более качественном коде, не обязательно поможет.
Тут немного есть ассоциация с анекдотом, где мысль в том, что жена прибралась в гараже у мужа и теперь он ничего не может найти.
если такой код не покрыт тестами на 1000%Ни разу не видели код покрытый тестами хотя бы на 80%.
Как по нам с рефакторингом ситуация простая — «работает — не трогай»©
Просто «для искусства» можно рефакторить свои пет-проекты на досуге, благо это процесс бесконечный, как ремонт.
Ни разу не видели код покрытый тестами хотя бы на 80%.
В SQLite 100% branch coverage. Правда, на практике это не особо помогает.
Жизнь…
В этой ветке остро не хватает Дедфуда.
Я из тех кто раньше делал бакабы теперь в коробках дежат cd, dvd, mini-dv кассеты (а ведь их перематывать надо) hdd от 100мб до 1гб, теперь уже и не подключить часть из них ide.
Короче я перестал делать бакапы их невозможно хранить, т.е. носители отмирают слишком быстро.
Все что мне нужно или на гугл-фото или на гитхабе, остальное вероятно никому-никогда не понадобится.
я перестал делать бакапы их невозможно хранить, т.е. носители отмирают слишком быстро.Без обид, но имхо это не носители отмирают слишком быстро, а Вы просто забили на это дело.
Даже сейчас — mini-dv кассету — на автомате можно слить, поставил оно качается, в зависимости от камеры можно даже 4х скорость сделать. Да, менять руками придется. А h264/h265 ужмет некисло и быстро на современном железе.
cd/dvd до сих пор живы, даже ноуты с ними можно найти.
Для ide устройств продаются недорогие usb адаптеры, тоже не проблема.
остальное вероятно никому-никогда не понадобится.У нас была куча фоток сделанных еще хз когда, когда много ходили с фотиком и снимали все подряд. Думали уже — никто никогда все это не разберет и на фиг надо.
Ан нет.
Благодаря относительно недавнему появлению качественного софта по сортировке и объемным современным дискам — удалось загнать всё на пару хдд и большУю часть разложить по датам (данные с exif или имя файла там где оно не dsc000), людям (распознавание лиц), географии съемки (снятые объекты + привязка по времени соседних фоток) и заодно избавиться от некачественных дублей на автомате (там где снимаешь серию, но 9 из 10 со смазом).
Некоторые вещи которые уже и забыли что они где-то есть — неожиданно и приятно всплыли.
Но сомнительный он из-за возможных изменений в тех частях кода, которые не связаны с вашей текущей задачей. Это вносит большую неразбериху в систему контроля версий. Тот, кто после вас будет разбираться с тем, как вы решили задачу, увидит, например, что вы изменили 10 файлов вместо одного, в котором содержался баг. Как ему найти собственно исправление бага?
Тот, кто после вас будет разбираться с тем, как вы решили задачу, увидит, например, что вы изменили 10 файлов вместо одного, в котором содержался баг. Как ему найти собственно исправление бага?Два коммита? Рефакторинг и непосредственно исправление бага. С пометками о том что есть что разумеется.
Это был ритуал? Или какое-то обоснование такой практике озвучивали?
Обоснование очевидно любому, кто делает ревью таким PR.
У нас, например, есть четкое правило: заметил говнокод — заводи отдельный таск в джире с типом «рефакторинг» и делай все изменения в его ветке. Если позволяет время спринта, он вносится асапом и выполняется до таска с изменениями логики. Если нет — ну сорян, надо работать с тем, что есть, а рефакторить после (зависит от критичности таска, конечно).
У нас, например, есть четкое правило: заметил говнокод — заводи отдельный таск в джире с типом «рефакторинг» и делай все изменения в его ветке.
А если исправление маленькое? Завести задачу, завести ветку, а потом ещё раз описать изменение в коммите — звучит чересчур громоздко для, например, исправления опечатки в имени переменной. Не получается ли в результате, что на мелкие исправления просто забивают? Или "протаскивают" их в несвязанных по смыслу коммитах, за что потом шлются лучи добра?
Или, скажем, мы видим очепятку на сайте, вносим никому не сказав правки. В том числе не предупредив людей, отвечающих за контент сайта. В следующий раз они пришлют правки текстов снова с этой очепяткой и другой программист тупо заменит пофиксенные тексты на сайте на пришедшие с оставшейся в них опечаткой.
Общая идея, что с одной стороны бюрократия — зло, но с другой лично у меня заведение типовой задачи в джире + сделать ветку в гитхаб десктоп занимает ну секунд 30 времени. Зато потом всем проще.
Проблема в том, что вся эта библиотека, сверху донизу и снизу доверху — построена вокруг работы с этой структурой. Банальная правка привелабы к тому, что перестали бы собираться тысячи программ у десятков тысяч разработчиков…
В результате переименования
childs
в children
пришлось ждать несколько лет — до выхода версии libxml 2.Если коммитов в ветке два — на первый взгляд никакой, их действительно несложно посмотреть отдельно. Но первая проблема в том, что их почти никогда не два — дальше начинаются правки по замечаниям ревью и количество коммитов растёт, а вторая в том, что на ревью замечания пишутся на весь PR, т.е. на ветку, а не на коммит. Аппрув ставить и мержить так же имеет смысл раздельно рефакторинг и изменения функционала.
Тут зависит от того, как процесс построен. Если команда понимает и обсудила workflow, то проблем никаких. И да, я определенно согласен с автором комментария, что заводить отдельную(!) таску(!!) в джире(!!!) на рефакторинг, который может быть произведен «на лету» без последствий (и когда время, затраченное на, собственно, рефакторинг, много меньше времени, затраченного на создание тасок, лишней ветки, времени ревьюверов) — это какой-то оверкилл.
В то же время, если рефакторинг затрагивает внешнее API, или затрагивает сотню файлов проекта, или это не тупое переименование переменных а именно смысловое написание кода, то в этом случае, конечно, отдельная таска и планирование имеют место быть.
обычно амендятся в тот коммит
Это требует несколько большей аккуратности от разработчиков, чем в большинстве команд реально добиться. Я пришёл к тому, что разрешаю в своей команде коммиты в PR делать сколько и как им угодно, но при мерже этот PR squash-ится с корректным (по conventional commits) сообщением. Это даёт и чистую историю, и не перегружает команду формальными требованиями к процессу.
Кроме того, мы не практикуем никакое переписывание истории после git push, а ревью определённо начинается уже после git push.
Так после отправки кода на код ревью он уже не один работает. Кто-то посмотрит в вебморде пулл-реквест, а кто-то спуллит локально, чтобы в ИДЕ посмотреть нормально
git fetch && git reset --hard origin/myfeaturename
git pull проще
Это, конечно, ваше право — но если вы этим занимаетесь, то и разгребание последствий — тоже ваша задача.
Git — это инструмент для написания истории, не для хранения (для этого есть mercurial).
Если вы историю, в принципе, не пишите — нафига вам Git???
Почему я смотрю не тот код, который мне прислали на ревью? У меня нет привычки в ветках, отданных на ревью коммитить что-то своё. А изменения истории запрещены соглашениями.
Можно пруф, что гит — это для написания истории? Мне-то как раз mercurial больше нравится, но стандарт де-факто — git.
У меня нет привычки в ветках, отданных на ревью коммитить что-то своё.Если вы туда ничего своего не коммитите, то зачем вам Git Pull? Достаточно Git Fetch.
Можно пруф, что гит — это для написания истории?Ну почитайте оффициальные рекомендации.
Или загляните в LKML. Вот прямо сегодня: PATCH v2, PATCH v3, PATCH v5, PATCH v7… вот что это такое?
Это — изменение серии PATCH'ей с целью сделать так, чтобы каждый из них был понятен. Ядро всегда так разрабатывалось. Ещё с прошлого века. А Git, в общем-то, был создан именно чтобы поддержать разработку ядра. Про остальное же Линус чётко говорит: What I find interesting is how it took over so many other projects, too.
Они никогда этого не планировал и на это не рассчитывал…
Мне-то как раз mercurial больше нравится, но стандарт де-факто — git.Меня это вообще всегда удивляли. Потому что у них, во-многом, противоположные подходы.
У Mercurial подход «история проекта важна — и потому мы не позволим каким-то там девелоперам её тривиально менять». И набор инструментов которые позволяют (и не позволяют, что в данном случае важнее) что-то делать.
У Git подход «история проекта важна — и потому мы дадим разработчику инструменты для её написания».
Git rebase
, Git cherry-pick
, Git rerere
(вы вообще в курсе существования такой команды… поинтересуйтесь что она делает и подумайте о том — зачем она вообще может быть нужна) и прочие.Потому больше всего удивляют люди, которые выбирают Git, а потом начинают жаловаться на то, что кто-то им историю «попортил»! Не «попортил», а «написал»… и Git для этого, в общем-то, и предназначен!
Хотите неизменную историю и стабильных бранчей… зачем вам Git, возьмите Mercurial!
Вот в официальных рекомендациях первое правило: History that has been exposed to the world beyond your private system should usually not be changed
Ну и дальше: Merging is a common operation in the kernel development process;
И ещё раз, обычно git выбирают как индустриальный стандарт де-факто, а не сравнивая его с другими VCS.
Ну и дальше: Merging is a common operation in the kernel development process;А вот ещё дальше: While merges from downstream are common and unremarkable, merges from other trees tend to be a red flag when it comes time to push a branch upstream
Вот в официальных рекомендациях первое правило: History that has been exposed to the world beyond your private system should usually not be changedДа. Это полезно и разумно. Потому после
rebase
ваша история получает приписку «v2», «v3», «v7», «v10». Разумеется вливается в окончательную версию только «v7» или «v10». Всё остальные — остаются только в деревьях авторов, их породивших.У других (пользователей Gerrit, например) — считается, что это не так важно и приписка «v10» не делается. И да, история, конечно, не меняется — в том смысле что в том репозитории, которым заведует Gerrit-сервер всё остаётся.
Но вот уже в релиз — идёт только «причёсанная история». Ну вот как-то так и так.
И ещё раз, обычно git выбирают как индустриальный стандарт де-факто, а не сравнивая его с другими VCS.Но… почему? Я понимаю отказаться от какого-нибудь GNU arch, которым никто не пользуются и разработка заброшена, потому что это «не стандарт», но Mercurial не является уж чем-то сильно хитрым и редким.
.bashrc
alias pullmr=git fetch && git reset --hard `git rev-parse --abbrev-ref --symbolic-full-name @{u}`
Ну или через git aliases что-то подобное можно провернуть. И та-да, теперь еще проще — просто «pullmr» и все.
А просто pull, как уже выше сказали, имеет некоторые сайд эффекты, что рано или поздно приведет вас к неожиданному редактированию сообщения мерж-коммита (когда образовались конфликты). А если вы не работаете с мержами в принципе, то «проще» разработчикам будет «git pull --rebase», что уже не так «проще» напечатать как просто пулл.
Амендятся на давно запушенный коммит?
Амендятся в соответствующий коммит в фиче-ветке, ну да, который был запушен некоторое время назад, до того, как ревьювер написал комментарий.
Никто не призывает рефакторить вообще везде (см. совет про перебарщивание). Выносите рефакторинг отдельным PR, если там много изменений. А совет отличный.
А если совсем не заниматься «улучшайзингом», то система в скором времени скатится в «Big ball of mud» — для чего есть очевидные предпосылки — первоначальный дизайн никогда не удовлетворит все будущие требования.
Он предлагает улучшить код, например, в том же классе, в который вы вносите правки. А не переделывать всё подряд под себя, потому что не нравится.
Читайте комментарий полностью. Я нигде не утверждаю, что «улучшайзингом» надо заниматься сразу по всей кодовой базе.
А если совсем не заниматься «улучшайзингом», то система в скором времени скатится в «Big ball of mud»
Читайте комментарий полностью. Я нигде не утверждаю, что «улучшайзингом» вообще не надо заниматься.
TL;DR: Я утверждаю, что смешивать рабочий код с «улучшайзингом» соседнего кода — плохо.
Совет плох из-за того, что предлагает смешать два различных вида деятельности в один. Это вносит неразбериху в систему контроля версий: вместе с кодом задачи программист коммитит и не относящиеся к задаче правки. В результате найти концы («кто добавил эту логику?») становится затруднительно: git annotate покажет не автора, а «я только табы подправил».
Ну и про недостаточно тщательное тестирование правок в «не своем» коде я уже писал. «Собирается — и ладно, что там может сломаться?». Автотесты — это прекрасно, но далеко не везде они покрывают 100% кода (а в большинстве проектов, которые я видел, они вообще покрывают произвольно выбранные клочки кода — то, что автор посчитал «сложным кодом»).
Посему во многих проектах такая деятельность прямо запрещена гайдами. Для рефакторинга — отдельная таска. Обнаружил ошибочный код при решении своей задачи? Если он не входит в область видимости задачи — создавай отдельную таску. А мешать — не надо.
И тут, конечно, могут быть свои стайл гайды — вплоть до отдельной таски или отдельной ветки. Но всё это не нарушает совет — главное оставить код лучше, чем он был — хоть немного.
— Код встречается в 20 разных местах? Нет, в функцию выносить не надо, правь все 20 мест как есть.
— Надо добавить параметр в функцию? Дай ему значение по умолчанию, чтобы не править все вызовы, даже если оно там бессмысленно.
— Код зависит от глобальных переменных? Нет, не надо ничего переделывать, просто добавь еще одну. Тестов у нас все равно нет.
— Что-то больше не используется? Ну и что, пусть будет. В крайнем случае закомментируй.
И так далее и тому подобное, повторенное многократно.
В результате получается прекрасная, компактная история в контроле версий, где все по делу.
Правда, непосредственно код при этом почему-то превращается в спагетти, правка багов требует медитации и телепатических способностей, а от предложений новых фич случается нервный смех и дергается глаз.
— Надо добавить параметр в функцию? Дай ему значение по умолчанию, чтобы не править все вызовы, даже если оно там бессмысленно.
Понятно, что через несколько итераций имеет смысл пересмотреть и, быть может, переписать функцию, но конкретно сейчас «ага, вот у нас есть функция, нам надо добавить выбор формата, допустим. Добавим параметром, по умолчанию оставим текущее поведение» — я в упор не вижу проблемы, что не так? Если условный параметр на самом деле принадлежит этой функции, то почему бы и нет?
Добавим параметром, по умолчанию оставим текущее поведение» — я в упор не вижу проблемы, что не так?
Не так там последняя часть. А именно «даже если оно там бессмысленно».
Через 10 итераций у функции 10 параметров по умолчанию, половина из них используется только если, например, первый параметр равен false, другая половина — если первый параметр равен true, а третий — 42. Новые параметры, естественно, добавляются в конец (чтобы не менять существующий код), даже если они используются куда чаще существующих. В результате, чтобы указать этот последний параметр, надо указать и все остальные, при этом см. выше — понять по сигнатуре, что там происходит, уже невозможно. В коде тут и там появляются шедевры вида function(false, true, false, false, false, true, true, true, false). В один прекрасный день кто-то забывает явно указать последний параметр в паре мест, но все компилируется и даже вроде бы работает, а через пару лет вы тщетно пытаетесь понять, что давно уволившийся автор хотел этим сказать.
И как вы рекомендуете поступать в такой случае? После условно четвёртого параметра делать рефакторинг и принимать структуру functionParams? Разбивать на несколько функций, делающих что-то похожее, но по-другому?
Надо добавить параметр в функцию? Дай ему значение по умолчанию, чтобы не править все вызовы, даже если оно там бессмысленно.
Конечно дай значение по умолчанию. Нет, оно там не бессмысленно, иначе бы не добавляли.
«Улучшательство» ради «улучшательства» может привести к ошибкам во всей системе. И выставлению санкций «улучшателю» само собой
Какая-то странная аргументация. Любое изменение может привести к ошибкам. Так что теперь код вообще не трогать?
5 никто не использует. Код меняется быстрее и комментарии устаревают, вводят в заблуждение. Комментарии нужны только там, где код делает то что от него не ожидается.
5 никто не использует. Код меняется быстрее и комментарии устаревают, вводят в заблуждение. Комментарии нужны только там, где код делает то что от него не ожидается.
Вот не надо говорить за всех. Потому что во первых комментарии и документация это далеко не всегда одно и тоже.
Кроме того там стоит и следующее:
Дайте себе некоторое время на то, чтобы придумать описательные имена переменных. Это себя окупит. Выбор хороших имён для переменных, методов и классов требует времени. Но хорошие имена позволят сэкономить больше времени, чем нужно на их выдумывание.
И это на мой взгляд очень хороший совет.
3-ий говорит о том, что не надо писать то, что действительно не требуется прямо сейчас (заказчиком или требованиями), то есть не надо реализовывать что-то на будущее. Потому что в будущем могут появиться, вообще, прямо противоположные требования.
1-ый же говорит: «Вот делаешь изменения — почисти старый код, чтобы он, возможно, не конфликтовал с новыми реалиями, а, возможно, просто не очень аккуратный старый код или совсем не аккуратный старый код.»
Комментарии нужны только там, где код делает то что от него не ожидается.
Да, было у нас таких пара человек. Слава богу быстро выгнали.
Наводите порядок в коде, с которым работаете
В политике гугла (или рекомендациях), тоже есть это, мол новый код не должен быть идеальным, и если он уже приносит улучшения в существующий код, то это уже хорошо.
1. Наводите порядок в коде, с которым работаете
— Практика показала, что после парочки «мини изменений» git blame и прочие способы понять в какой момент была написана эта строча, а значит понять связь с таской, перестают работать.
4. Планируйте работу над кодом
— Самый ужасны из советов. Дело в том, что более менее сложные задачи достаточно тяжело распланировать заранее в голове. Как результат — ступор, когда ты не можешь написать первую строчку кода. Гораздо более надежное решение — итеративный инкрементальный подход, когда мы сначала реализуем задачу, а потом обязательно полируем-рефакторим. И да, я специально не говорю про TDD, дабы не разводить флейм.
5. Документируйте свои проекты
— А вот это, определенно, полезно. Только есть проблема — написать работающую безвредную документацию не менее сложно чем написать работающий безвредный код. Одной привычке недостаточно, необходимо умение и опыт
Окончательную полировку-рефакторинг, конечно, никто не отменял. В процессе реализации может накопиться много лишнего — всё-таки примерный план — это не UML с описанием всего возможного.
Гораздо более надежное решение — итеративный инкрементальный подход, когда мы сначала реализуем задачу, а потом обязательно полируем-рефакторим.
Ну вот вы уже составили минимальный план:
- реализуем задачу
- полируем-рефакторим
Разве это ужасно?
Ещё куда-то можно впихнуть "хотя бы минимально тестами покрыть"
- Сначала делаем, потом думаем и исправляем?
баг или таск — это проблема, если вы впали в ступор, значит проблему вы ещё не нашли
если вы не нашли проблему, то что править? вы за слепой поиск?
Давайте, придумайте мне хорошее называние для переменной «количество пользователей, зарегистрировавшихся за последний месяц, но еще не удаленных, исключая бета-пользователей, и имевших за это время такую-то(еще предложение текста) активность»Лехко! Спросите у ваших менеджеров — как они таких пользователей называют. Получится имя в духе ActiveUsers, RealUsers или, на худой конец Active30DayUsers.
Ну потому что не зря же вы в переменную вот это вот число засовывается — очевидно что оно как-то участвует в ваших бизнес-процессах. И в документации и переписке ну никто же не говорит про «количество пользоватей, которые бла-бла-бла… » — так обсуждать ничего невозможно… какой-то термин для вот этого числа используется же.
Если вы не можете придумать хорошее название для переменной — значит вы не понимаете ни зачем она вам нужна, ни что в ней, на самом деле хранится… продолжайте читать код и/или ТЗ дальше, до наступления просветления.
activeLastMonthUsers_toDelete
activeLastMonthBetaUsers
Можете qty добавить если хотите подчеркнуть что это именно количество
Можете змею с верблюдом не смешивать, если религия
А вообще вон выше подсказали, что наверняка для всей этой бабушки уже есть какое-то общепринятое в команде слово. Вот его и используйте.
В конечном счете, все советы, кроме, может быть, третьего, сводятся к тому, чтобы «быть хорошим мальчиком» и выполнять большую работу «за ту же зарплату».
Выполнение этих советов полезно непосредственно для вашего здоровья, если только вы не последний день работаете с этим проектом.
Айзек Азимов сказал: «День, когда вы перестанете учиться, это день, когда вы начнёте деградировать»полностью согласен, уже на себе чувствую этот эффект. Надо это исправлять, пока не начал смотреть еще и пусть говорят…
2. Заботьтесь о тех, кто будет работать с вашим кодом после вас
Как-то я слышал, что код нужно писать думая, что его будет поддерживать маньяк-убийца, который будет знать ваш адрес.
Я стараюсь писать, думая про миллениала, получавшего "автоматы" по информатике.
Сорри, никого не хотел обидеть если что.
Заботьтесь о тех, кто будет работать с вашим кодом после вас.
Пошёл налил себе кофе, погладил по голове, сказал какой я молодец и что у меня всё получится
Одной из самых полезных привычек, которых нет у программистов: просыпаться до 11.
Разные есть хорошие технологии для написания проекта с нуля. Но чаще надо что-то модифицировать.
Вот взять правило про планирование. Ну ведь каждый перед тем, как написать первую строку кода хоть чуть задумался, а что он кодит? Ну хоть на тридцать секунд? Ну а чем не планирование?!
и так далее, и так далее…
Вы думаете — это возможно распланировать? Нет, если у вас задача — попил бюджета, тогда да, легко… А вот если вы чего-то реально разрабатываете?
6 полезных привычек, которые, что удивительно, есть лишь у немногих программистов