Comments 32
UFO just landed and posted this here
Можно навязать единый code style и реально проверять, что он соблюдается, а не просто на бумажке закрепленИ это должен делать софт, а то Code Review превратится в Code-Style Review. Благо сегодня каждый ЯП обзавелся дюжиной статический анализаторов.
UFO just landed and posted this here
Вы правильно сказали, что это должен делать софт, но неправильно назначили ответственных за это :) Это работа не статических анализаторов, а обычного IDE, или правильно настроенного продвинутого текстового редактора.
Стоит один раз создать файл стиля, принятого в компании, и проблема решена.
Стоит один раз создать файл стиля, принятого в компании, и проблема решена.
IDE многое пропускают и они могут быть разные. В моей команде, например, используется минимум 3 различных IDE/текстовых редакторов и нам приходится использовать инструменты jshint + jscs + editorconfig + csscomb, чтобы уровнять редакторы.
У многих IDE, конечно, есть интеграция с jshint, но он отловит только часть проблем, да и ничего не мешает забыть/не заметить/проигнорировать замечание IDE.
Я предпочитаю максимально минимизировать потенциальную проблему всеми возможными средствами. С текущим workflow это прекрасно получается. За последний год я не видел ни одного комента вида «а ты тут пробел забыл».
У многих IDE, конечно, есть интеграция с jshint, но он отловит только часть проблем, да и ничего не мешает забыть/не заметить/проигнорировать замечание IDE.
Я предпочитаю максимально минимизировать потенциальную проблему всеми возможными средствами. С текущим workflow это прекрасно получается. За последний год я не видел ни одного комента вида «а ты тут пробел забыл».
Можно заставить это делать не в IDE.
Несколько лет назад, когда у нас команда была сильно разношерстной, проверка стиля была прикручена на pre-commit hook в svn. Первый месяц многих бесило, что нельзя закоммитить код, в котором пробел или фигурная скобочка не там, но потом как-то страсти улеглись и у всех получалось закоммитить код с первого раза, без претензий к стилю кода.
Ну и, кроме этого, в самой IDE прикручен файл стиля, и неправильно стилизованный код в IDE подчеркивается.
Несколько лет назад, когда у нас команда была сильно разношерстной, проверка стиля была прикручена на pre-commit hook в svn. Первый месяц многих бесило, что нельзя закоммитить код, в котором пробел или фигурная скобочка не там, но потом как-то страсти улеглись и у всех получалось закоммитить код с первого раза, без претензий к стилю кода.
Ну и, кроме этого, в самой IDE прикручен файл стиля, и неправильно стилизованный код в IDE подчеркивается.
Задача reviewerа — удостовериться в том, что код, написанный автором корректен.
Я думаю, что вот эта фраза как раз и требует отдельной статьи. Потому что самый важный момент CR совсем не расписан. Я проверяю чужой код каждый день и проблема чаще всего не в том, что код не корректный, а в том, что программист в силу не знания того или иного framework-a пишет не эффективный код и я, как «носитель знаний» системы, показываю что какая-то часть его кода уже есть где-то, или что другая часть кода это функциональность framework-a который мы используем. Тем самым я делюсь с ним знаниями о системе и framework-е.
Странно, что комментирование кода это то, что должно обидеть разработчика, я наоборот положительно отношусь к тому, чтобы в моем коде мне показали на мои ошибки или мы могли обсудить, как это можно улучшить… У вас же CR больше сводиться к галочке — код просмотрен, ничего страшного не замечено. Но должны также соблюдаться основные архитектурные особенности, которые были приняты в проекте… или еще какие-то правила, которые не могут быть проверены статическими анализаторами.
Но в целом, спасибо за статью, есть над чем подумать.
Я думаю, что вот эта фраза как раз и требует отдельной статьи. Потому что самый важный момент CR совсем не расписан. Я проверяю чужой код каждый день и проблема чаще всего не в том, что код не корректный, а в том, что программист в силу не знания того или иного framework-a пишет не эффективный код и я, как «носитель знаний» системы, показываю что какая-то часть его кода уже есть где-то, или что другая часть кода это функциональность framework-a который мы используем. Тем самым я делюсь с ним знаниями о системе и framework-е.
Странно, что комментирование кода это то, что должно обидеть разработчика, я наоборот положительно отношусь к тому, чтобы в моем коде мне показали на мои ошибки или мы могли обсудить, как это можно улучшить… У вас же CR больше сводиться к галочке — код просмотрен, ничего страшного не замечено. Но должны также соблюдаться основные архитектурные особенности, которые были приняты в проекте… или еще какие-то правила, которые не могут быть проверены статическими анализаторами.
Но в целом, спасибо за статью, есть над чем подумать.
Про комментирование сказано потому, что при комментировании обычно (сколько себя помню) все комментарии сводятся сводится к «этот код %вставить_нехорошое_слово%, потому что я написал бы не так, и единственный верный способ — сделать это так, как сделал бы я». Всё. Точка. И этим грешат по-моему абсолютно все, кого я встречал на ревью.
Какой-то детский пост, складывается впечатление, что автор пришел в компанию, впервые увидел CR, услышал про достоинства процесса, и с радостью об этом говорит. Я участвовал во внедрении CR в двух распределенных командах, которые ранее не делали CR. То, что делать CR нужно — всем понятно, однако подводных камней хватает. Вот типичные проблемы, которые приходится решать:
1. Сколько времени ждать CR? Можно ли считать молчание (24, 48, 72 часа) знаком согласия? Как длительное отсутствие ответа ревьюеров соотносится со срочностью проекта?
2. Кто назначает ревьюера? Автор, техлид, менеджер, или добровольцы сами вызываются? Кто может быть ревьюером? Сколько ревьюеров нужно?
3. Как искать ревью ретроспективно? Это очень нужно для суппорта.
4. Что именно проверяется в ревью? На сколько ревьюер полагается на «доказательства» корректности, представленные автором?
5. Как убедить начальство во временных затратах на CR?
6. Как избежать негативных и перегретых обсуждений во время CR? Примеров крупных ссор, начавшихся с CR, немало.
7. Нередко при CR выясняется, что до написания кода следовало бы сделать design review. Надо найти баланс, какие изменения кода требуют design review, чтобы избежать переделок кода после непройденного CR.
Если тема покажется интересной community, то можно подготовить пост на эту тему
1. Сколько времени ждать CR? Можно ли считать молчание (24, 48, 72 часа) знаком согласия? Как длительное отсутствие ответа ревьюеров соотносится со срочностью проекта?
2. Кто назначает ревьюера? Автор, техлид, менеджер, или добровольцы сами вызываются? Кто может быть ревьюером? Сколько ревьюеров нужно?
3. Как искать ревью ретроспективно? Это очень нужно для суппорта.
4. Что именно проверяется в ревью? На сколько ревьюер полагается на «доказательства» корректности, представленные автором?
5. Как убедить начальство во временных затратах на CR?
6. Как избежать негативных и перегретых обсуждений во время CR? Примеров крупных ссор, начавшихся с CR, немало.
7. Нередко при CR выясняется, что до написания кода следовало бы сделать design review. Надо найти баланс, какие изменения кода требуют design review, чтобы избежать переделок кода после непройденного CR.
Если тема покажется интересной community, то можно подготовить пост на эту тему
Тема интересна.
Пожалуйста, говорите! На хабре мало метериалов по техническим деталям CR.
Вот как мы решили некоторые из этих типичных проблем:
1. CR происходит в течение от нескольких минут до двух-трех часов. Commit не может быть без активности 24 часа, и автор получает напоминание о том, что хорошо бы что-либо сделать с ним. Если 5 дней нет активности, тогда «Abandon change» (мы используем Gerrit)
2. Автор назначается сам, пришел на работу/пришел с паузы/конец рабочего дня — берешь и проверяешь пару коммитов
5. Проблема не стоит, начальство в теме
6. У нас нет ссор из-за CR.
Вот как мы решили некоторые из этих типичных проблем:
1. CR происходит в течение от нескольких минут до двух-трех часов. Commit не может быть без активности 24 часа, и автор получает напоминание о том, что хорошо бы что-либо сделать с ним. Если 5 дней нет активности, тогда «Abandon change» (мы используем Gerrit)
2. Автор назначается сам, пришел на работу/пришел с паузы/конец рабочего дня — берешь и проверяешь пару коммитов
5. Проблема не стоит, начальство в теме
6. У нас нет ссор из-за CR.
Суть не в том, чтобы найти оптимальные правила для всех, ибо таких не существует. Правила варьируются в зависимости от характера проекта, состава команды и т п. Но есть общие вопросы, которые надо задать, обсудить и прийти к решению при внедрении QR. Вот эти вопросы я и попытался перечислить.
6. Вот и я ссор из-за CR не встречал. Хотелось бы узнать откуда ноги растут :) А вдруг где-нибудь да случится такое!
Мне тоже интересно. Работал в командах с CR, но многих ваших пунктов не встречал.
Например, ни разу не видел крупных ссор. Ясно полагаю, что такое возможно. Хотелось бы узнать как решались такие проблемы :)
Еще описание самого процесса интересно. И думаю для многих было бы полезно.
Казалось бы все должно быть просто в организации в процесса. Но нет!
1. Работал в компании, где ревьюили каждый коммит в течение 2-х часов и это было круто.
2. Потом работал в компании, которая пыталась внедрить CR. Вся команда в пятницу на 4 часа садилась ревьюить чужой код. Выдавался какой-то непонятный участок кода. Это было очень скучно, не интересно и малоэффективно. Я часто отвлекался на новости, потому что 4 часа читать чужой код очень сложно :) Мой рассказ как это нужно делать не возымел тогда успеха.
Например, ни разу не видел крупных ссор. Ясно полагаю, что такое возможно. Хотелось бы узнать как решались такие проблемы :)
Еще описание самого процесса интересно. И думаю для многих было бы полезно.
Казалось бы все должно быть просто в организации в процесса. Но нет!
1. Работал в компании, где ревьюили каждый коммит в течение 2-х часов и это было круто.
2. Потом работал в компании, которая пыталась внедрить CR. Вся команда в пятницу на 4 часа садилась ревьюить чужой код. Выдавался какой-то непонятный участок кода. Это было очень скучно, не интересно и малоэффективно. Я часто отвлекался на новости, потому что 4 часа читать чужой код очень сложно :) Мой рассказ как это нужно делать не возымел тогда успеха.
Вот один из известнийших примеров с Linux Kernel mail list
marc.info/?l=linux-kernel&m=137390362508794&w=2
Даже не вникая в суть дискуссии, понятно, что если дошло до F-word, то конфликт имеется.
Как решать проблемы? Как и любые другие ссоры в жизни. Если люди работают в одной компании, то менеджмент способствует улаживанию конфликта, а если в разных — то как-то сами справляются.
Ссоры и недопонимание возникают чаще в распределенных командах, где накладываются невозможность поговорить лично, разница культур, разница во времени, степень владения английским языком. Можно поискать примеры для каждой из перечисленных причин.
Вот у Вас отличный пример того, что неправильно построенный процесс привел к неприятию его членами команды. С внедрением QR проецсса часто бывает, что если на первый раз «не пошло», то второго шанса не дают.
marc.info/?l=linux-kernel&m=137390362508794&w=2
Даже не вникая в суть дискуссии, понятно, что если дошло до F-word, то конфликт имеется.
Как решать проблемы? Как и любые другие ссоры в жизни. Если люди работают в одной компании, то менеджмент способствует улаживанию конфликта, а если в разных — то как-то сами справляются.
Ссоры и недопонимание возникают чаще в распределенных командах, где накладываются невозможность поговорить лично, разница культур, разница во времени, степень владения английским языком. Можно поискать примеры для каждой из перечисленных причин.
Мой рассказ как это нужно делать не возымел тогда успеха.
Вот у Вас отличный пример того, что неправильно построенный процесс привел к неприятию его членами команды. С внедрением QR проецсса часто бывает, что если на первый раз «не пошло», то второго шанса не дают.
Мне кажется иногда решать проблему с помощью начальства — означает решать не проблему, а симптомы проблемы.
А действительно решить проблему — это поставить процесс так, чтобы до начальства вовсе дело не доходило, т.е. когда прецендентов вовсе нет.
Хотя идеального, наверное, ничего не бывает. Есть только постоянное стремление к совершенству. Тогда задача начальства свести количество подобных эскалаций наверх к минимуму.
А действительно решить проблему — это поставить процесс так, чтобы до начальства вовсе дело не доходило, т.е. когда прецендентов вовсе нет.
Хотя идеального, наверное, ничего не бывает. Есть только постоянное стремление к совершенству. Тогда задача начальства свести количество подобных эскалаций наверх к минимуму.
Поддерживаю
Из моих наблюдений, на ревью люди распадаются на две категории — первая не проверяет почти ничего, задачу видят в том чтобы было «в целом нормально», вторые роют до дна и видят задачу в том чтобы «подписаться под этим». Деление практически бинарное. И вторых конечно много меньше. Проблема тут в том что — для того чтобы провести качественное ревью нужно работать на том же уровне что и программист его писавший — т.е. собрать все требования, вникнуть в задачу, понять решение, пройти по алгоритму. Получаются затраты, сравнимые с написанием этого же кода. Зато и результат в таком случае — нахождение всего объёма проблем, от архитектуры до "= вместо ==".
Эта картинка вначале не дала мне сосредоточиться на тексте, всё время в голове повторял это:
Я бы еще добавил к плюсам ревью «обучение молодых программистов».
Я был молод, когда попал в команду с CR. И тогда я писал много бякакода. За пол года более опытные программисты научили меня писать нормальный код, просто делая ревью моего кода и давая нужные советы.
Я был молод, когда попал в команду с CR. И тогда я писал много бякакода. За пол года более опытные программисты научили меня писать нормальный код, просто делая ревью моего кода и давая нужные советы.
Пара вопросов по процессу разработки:
1) Как сделать так, чтобы коллеги достаточно быстро отсматривали ревью?
2) Как сделать так, чтобы коллеги сосредотачивались на том, насколько функционально правилен код, а не на code style? Так как, при отвлечении внимания на второе, очень высока вероятность пропустить проблемы в первом кейсе…
1) Как сделать так, чтобы коллеги достаточно быстро отсматривали ревью?
2) Как сделать так, чтобы коллеги сосредотачивались на том, насколько функционально правилен код, а не на code style? Так как, при отвлечении внимания на второе, очень высока вероятность пропустить проблемы в первом кейсе…
п.2 применять проверяльщики стиля, к примеру для Python есть общепринятый стиль PEP8 и есть тулзы проверяющие согласованность кода по этому стилю и выдающие ошибки. К примеру 'pip install pep8' и прияменяя тулзу 'pep8' можно получить список ошибок. А все это действие повесить на хук при комите так чтобы ни один из разработчиков в команде НЕ МОГ продвинуть код не по стилю.
1. Покажите коллегам эту статью и обратите их внимание на вот этот кусок:
2. Присоединяюсь к вышестоящему ответу. Для PHP — PSR2, для JS — eslint.
Обязательное условие проверки code style — она должна быть автоматической, т.е. в вашем редакторе/IDE должен быть плагин который делает это сам, например на onsave.
Третье — скорость. Не нужно сломя голову бежать проверять только что посланный на review код, но с другой стороны нужно делать это быстро. Ваши коллеги ждут вас. Если вы и ваши коллеги не уделяете время тому, чтобы CR был сделан, причем сделан быстро, тогда CR может стать причиной недовольства людей в коллективе. Может показаться, что это переключение фокуса — взяться за CR. Это не совсем так. Не нужно бросать все в тот момент, когда новый код послан на review. Но в течение нескольких часов вы совершенно точно сделаете паузу для того, чтобы попить что-нибудь, сходить в туалет или просто походить. Когда вы вернетесь с паузы уделите внимание CR. Если вы возьмете это в привычку, то никто в команде не будет подолгу ожидать ваш review и он не будет доставлять никакого дискомфорта в работе.
2. Присоединяюсь к вышестоящему ответу. Для PHP — PSR2, для JS — eslint.
Обязательное условие проверки code style — она должна быть автоматической, т.е. в вашем редакторе/IDE должен быть плагин который делает это сам, например на onsave.
1. Ну вот не хотят они уделять время для ревью, приходится по 5 раз тыкать палкой :)
2. Код заведомо соответствует PEP8. Под кодстайл, который может не понравится ревьюверу, я имею ввиду, например, названия переменных, внутренние соглашения компании и тд. Но при проверке их ревьюверы, как правило, уходят от сути задачи и упускают возможные ошибки в самом решении.
2. Код заведомо соответствует PEP8. Под кодстайл, который может не понравится ревьюверу, я имею ввиду, например, названия переменных, внутренние соглашения компании и тд. Но при проверке их ревьюверы, как правило, уходят от сути задачи и упускают возможные ошибки в самом решении.
п.2. Если по PEP8 это уже офигенный задел на будущее!!! А остальное: название переменных и др. можно попробовать автоматизировать с помощью своих тулзов, к примеру pyparse. Но если честно признаться, то это уже обычная, рутинная работа обычного программера. Не всегда сразу удается дать хорошее название переменной или методу.
Код коллег никто не ревьюит? Это же взаимный процесс: ты ревьюишь быстро, тебя тоже быстро проверяют.
У меня есть скрипт, который три раза в день (после стенд-апа, после обеда и за час до конца рабочего дня) в слак пишет список открытых пулл-реквестов. Там же в слаке есть команда, которая триггерит этот «дайджест» вне расписания. На мой взгляд удобно, открыл список, выбрал то, что тебе нравится, проверил и дальше работаешь.
Ну, и во всяких Скрамах есть такая штука, как ретроспектива. Обсудите с командой, почему они не хотят проводить ревью?
У меня есть скрипт, который три раза в день (после стенд-апа, после обеда и за час до конца рабочего дня) в слак пишет список открытых пулл-реквестов. Там же в слаке есть команда, которая триггерит этот «дайджест» вне расписания. На мой взгляд удобно, открыл список, выбрал то, что тебе нравится, проверил и дальше работаешь.
Ну, и во всяких Скрамах есть такая штука, как ретроспектива. Обсудите с командой, почему они не хотят проводить ревью?
Sign up to leave a comment.
Вещи, которые должен делать каждый: Code Review