Здравствуйте, Буравчик, Вы писали:
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>Какими должны быть правильные комментарии к PR/MR/т.п.?
По делу это те, которые (1) содержат информацию, что надо изменить (2) объясняют, почему это надо изменить и (3) говорят, как это изменить.
Здравствуйте, Буравчик, Вы писали:
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>Какими должны быть правильные комментарии к PR/MR/т.п.?
Б>(Навеяно темой в "коллеги улыбнитесь")
Вообще считаю код-ревью плохой практикой. Комментарии надо собирать, когда код ещё пишется, а не когда код уже написан. Например, тимлид может несколько часов в неделю посидеть с каждым. Или люди могут периодически работать вместе, показывать друг другу свой код. А когда код написан, и его надо переписывать, это приводит к прохождению через все стадии гнева/отрицания/торга/принятия, а это отнимает силы и мотивацию. Жаль, что мало команд умеет обходиться без этого всего.
Здравствуйте, Буравчик, Вы писали:
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>Какими должны быть правильные комментарии к PR/MR/т.п.?
Б>(Навеяно темой в "коллеги улыбнитесь")
Кодревью нужен в первую очередь для того, чтобы не пропустить код, где
* явные ошибки — чаще всего по невнимательности или по причине температуры/недосыпа у автора
* непонятного/запутанного кода — в голове у автора какая-то картинка имеется, и он из-за этого под действием "проклятия знания" (остальным непонятно/сложно)
Ещё кодревью нужен, чтобы кто-то другой был тоже в курсе, что задача решалась/решена. Иногда на ревью вылезает, что некоторую подзадачу вообще не надо было решать — для неё уже существовало решение.
----------------
Вот от этого и пляшем. Всё остальное от лукавого, и даже более того — демотивирует и разваливает коллективы.
Всё сказанное выше — личное мнение, если не указано обратное.
Здравствуйте, Буравчик, Вы писали: Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>Какими должны быть правильные комментарии к PR/MR/т.п.?
Именно из-за прохождений код-ревью, то есть молчаливого принятия прилюдного унижения человеческого достоинства, женщины считают программистов недомужиками. Разве альфа-самец потерпит какие-то возражения на код-ревью? Успешно проходишь код-ревью? — Рога уже ждут тебя!
Здравствуйте, Буравчик, Вы писали:
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>Какими должны быть правильные комментарии к PR/MR/т.п.?
1. Баги. Это самое главное, конечно, тут обсуждать нечего.
2. Очевидно неверная архитектура кода. К примеру в используемых фреймворках есть "правильный" подход к решению этой проблемы, а автор по незнанию наворотил велосипедов.
3. Несоответствие принятому стилю кодирования. Это пункт спорный, я считаю, что подобные проблемы должны проверяться автоматическими инструментами, а также форматирование должно применяться автоматическими инструментами, т.е. при настроенном редакторе этой проблемы не должно возникать вообще. Но если с этой автоматикой не получилось подружиться, то так. Повторюсь, что пункт спорный и мелкие несоответствия я бы не отмечал, т.к. пользы от этого не так много, если в целом код нормальный.
Что, я считаю, не должно комментироваться никак: Несоответствие написанного кода идеалам ревьюера. К примеру в Java можно обработку коллекции сделать через for в императивном виде, а можно через streams в функциональном виде. Вот тут в 99% случаев получается чистая вкусовщина, оба варианта читабельные, оба варианта достаточно производительные, причём часто люди предпочитают один из них в своём коде. И тут может разгореться сражение тупоконечников с остроконечниками. Вот такую ситуацию допускать нельзя. Либо заранее принять стандарт кодирования через streams, к примеру, а лучше всего просто не заострять внимание на этом вопросе. Написано правильно, багов нет, и ладно.
В общем самое главное это чтобы код делал полезную работу без багов.
Менее главное это чтобы код соотвествовал нефункциональным, но принятым и задокументированным требованиям в данном проекте.
И совсем не важно, чтобы код соответствовал нефункциональным и не задокументированным требованиям.
Здравствуйте, SkyDance, Вы писали:
Pzz>>По делу это те, которые (1) содержат информацию, что надо изменить (2) объясняют, почему это надо изменить и (3) говорят, как это изменить.
SD>Это идеальный коммент, настолько, что даже может быть code suggestion. Но это же обычно тривиальный коммент, практически линтер. В реальном же мире зачастую должно хватать коммента типа ай-ай-ай. Например, "где в этом коммите автотесты", "эта функциональность уже реализована в ХХХ", "мы же договорились — без костылей".
В реальном мире получаешь 10 комментов про то, что хвостик у запятой не той формы, и редко-редко получаешь коммент по делу.
Здравствуйте, Буравчик, Вы писали:
Б>Так обычно и бывает: Б>- либо команда уже понимает принципы реализации тех и иных задач Б>- либо тимлид рассказывает свое видение реализации Б>- либо исполнитель (случае сомнений) согласовывает его с тимлидом и/или другими членами команды
Это всё не то. Надо не "вИдение" обсуждать, а периодически вместе код писать. Тогда его можно не ревьюить. Зачем ревьюить код, если его уже видело как минимум два разных человека?
Здравствуйте, Буравчик, Вы писали:
A>>Это всё не то. Надо не "вИдение" обсуждать, а периодически вместе код писать. Тогда его можно не ревьюить. Зачем ревьюить код, если его уже видело как минимум два разных человека?
Б>Вдвоем код писать — дорого. В отличие от ревью
Зато и результат лучше, и в итоге себя оправдывает.
Но да, я не совсем удачно выразился. Надо не просто чтобы команда умела в подобные коммуникации, но и контора не была почасовой потогонкой.
Здравствуйте, sergii.p, Вы писали: SP>На практике получается вполне недурно. Ну реально блин, 95% времени ты отбиваешься от вкусовщины ревьювера. Да, остальные 5% — дельные замечания, но стоит ли оно потраченного времени? Реальные баги всё равно эффективнее выловит система тестирования, чем разработчик просто пробежав глазами.
Ревью призвано выявлять не баги, а говнокод. Цель — минимизация накопления техдолга.
Потому что без ревью есть искушение сделать всё тяп-ляп — потом разберёмся. Или тупо по незнанию залудить что-нибудь лишнее, вроде дубликата функции, которая уже есть в проектной библиотеке. Тесты проходят — алга в продакшн.
А вот потом, через полгодика, кто-то смотрит на этот код (например, в процессе добавления новой фичи), и видит, что всё как-то кривовато сделано.
Но времени переписывать нормально нет — это ж надо погрузиться в код; вспомнить что там, как, и зачем; аккуратно переписать; перетестировать. Даже если это делает автор того самого кода. Ещё веселее, если это чужой код — поди тут отличи "чувак не знал, что это поле можно не обнулять" от "чувак знал, что это поле нельзя не обнулять".
Вот уже и день прошёл — а собственно то, ради чего в этот код зашёл, ещё даже не началось.
И никто не стоит над душой, не давит "увидел бардак — прибери". Так что велики шансы не рефакторить, а костылить. И так постепенно код превращается в неподдерживаемый mess, и возникают вопросы вроде "как убедить менеджмент выделить время на рефакторинг говнокода и прочий техдолг".
Перефразируя известный мем, две недели рефакторинга могут сэкономить вам целых четыре часа код-ревью.
Уйдемте отсюда, Румата! У вас слишком богатые погреба.
Здравствуйте, Буравчик, Вы писали:
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>Какими должны быть правильные комментарии к PR/MR/т.п.?
Б>(Навеяно темой в "коллеги улыбнитесь")
Pzz>По делу это те, которые (1) содержат информацию, что надо изменить (2) объясняют, почему это надо изменить и (3) говорят, как это изменить.
Это идеальный коммент, настолько, что даже может быть code suggestion. Но это же обычно тривиальный коммент, практически линтер. В реальном же мире зачастую должно хватать коммента типа ай-ай-ай. Например, "где в этом коммите автотесты", "эта функциональность уже реализована в ХХХ", "мы же договорились — без костылей".
Здравствуйте, Sharov, Вы писали:
Pzz>>По делу это те, которые (1) содержат информацию, что надо изменить (2) объясняют, почему это надо изменить и (3) говорят, как это изменить.
S>(1) и (2) уже достаточно, с (3) это прям совсем хорошо.
(3) отфильтровывает комментарии из серии, что сделать было бы хорошо, но никто не знает, как.
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет?
Цель "дела" — довольный расплатившийся заказчик. Соответственно, что ведёт к получению денег от заказчика, а что к удовлетворению ЧСВ одного ресурса за счёт безсмысленной работы другого...
Данное сообщение является художественным произведением и освещает вымышленные события в вымышленном мире. Все совпадения с реальностью являются случайными. Не является инвестиционной рекомендацией.
Здравствуйте, Pzz, Вы писали:
Б>>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>>Какими должны быть правильные комментарии к PR/MR/т.п.? Pzz>По делу это те, которые (1) содержат информацию, что надо изменить (2) объясняют, почему это надо изменить и (3) говорят, как это изменить.
(1) и (2) уже достаточно, с (3) это прям совсем хорошо.
Здравствуйте, Буравчик, Вы писали:
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>Какими должны быть правильные комментарии к PR/MR/т.п.?
Б>(Навеяно темой в "коллеги улыбнитесь")
Смотря какой code review. У нас все смотрит один человек и я если честно стараюсь поменьше с ним спорить, что бы быстрее его пройти. Ну только если в случае явных его косяков делаю возражения.
Pzz>(3) отфильтровывает комментарии из серии, что сделать было бы хорошо, но никто не знает, как.
Потому что это задача автора кода. Ревьювер знает специфику области ревью, и если автор кода ее не знает, то самое время узнать. Можно, конечно, заявить, что ревьюер еще должен быть и учителем. Но мы же ше в школе и не в детском саду. Взрослые люди должны уметь сами разобраться. Разумеется, если только между ревьюером и автором нет установленных отношений типа ментор-падаван.
Что до комментов "запятая не там", это не комменты, это линтеры, их замечания должны быть исправлены автоматически. Код-форматтеры и прочие инструменты уже давно придуманы.
Здравствуйте, Pzz, Вы писали:
Pzz>Здравствуйте, SkyDance, Вы писали:
Pzz>>>По делу это те, которые (1) содержат информацию, что надо изменить (2) объясняют, почему это надо изменить и (3) говорят, как это изменить.
SD>>Это идеальный коммент, настолько, что даже может быть code suggestion. Но это же обычно тривиальный коммент, практически линтер. В реальном же мире зачастую должно хватать коммента типа ай-ай-ай. Например, "где в этом коммите автотесты", "эта функциональность уже реализована в ХХХ", "мы же договорились — без костылей".
Pzz>В реальном мире получаешь 10 комментов про то, что хвостик у запятой не той формы, и редко-редко получаешь коммент по делу.
может уровень ревьюера ниже просто и по делу сказать нечего?
Здравствуйте, Alekzander, Вы писали:
A>Вообще считаю код-ревью плохой практикой. Комментарии надо собирать, когда код ещё пишется, а не когда код уже написан. Например, тимлид может несколько часов в неделю посидеть с каждым. Или люди могут периодически работать вместе, показывать друг другу свой код.
Так обычно и бывает:
— либо команда уже понимает принципы реализации тех и иных задач
— либо тимлид рассказывает свое видение реализации
— либо исполнитель (случае сомнений) согласовывает его с тимлидом и/или другими членами команды
Да и ревьювить надо даже, если предварительно обо всем договорились.
A>А когда код написан, и его надо переписывать, это приводит к прохождению через все стадии гнева/отрицания/торга/принятия, а это отнимает силы и мотивацию.
Код во время ревью не надо полностью переписывать, а надо улучшать.
Т.е. общий подход к решение задачи будет верный (см. выше), но нужно изменить некоторые детали. Поэтому редко нужен "гнев/отрицание/торг/принятие".
A>Жаль, что мало команд умеет обходиться без этого всего.
Здравствуйте, Alekzander, Вы писали:
A>Это всё не то. Надо не "вИдение" обсуждать, а периодически вместе код писать. Тогда его можно не ревьюить. Зачем ревьюить код, если его уже видело как минимум два разных человека?
Здравствуйте, Pzz, Вы писали:
Pzz>В реальном мире получаешь 10 комментов про то, что хвостик у запятой не той формы, и редко-редко получаешь коммент по делу.
Это называется имитация деятельности, и происходит потому что ревьюеры не понимают целей ревью.
Видел такое, да, печалька.
Всё сказанное выше — личное мнение, если не указано обратное.
Здравствуйте, Буравчик, Вы писали:
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>Какими должны быть правильные комментарии к PR/MR/т.п.?
Б>(Навеяно темой в "коллеги улыбнитесь")
Для начала стоит уяснить чего не должно быть в код ревью.
Все вопросы стилистики и всего что может выдать линтер.
Если у нас код ревью состоит из улучшений где ставить пробелы, нужно это перенести в автоматические комментарии.
А так должны быть по логике кода и дизайна, почему здесь недостаточно хорошо, и как стоит сделать лучше.
Цель ревью должна быть, не просто указать на недочёты, а обучить и больше не возвращаться к таким ошибкам.
Часто встречал недовольство предложением, а как правильно написать, конечно, никто не удосуживается.
В итоге и сам не понимаешь, что не нравится, и не знаешь как сделать "правильно".
Здравствуйте, Буравчик, Вы писали:
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>Какими должны быть правильные комментарии к PR/MR/т.п.?
— грубые ошибки "в таком виде функцию нельзя покрыть юнит-тестами, а вот в таком — можно"
— баги
— секурити проблемы
— отхождение от принятого дизайна/архитектуры — вопросы "почему конфиг используется напрямую"
— где тесты "вот этой штуки"
— непонятные места — вопросы "а что это за константа"
до кучи — накидывать тикеты на те части, что требуют доработки. Это проще, чем переписываться неделями. В пр нужно просто указать, что создан тикет "исследовать, что будет в случае xxx при условии yyy"
стили, запятые, форматирование, линтинг — это всё мусор, решается автоматикой
много полезных вещей которые не нужно совать в ПР
— дизайн-архитектура, если это не просто какое то сиюминутное расхождение. Для этого лучше запланировать время на дизайн-ревью итд, заранее, до начала работ
— "херли ты пакеты публикуешь без тестов-билдов-доки" — это идти сразу к пм
— проверку каждой функции-строчки-итд — для этого есть другие подходы
Здравствуйте, Буравчик, Вы писали:
Б>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>Какими должны быть правильные комментарии к PR/MR/т.п.?
Б>(Навеяно темой в "коллеги улыбнитесь")
missed null check, broken flow (details), hidden bug (details). Ignored exception. — Это то, что по делу.
Не по делу — "А как это вообще работает? Мне непонятно. Переделать! Need Work!!!!
Здравствуйте, Doom100500, Вы писали:
D>Не по делу — "А как это вообще работает? Мне непонятно. Переделать! Need Work!!!!
Кстати, "А как это вообще работает? Мне непонятно" обычно указывает, что в коде не хватает комментариев.
Задаешь этот вопрос, зачем это здесь? — и появляется либо описание магии, либо магия заменяется на что-то более простое. В обоих случаях код становится лучше.
Здравствуйте, Буравчик, Вы писали:
Б>Здравствуйте, Doom100500, Вы писали:
D>>Не по делу — "А как это вообще работает? Мне непонятно. Переделать! Need Work!!!!
Б>Кстати, "А как это вообще работает? Мне непонятно" обычно указывает, что в коде не хватает комментариев.
Это значит, что был пропущен этап "Design Review".
Б>Задаешь этот вопрос, зачем это здесь? — и появляется либо описание магии, либо магия заменяется на что-то более простое. В обоих случаях код становится лучше.
Ну я, в таких случаях пишу развёнутее. Например это бессмысленный код потому-что мы это уже сделали, или сделаем там сям. Короче сложно рассуждать абстрактно, но я подразумеваю, что люди, которые делают ревью, находятся хотя бы близко к скопу этого кода, и всегда есть что сказать конкретно вместо того, чтобы расчитывать, что кто-то умеет читать мысли.
Здравствуйте, Alekzander, Вы писали:
A>Здравствуйте, Буравчик, Вы писали:
Б>>Какие комментарии в код-ревью вы считаете "по делу", а какие нет? Б>>Какими должны быть правильные комментарии к PR/MR/т.п.?
Б>>(Навеяно темой в "коллеги улыбнитесь")
A>Вообще считаю код-ревью плохой практикой. Комментарии надо собирать, когда код ещё пишется, а не когда код уже написан. Например, тимлид может несколько часов в неделю посидеть с каждым. Или люди могут периодически работать вместе, показывать друг другу свой код. А когда код написан, и его надо переписывать, это приводит к прохождению через все стадии гнева/отрицания/торга/принятия, а это отнимает силы и мотивацию. Жаль, что мало команд умеет обходиться без этого всего.
Дело в том, что на практике в условиях не хватки времени, часто время тратиться на вылизывание кода который ни на что не влияет, типа контроллеров wep-api каких не будь, а код отвечающий за безнес-логику ни кто не трогает от греха подальше.
Здравствуйте, Alekzander, Вы писали:
A>...А когда код написан, и его надо переписывать, это приводит к прохождению через все стадии гнева/отрицания/торга/принятия, а это отнимает силы и мотивацию. Жаль, что мало команд умеет обходиться без этого всего.
Ну так не надо, конечно. Порефакторить после ревью — да, возможно, но не переписывать. Если код уже написан, и делает то, что надо нужным образом (типа хранение в таблице БД, а не файлах), то переписывать его не нужно — доработать намного проще.
Всё сказанное выше — личное мнение, если не указано обратное.
Здравствуйте, Alekzander, Вы писали:
A>Вообще считаю код-ревью плохой практикой.
охотно бы не согласился, если не поработал бы в компании без code review.
На практике получается вполне недурно. Ну реально блин, 95% времени ты отбиваешься от вкусовщины ревьювера. Да, остальные 5% — дельные замечания, но стоит ли оно потраченного времени? Реальные баги всё равно эффективнее выловит система тестирования, чем разработчик просто пробежав глазами. Посчитайте сколько уходит времени на ревью у вас. У меня может уходить x3 от времени собственно разработки. В компании без ревью я прям был приятно удивлён как быстро проходит процесс принятия MR.
Похоже на ситуацию с остальными практиками agile. Почти все превратились в бюрократических монстров, которые уже давно не служат главной цели — экономить время, а работают только на поддержание бюрократического аппарата в виде менеджеров и бесконечных митингов.
Здравствуйте, Sinclair, Вы писали:
S>Ревью призвано выявлять не баги, а говнокод. Цель — минимизация накопления техдолга.
так определение говнокода — это и есть вкусовщина. Одному кажется, что код слишком тесно связан и надо добавить абстракций, другой возразит что это будет обычный оверинженеринг. В итоге ломается куча копий а движения нет.
И посыл что техдолг накапливается только из-за говнокода, мне кажется ошибочным. Техдолг возникает в основном из-за неопределённых требований. На первом этапе зачастую надо сделать быстрый прототип, быстро нарастить функционал, быстро показать заказчику. Поэтому избирается максимально простой дизайн без лишних наворотов. Но затем то требования растут по мере успешности проекта. И тут то и образовывается техдолг. То есть это объективный процесс и напрямую не связан с квалификацией разработчиков и качеством кода.
