Comments 19
Еще из моей практики — очень важно оформление кода. Если ревью — неотъемлемая часть процесса, то нужен и стандарт оформления кода, иначе ревьювер просто на эмоциональном уровне уже может быть «отрицательно заряжен» так сказать.
А я бы и по поводу архитектурного дизайна докапывался. Это как раз то, что «отливается в граните» навеки. Неудачное решение лучше переделать сразу, чем дать ему укорениться в проекте.
Автор кода должен был сначала все продумать и обсудить архитектуру с коллегами, провести дизайн-ревью. На этом этапе все поменять было бы элементарно. И только потом садиться писать код.
Автор кода должен был сначала все продумать и обсудить архитектуру с коллегами, провести дизайн-ревью. На этом этапе все поменять было бы элементарно. И только потом садиться писать код.
Примите как данность, что многие архитектурные решения — это вопрос личного вкуса разработчика. Старайтесь обсуждать только сильные/слабые стороны и как можно быстрее принять решение что делать с кодом.
Может быть мы о разных вещах говорим. Я имею в виду не вкусовщину, а именно ошибки проектирования архитектуры. Например, «смешивание модели данных с их представлением затруднит в будущем написание мобильного приложения». Можно сразу автора «обрадовать» и попросить переделать, а можно дать ему закоммитить это безобразие, а потом кому-то привалит «счастье» распутывать всё.
ИМХО, это зависит от того, кому этот код дальше поддерживать и развивать. Если автору, то в большинстве случаев «родная» архитектура для него будет ближе и понятнее, чем «чужая» или плод коллективного разума. Конечно, есть вырожденные случаи когда все плохо. Но в таких случаях тоже можно не про архитектуру написать, а про то что именно плохо. Например: «мы ожидаем до 10кк пользователей, это решение не будет работать, если пользователей больше 10к, см. потребление памяти». И пусть сам думает что с этим делать :)
И ещё дополнение. Если ревьюеру что-то не понятно в коде, автор должен не лично ревьюеру это объяснить, в комментах к ревью и т.д., а либо упростить код и сделать его более понятным, либо привести текстовые объяснения прямо в коде в виде комментариев языка программирования. Ибо если ревьюер чего-то не понял, то и следующие N человек, кому надо будет разобраться, тоже не поймут.
Говорят, в Google такой подход. Там ревьюверы часто отклоняют пулл реквесты с формулировкой «я ничего не понял, переделать»
Комментарий в таком духе — это, в первую очередь, неуважение к автору кода. Я такого ни разу не встречал. Обычно все комменты очень логичные и содержательные. Если что-то конкретное непонятно, то спрашивается, что именно. Например: «почему ты здесь делаешь X, если в других аналогичных местах везде делается Y?» И автор, если он действительно делал X намеренно, может оставить комментарий в коде: «Здесь X, потому что Z». Тогда следующий читатель кода не будет голову ломать, почему. Как-то так.
Ну а если код непонятен настолько, что ревьюер даже ничего комментировать не хочет, это мало того, что неуважение к ревьюеру и отвержение надобности код ревью, так еще и проблемы в будущем для проекта.
Неуважение надо отличать от кнута. Не знаю как остальным, мне, например, очень не нравится, когда меня кто-то «хвалит». Гораздо приятнее, когда получаешь пинка хорошенького и заставляешь себя переделывать что-то в лучшей форме. Это на пользу каждому.
Например на codereview.stackexchange.com часто вижу такие вопросы, заминусованные сообществом, потому что ни черта не ясно вообще. Это не неуважение, это нежелание тратить свое время на разбор кода либо слабого инженера, либо инженера, который не удосужился позаботиться о читаемости и понимании кода.
К тому же, обычно ревьюверы — куда более опытне инженеры/архитекторы, и я вполне представляю себя на месте вышеуказанных гуглеров, которые не могут понять, зачем тратить время на ревью плохого/непонятного кода.
Неуважение надо отличать от кнута. Не знаю как остальным, мне, например, очень не нравится, когда меня кто-то «хвалит». Гораздо приятнее, когда получаешь пинка хорошенького и заставляешь себя переделывать что-то в лучшей форме. Это на пользу каждому.
Например на codereview.stackexchange.com часто вижу такие вопросы, заминусованные сообществом, потому что ни черта не ясно вообще. Это не неуважение, это нежелание тратить свое время на разбор кода либо слабого инженера, либо инженера, который не удосужился позаботиться о читаемости и понимании кода.
К тому же, обычно ревьюверы — куда более опытне инженеры/архитекторы, и я вполне представляю себя на месте вышеуказанных гуглеров, которые не могут понять, зачем тратить время на ревью плохого/непонятного кода.
Это же типичный вопрос про курицу и яйцо, только здесь на него есть однозначный ответ.
Мне код на PR не с альфа-центавры доставляют, автор — вон он сидит, в пяти метрах. И если вместо того, чтобы позаботиться о будущем проекта в целом и здоровья рассудка команды в частности, он вывалил какую-то кашу из своего подсознания и тут же нажал кнопку «готово для ревью» — он проявил неуважение первым. Поэтому если я проскроллил один экран и перестал понимать, о чем это вообще, я отправляю задачу на доработку именно с такой пометкой, только еще короче: «Unreadable.»
«Уважение» — это такой зверь, который никак не зависит от того, что кто-то в какой-то момент может написать плохой код. И уж подавно уважение — не синоним толерантности. Я вообще очень люблю повторять «fuck your ego».
Мне код на PR не с альфа-центавры доставляют, автор — вон он сидит, в пяти метрах. И если вместо того, чтобы позаботиться о будущем проекта в целом и здоровья рассудка команды в частности, он вывалил какую-то кашу из своего подсознания и тут же нажал кнопку «готово для ревью» — он проявил неуважение первым. Поэтому если я проскроллил один экран и перестал понимать, о чем это вообще, я отправляю задачу на доработку именно с такой пометкой, только еще короче: «Unreadable.»
«Уважение» — это такой зверь, который никак не зависит от того, что кто-то в какой-то момент может написать плохой код. И уж подавно уважение — не синоним толерантности. Я вообще очень люблю повторять «fuck your ego».
Unreadable — понятие относительное. Если потакать такому поведению — ревьювер может облениться и даже не напрягаться понять. И если какой-то ревьювер ничего не понял, это совсем не значит, что код плохой. Читая исходники ядра FreeBSD я тоже пока мало что понимаю, но это совсем не означает, что код плохой. Если кто-то не понимает, например, алгоритм Ахо-Корасика, то это совсем не означает, что этот алгоритм плохой.
А зачем отдавать на ревью менее опытному члену команды? Нет, это тоже полезно, чтобы у всех было владение кодом, чтобы убедиться что оно читабельно для слабых и т.п., но слабому «не по рангу» будет писать вот так, а логичнее задавать вопросы.
Если же человек имеющий достаточно высокий опыт не может быстро понять что тут и о чем, то да, это всё надо возвращать на доработку. Не понял как сделать читаемо? Подойди спроси. Не обязательно у того, кто забраковал. У того, кого не забраковывают. Знаешь, что ты забраковываешь нечитабельную кашу студента/новичка? Подойди, потом, расскажи ему бестпрактик о том как писать читабельно. Или напиши его куратору если структура организации большая и он далек от твоей компетенции. Зачем лекции в ревью?
Писать читабельно это вопрос дисциплины и небольшого количества вещей которые нужно выучить.
Ну будет писать в три раза больше комментариев чем нужно — ничего страшного.
Выучит корпоративный стантарт кода, научится метко и емко называть сущности — сможет сократить.
Но обучение не входит в цели ревью и должно проходить вне ревью.
Если же человек имеющий достаточно высокий опыт не может быстро понять что тут и о чем, то да, это всё надо возвращать на доработку. Не понял как сделать читаемо? Подойди спроси. Не обязательно у того, кто забраковал. У того, кого не забраковывают. Знаешь, что ты забраковываешь нечитабельную кашу студента/новичка? Подойди, потом, расскажи ему бестпрактик о том как писать читабельно. Или напиши его куратору если структура организации большая и он далек от твоей компетенции. Зачем лекции в ревью?
Писать читабельно это вопрос дисциплины и небольшого количества вещей которые нужно выучить.
Ну будет писать в три раза больше комментариев чем нужно — ничего страшного.
Выучит корпоративный стантарт кода, научится метко и емко называть сущности — сможет сократить.
Но обучение не входит в цели ревью и должно проходить вне ревью.
Fuck нельзя говорить никому и никогда. Ну, может только если ногу сломал и никто не слышит.
Описанные в статье подходы полезны не только для код ревью, но и вообще при общении между членами команды
Sign up to leave a comment.
Перевод: Инструкция по проведению code review