Comments 25
О код ревью, если кратко, это самый лучший способ испортить отношения с командой. Если человек пишет полную дичь, я ему об этом сообщу. Если человек пишет дичь только наполовину, то я ему об этом сообщать не буду. Если тебе говорят сделать полный бред, то я об этом сообщу. А если наполовину, то нет. У тебя у самого проблемы с безопасностью начнутся, если тебя будут пытаться закритиковать до смерти. Особенно если учесть тот фактор, что начальство имеет право удалённо следить за рабочими мониторами. Ревью должны делать аудиторы.
Вот, простой пример из реальной жизни: человек случайно назвал переменную Snake Case-ом. Вроде first_name, а не firstName. Язык Java. Ревью, комментарий: "Смесь Camel Case и Snake Case в классе". Человек был на испытательном сроке. Человека после этого чуть не уволили. Из-за подобных вещей атмосфера становится токсичной. Ну да, у нас серпентарий, добро пожаловать.
Почему-бы не использовать анализатор кода?
Вон, на яву полно всяких есть (Не силён в яве):
https://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis
Проблема в том, когда начинают обращать внимание на стилистику кода, вместо анализа самого кода, то больше шансов что пропустят действительно ошибку.
Ибо проверить стилистику может и программа, а вот найти логическую ошибку - может только человек.
В некоторых фирмах анализатор используется, а в некоторых нет. По поводу логических ошибок, эффективно самому дебажить. Потому что ты читаешь код и сомневаешься в том, что оно вообще работает и что человек вообще проверял работоспособность этого кода. И большие сомнения, что сценарии поведения в документе были правильно написаны. Не все могут работать на legacy
В некоторых фирмах анализатор используется, а в некоторых нет
Ну анализаторы зачастую забесплатно раздаются, так что его и бизнесу-то продавать не надо особенно. Это ведь не M$ SQL Server Enterprise или IBM Web Sphere за кучу денег.
Хоть просто себе его поставить и настроить для начала. Как Notepad++, ConEmu, 7-Zip, IrfanView и т.п.
Потому что ты читаешь код и сомневаешься в том, что оно вообще работает и что человек вообще проверял работоспособность этого кода
Ну а человек просто по невнимательности что-то написал и не проверил. Или написал, проверил, а потом решил код причесать и не проверил. Человеческий фактор никуда не денется в любом случае. Да и анализатор это ведь не полноценный CI/CD, а просто дополнение к IDE или компилятору.
Уволить из-за ошибки в названии переменной? Ну может и ну его нафиг в такой фирме работать действительно...
У меня на проде, уверен лежит парочка не так названных переменных, а одно время целый интерфейс и соответственно куча его наследников были неправильно названы из за ошибки русской/английской с/c.
А проект самый высоко нагруженный в зелёном банке, ничего, никто не умер, исправили как нашли.
Ну и со стороны стажёра расстраиваться из за того что в замечании нет слов "ничего страшного, пожалуйста не волнуйся, но вот тут небольшая ошибочка" это тоже странно, как в обществе то жить, если в любом автобусе есть шанс ушат помоев огребсти)
Code Review заменить сложно!
Я тоже искала альтернативы, чтобы сэкономить время, но ни одна "альтернатива" такого итога не предвещает = (
не всех разработчиков можно посадить рядом
С шарингом экрана при видеозвонке вполне себе норм получается.
Code With Me тоже неплохо работает, если оба разраба используют IDE JetBrains
Это скорее было про уровень инженеров + их софт скиллы, а не про их географическую расположенность, не дописал мысль:) Если посадить рядом стажера и супер мега опытного человека, то для первого будет очень полезно конечно, а для второго это потеря времени в разрезе времени выполнения задачи. Конечно тут уже возникают другие преимущества в виде менторинга, но это уже отдельная задача, которая целью Code Review не является
DesignReview - если сложная фитча вполне возможен в твоей ветке, нет?
И CodeReview на PR.
И можно парное программирование, если пишем для банка допустим, где цена ошибки заоблочная.
Не понимаю, что не так то?
Про codeReview там принципиальная ошибка. Человек делающий codeReview не проверяет архитектуру, и не вдумывается в код и не знает бизнес-логики.
У меня была 10 работ и только три из них, с code-review. Отгадайте, где был самый плохой код?)
Кажется разумным автоматизировать все, что можно автоматизировать: линтеры/форматтеры кода и прочие плюшки, которые делают код красивым. Чтобы не тратить на это время при ревью.
Иметь набор договоренностей и стайлгайды, которым следует команда. Но они могут пересматриваться
Делать какую-то спецификацию дизайн разумно, если делается что-то новое. Можно выделить отдельным этапом во флоу разработки, и еще на этапе анализа понять как делать.
Вы точно мне ответили? Мне ваще всё равно на всё эти стайлы, из минусов только, если каждый по своему форматов в коммиь куча всякой отформатмрованного кода попадает.
Но вон какая по дефолту настройка стоит, ту и юзают обычно.
Спецификация дизайн - это что вообще?
Ответ был не Вам, а общим комментарием к статье)
Понятно, что там где нет code-review все будет очень плохо, сам работал без него однажды
Использовать дефолтные настройки не очень правильно, потому что у разработчиков могут быть разные IDE с разными дефолтными настройками. Разумно иметь единый источник правил, чтобы у всех разрабов был единый формат кода (был случай, когда поменял одну строку, но переформатировался весь файл).
"Спецификация дизайн", опечатался немного (спецификация или дизайн-ревью), но подразумевал спецификацию решения, или дизайн ревью решения. Какая-то активность, результатом которой будет описание технического решения, которое устроит остальных разработчиков, системных аналитиков или архитекторов (смотря какая команда)
У меня обратный опыт и убеждение, что код ревью И юнит тесты вводят на в ноль закаканных проектах, где каждая строчка может всё свалить и требуются тройные проверки. И это та жизненная необходимость,. Если код простоя и качественный, то и юнит тесты ни код ревью.
Но даже я был бы рад, увидеть какие нить независимые исследования, где показана корреляция между наличием тестов,код ревью и качеством кода.
В качестве дурацкого примера могу привести, то что в Норвегии даже Аналога нашего ОМОНа не существует, но это не значит, что там преступность распоясалась.
Мы вдвоём с коллегой разработали проект за одним компьютером в несколько месяцев. Очень классно. Главное, чтобы опыт был неновичковый. Идеи проговариваются, критикуются, совершенствуются очень быстро.
на практике визуальные составляющие кода отлавливаются и исправляются именно на code review
Это не самый удачный вариант, нужно сдвигать контроль оформления влево. Для большинства популярных языков есть и линтеры, и форматтеры, настройки (соглашения) как правило легко шарятся в виде закоммиченного в папку проекта файла. Разработчику нужно выдать экстеншн или инсталлятор того же инструмента, контроль переложить на CI, который бы не пропускал несоответствия.
задача по code review ставится в Jira
Бывает и такое, что обеденный перерыв ставят и в JIRA, и в календарь Outlook. Но до этого должен был случиться неслабый такой поворот не туда. Если это что-то решает, то почему бы и нет, но пахнет, как говорится, не очень хорошо.
Метод единоличной ответственности инженера
Такое есть, в рассуждениях про QualityGate как правило рассматривается как один из интипаттернов. Назначенный крайним быстро превратится в хранителя единственно правильного мнения и хорошо от этой ситуации не будет никому.
В парном программировании теряем эффективность
Суть рассуждений понятна, но формулировки кажутся не самыми удачными. Есть задачи, над которыми вдвоем сидеть странно и неуместно. Есть задачи, решение которых (далее вольно цитирую вас же) при парном подходе происходит быстрее, решение оказывается лучше, надежнее, эффективнее. И в рассуждениях, видимо, про финансовые затраты имеет смысл учитывать, что один бы делал дольше, сделал бы хуже, на ревью все равно бы двое тратили время, причем второй вникал бы с нуля, потом бы с ненулевой вероятностью все они повторили упражнение, т.к. рефактор и переосмысление непременно случились бы. Рассуждения об эффективности уместны в любом случае, но однозначный вывод возможен только если удастся куда-нибудь какую-нибудь линейку приложить и фактически увидеть, что где-то чего-то больше, а в другой ситуации - меньше.
За счет code review адаптация происходит быстрее
Не смог разгадать этот паззл. Толкать на прод не глядя от только что пришедшего в коллектив сотрудника и никак его не онбордить - это предполагается как альтернатива? Возможно просто не совсем точно сформулировано, но пахнет странным. Как если бы в нового сотрудника молча кидались задачами и только на ревью сообщали "неправильно сделал". Адаптация - она же сама по себе должна существовать, как процесс, тот самый "онбординг". Там же и про местные подходы, архитектуру, инструментарий тоже. А ревью как был самим собой и играл какую-то роль во взаимодействии с остальными разработчиками, тем же элементом процесса и для того же и остался.
Это не самый удачный вариант, нужно сдвигать контроль оформления влево
К сожалению, не все можно автоматизировать в виде линтеров, поэтому до сих пор пишутся стайл гайды. Но вы правы, нужно автоматизировать все, что можно автоматизировать
Бывает и такое, что обеденный перерыв ставят и в JIRA, и в календарь Outlook. Но до этого должен был случиться неслабый такой поворот не туда. Если это что-то решает, то почему бы и нет, но пахнет, как говорится, не очень хорошо.
У нас используется обычная канбан доска, есть статус "в процессе" за ним следует статус "Code Review" Когда требуется отдать задачу в ревью, инженер двигает ее в этот статус и ставит нужного инженера на ревью. Всем удобно, все довольны. Объясните поподробнее пожалуйста, почему это не очень хорошо пахнет?
Суть рассуждений понятна, но формулировки кажутся не самыми удачными....
Со всем написанным полностью согласен, по формулировкам дело вкуса, в ваш я, к сожалению, не попал :)
Не смог разгадать этот паззл. Толкать на прод не глядя от только что пришедшего в коллектив сотрудника и никак его не онбордить - это предполагается как альтернатива?
Постараюсь развернуть мысль
Это было написано к тому, что у code review есть минусы, но при этом, если его убрать, возникает проблема с новыми инженерами, как вы и написали. Как альтернативу в этом случае я как раз ничего не предлагаю, тк считаю code review в данном случае максимально полезной практикой. А адаптация происходит быстрее за счет того, что ревью проводит не один человек, который онбордит новичка, а вся команда(в сумме нескольких задач конечно)
P.S. Благодарю за развернутый комментарий
Мой опыт разработки (больше восьми лет в разных командах) показывает, что лучше всего работает т.н. peer review — когда код проверяет коллега твоего же уровня. В этом случае роли чётко разделены:
Автор кода целиком несёт ответственность за свой код. Однако он не может оценить читаемость — для него его код всегда понятен — поэтому привлекает коллегу для code review.
Ревьюер читает код и убеждается, что код понятен и ему тоже. Он заинтересован в этом, потому что в дальнейшем ему скорее всего придётся поддерживать данный код. Но саму реализацию он не проверяет, доверяя коллеге как эксперту.
Поддержка уровня читаемости кода — задача, с которой code review справляется, не добавляя лишних проблем.
Другой допустимый вариант: когда старший разработчик выступает ментором для джуна. В этом случае он проверяет работу джуна целиком, включая весь написанный им код. Но это уже не совсем code review, это скорее проверка навыков ученика, то есть преподавательская деятельность. Тут роли тоже ясны — ментор готов тратить своё время на обучение джуна, а джун хочет научиться. Ответственность за качество итогового кода ментор целиком и полностью берёт на себя.
Проблемы начинаются, когда на code review пытаются навесить другие функции, пытаясь таким образом удешевить разработку — сэкономить на сеньорах или тестировщиках. Часто это выглядит так:
Есть несколько миддлов, которые не дотягивают до уровня проекта, но были наняты за неимением лучшего (например, других не заинтересовали предлагаемые условия).
Есть один сеньор, который прекрасно ориентируется в проекте, но не успевает всё делать сам.
Руководство вешает на сеньора обязанность проверять за миддлами их работу, надеясь таким образом и разгрузить сеньора, и не упасть в качестве.
В итоге имеем описанный в статье букет: приходится переделывать уже написанный код, миддлы демотивируются, портятся отношения в команде, сеньор всё равно перегружен, не всегда ясно кто ответственен за ошибки в коде.
Исправить это со стороны процесса не получится, т.к. причины проблемы не организационные. Но есть способы облегчить ситуацию:
Сеньору целенаправленно выделять время для работы с миддлами. Принять, что это теперь его основная обязанность.
Миддлам обсуждать с сеньором реализацию до написания кода, а не после.
Дробить работу на задачи меньшего размера.
Сеньору прокачивать навыки ненасильственного общения.
Не играть в "бокс по переписке" в комментариях к ревью. Любой диалог длиннее двух реплик должен проходить в формате в очной встречи или созвона.
Автоматизировать линтерами всё, что можно. Настроить прекоммит-хук, чтобы неправильно отформатированный код в принципе не мог попасть в ветку. Не тратить время на проверку code style на ревью.
Писать юнит-тесты и настроить CI/CD на каждый пуш. Приступать к ревью только по факту успешного прохождения тестов.
Отдельный разговор про тестирование — ведь задачу могут вернуть на доработку, что повлечёт за собой новый цикл code review, и так по несколько раз. Но это уже совсем другая история.
А сталкивались ли в peer review с тем, что ревьюер на самом деле не понимает код, но боится/стесняется показаться глупым и поэтому делает апрув, не разобравшись в написанном?
Я не могу однозначно ответить «нет», так как не знаю, что происходит в голове у человека, но мне на такое не жаловались.
Цель code review — убедиться, что написанный код понятен другим участникам команды. Если код непонятен, а вопросы не задаются из страха перед коллегами — это тревожный симптом, который может являться частью большей проблемы.
Что значит мидл не дотягивает до уровня проекта?)))
Если уровень проекта хороший, то там и джун разберется.
Так уж и пишите, ацкоей калище и даже мидлы не тащат) Приходится "сеньору" объяснять, что он там накропал.
Code review: почему мы до сих пор его используем и какие альтернативы?