🏆 Рейтинг ІТ-работодателей 2019: уже собрано более 5000 анкет. Оцените свою компанию!
×Закрыть

Code Review, как это фактически происходит?

Доброго дня, уважаемые гуру разработки.
Подскажите пожалуйста что обычно подразумевают под Code Review? Это некоторые собрания вживую когда написавший код рассказывает что он делал а другие задают вопросы или нечто другое? смотрел публикации на хабре и других ресурсах по этой теме, но там все очень обобщенно и вскользь. с кучей хвалебных отзывов но не раскрывают фактическую суть понятия. Знакомый с курсов говорит, что у них Code Review, это когда кто-то из менторов/преподователей через онлайн смотрит твой код и дает по ходу просмотра коментарии. Какая из трактовок наиболее правильная?

LinkedIn

Лучшие комментарии пропустить

Знакомый с курсов говорит, что у них Code Review, это когда кто-то из менторов/преподователей через онлайн смотрит твой код и дает по ходу просмотра коментарии.
Это ближе всего.

У нас используется www.reviewboard.org как часть процесса, никуда ничего нельзя закоммитить без code review. В процессе code review нужно получить минимум два ’Ship It’ от двух разных людей одного профиля для того, чтобы закоммитить свой код.

Из плюсов — довольно таки много ньюансов всплывает во время добросовестного code review и это очень даже здраво. Второй плюс — практически нереально протащить разные хаки, бекдоры, закладки и т.п.

Из минусов — если код сложный и твоя команда занята чем-то срочным, то ты хрен закоммитишь свой код. Второй минус — это наличие ’ship it-guy’ в офисе, которые жмут ship it не читая код, с такими клиентами приходится бороться и не всегда успешно. Третий минус — возможен сговор трёх (иногда более) людей, которые делают «code review» друг друга в основном тоже не читая код, пропуская откровенную лажу. Когда терпение лопается, то начинаешь оставлять замечания и открывать баги в JIRA для каждой закомиченной лажи. Два раза в год проводится внешний аудит по процессу и такие лажи всплывают, вот только за них сильно не бьют, а зря. Возможно не будет повышения зарплаты или премии. Со вторым и третьим минусом борятся в основном изгнанием из команды, если не доходит по-хорошему. Четвертый минус — это отнимает много времени, поэтому часто выставляется не маленький кусочек кода для просмотра, а большой законченный кусок, для которого, чтобы посмотреть и понять его, нужно пол-часа — час времени, которые изымаются из рабочего времени сотрудников. Правило простое, жертвуй своим временем ради других и другие сделают для тебя тоже самое.

Тем не менее даже несколько плюсов покрывают все минусы в конечном счёте. Самое эффективное количество человек, которые могут делать ревью (фактически это одна команда разработчиков) — это около 8-16 человек. Если меньше, ревью просто не будет работать как надо, если больше, то это уже анархия.

Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Допустимые теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter

10 lines of code = 10 issues
500 lines of code = “looks fine”

© twitter

До первого настоящего теста.

Чтоб не создавать новую темку, спрошу тут.

В связке Visual Studio + TFS есть 2 подхода когда ревьювить изменения: до чекина или после.

Код-ревью до чекина vs код-ревью после чекина.

Какие плюсы/минусы каждого из подходов?
Когда у вас происходит код-ревью — до чекина изменений в source control или после?

«чекин» — это когда изменения заливаются куда-то, так?
если используете ветки — лучше уже после. так не блокается разработка. стабильность не затрагивается — и правки по код ревью можно внести без проблем, в зависимости от VCS можно даже склеить вместе правки по спорным моментом вместе с самими изменениями.
если без ветки, как в SVN обычно делают — что пришло, то навсегда — тогда лучше до чек-ина, безопаснее, стабильнее.
как там у вас в TFS работа ведется — я фиг знает

Какие плюсы/минусы каждого из подходов?

До чекина: основной случай — уже более-менее устоявшегося проекта/компонента, с несколькими участниками. Ревью обеспечивает контроль явных глупостей и ознакомление остальных с движением по компоненту. Другой важный случай — контроль новичка (тогда контролируют менторы).

После чекина: проект с одним активным участником и/или в стадии активной начальной разработки, когда всё перекраивается по несколько раз (обычно это одно и то же); разработка в субветке, которая будет анализироваться по итогам работы.

У нас есть Phabricator, который делает рассылку команде когда делается коммит.
Отвественный за код ревью просматривает коммит, делает замечания, принимает или реджектит коммит, пишет коментарии.
Потом ты устраняешь замечания, отвечаешь на комментарии и т.д.
Есть интеграция с JIRA, куда пишеться история коммитов к каждой таске.

> кто-то из менторов/преподователей через онлайн смотрит твой код и дает по ходу просмотра коментарии

было именно так. Расовый ирландец на общей сходке через циско-конфу давал указявки по форматированию кода и по внедрению всяких полезных и не очень параной.
Так же было, что код смотрел тимлид или назначеный тимлидом коллега в рамках официального и запланиронного на ревью в спринте времени.

Это некоторые собрания вживую когда написавший код рассказывает что он делал а другие задают вопросы
Нет, это называется «защита дипломной работы». Имею опыт 3 раза. А code review — это когда другой программист (скорей всего более опытный) смотрит твой код и оставляет замечания/комментарии.

працюєм із github
присутні:
— автоматичний код рев’ю (code sniffers, linters, security scanners) — все це робить CI сервер
— ручний рев’ю — Pull Request відправляється іншому розробнику команди на перегляд
— QA — де вичисляються функціональні баги
Якщо три компоненти виконано — код мержиться в master гілку

Ревьювим с помощью Crucible. Каждый коммит добавляется как итерация, ревьюверы пишут замечания, автор кода исправляет или не исправляет. В случае конфликта по поводу необходимости исправления зовется модератор который принимает решение. Каждое ревью имеет двух ревьюверов, модератор один на все ревью назначается на месяц, потом все по кругу.

Тоже пользуемся fisheye/crucible. Правда нет таких жестких правил :)

В последние 2 мес полностю перешел на гитхаб. Любой код чендж от левых людей через пул реквест. Там можно проглянуть код и принять, зареджектить. Удобно. Правда иногда не хватает возможности вычекаутить код себе и просмотреть в ИДЕ.

Лол. Это очевидно я так и делаю. Но не удобно. Должно быть в 1 клик.

самое оптимальное:
1. ревьювер смотрит изменения сам, в удобное для него время, и составляет список своих мыслей и отзывов
2. разработчик получает этот список, изучает его, вносит правки
3. «личная» встреча только по необходимости — если что разработчику не понятно или он не согласен с мнением коллеги.

еще частое заблуждение, но почему то это процветает в укр компаниях. откуда то пришла идея, что код-ревью должен делать человек с большей синьорити. это полная фигня. для код-ревью не важно синьорити, опыт и так далее. у него совсем другие цели.

Верно. Еще иногда полезно привлекать на ревью сторонних людей не из команды или не работавших с этим кодом вообще. Хороший тест на читабельность (говно)кода.

еще частое заблуждение, но почему то это процветает в укр компаниях. откуда то пришла идея, что код-ревью должен делать человек с большей синьорити. это полная фигня. для код-ревью не важно синьорити, опыт и так далее.

А можно чуть более детальную аргументацию, почему фигня?

Сеньоров на всех не хватит. Конечно, сеньорность предпочтительней и хорошо, когда в компании (команде) одни сеньоры и люди с опытом.

Сеньоров на всех не хватит.

Так никто вроде не требовал, чтобы ревьюили именно они.

потому что у код ревью три цели:
— увеличить знание кода в команде
— улучшить читаемость кода
— обучение

как по мне во всех трех этих пунктах — синьоры ничем не отличаются от не-синьоров. или вы так не считаете?

— улучшить читаемость кода
вот тут не согласен. цель стоит «улучшить качество кода» и только более опытный чувак может дать правильные корректировки

То что я написал это общепринятая практика. Так что полимизировать по данному вопросу нужно точно не со мной.
Код должен быть читаемым как для «опытного» так и не «опытного». Толку от того кода, если его только «супер пупер гуру» могут понять. Кому он такой нужен?

То что я написал это общепринятая практика.

Доказательство общепринятости — в студию.

Так что полимизировать по данному вопросу нужно точно не со мной.

Простите, «полемизировать» или что? Я вынужден уточнить, потому что уже не понятно, а не о полимеризации ли речь. :) Слишком уж странные, как для меня, пишете вещи.

Код должен быть читаемым как для «опытного» так и не «опытного». Толку от того кода, если его только «супер пупер гуру» могут понять. Кому он такой нужен?

А из какого рукава вдруг взялся туз этого логического перехода от простого участия более опытного коллеги (с «большей синьорити», как Вы раньше говорили), заметим, не единственного ревьюера! — до того, что код смогут понять только супергуру?

полемика о полимерах особенно доставила =)

Главная цель инспекции кода — это обнаружение ошибок. Она позволяет не только найти ошибки, которые трудно обнаружить при обычном тестировании, но и значительно уменьшить затраты на разработку (минимум на 20%).

Возможно у вас в команде именно так. Обычно это совсем не так. Искать чужие ошибки довольно трудоемкий, а главное не эффетктивный процесс. Для тестирования есть другие приемы. Плюс то что вы предлагаете — скорее всего будет вести к трениям в коллективе.

Это статистика. Ни одна методика тестирования не способна обнаружить все дефекты. Поэтому нужно использовать несколько. А инспекция кода, согласно статистике, наиболее эффективна (до 60% дефектов). Также, согласно статистике, инспекция кода существенно сокращает затраты на разработку.

Это не я предлагаю. Это известные вещи. Если у вас это неэффективный процесс, значит что-то не так делаете.

Очень часто я обнаруживаю причину ошибки путем просмотра кода, и это часто значительно быстрее, чем искать отладчиком.

Кем собрана и где опубликована эта статистика?
Обычно после фразы — "

неэффективный процесс
" или "
эффективный процесс
" мне человек перестает быть интересен. Потому что мнение менеджеров меня не интересует.

Зачем переходить на личности? О неэффективном процессе вообще то Вы писали. А я всего лишь описал известные вещи, и даже поддержал один Ваш коммент...

А статистика из книги «Совершенный код».

Плюс то что вы предлагаете — скорее всего будет вести к трениям в коллективе.

Простите, если обнаружение ошибок будет вести к трениям в коллективе и это считается аргументом против ревью (а как иначе Вас понять?), то нафиг вообще тот коллектив и та разработка? Давайте лучше всех вывезем на природу с травкой и устраним все трения...

Давайте лучше всех вывезем на природу с травкой и устраним все трения...

Вы были бы идеальным тимлидом...

Мне кажется, для второго пункта синьерность-таки нужна, так как неопытный разработчик не увидит в чужом коде проблем с читабельностью, так как сам еще не постиг дао совершенного кода.

Но есть еще один фактор, для которого важна синьорность: это выявление ошибок в коде. Опыт индустрии говорит, что в процессе качественного code review обнаруживается приличный процент багов.

читабельность она для всех. независимо от синьориты, цвета кожи, религии и так далее.
читаемый код — понятен всем.

И как это сочетается с dou.ua/...es/pasta-decoding/#645899 ?

еще частое заблуждение, но почему то это процветает в укр компаниях. откуда то пришла идея, что код-ревью должен делать человек с большей синьорити. это полная фигня. для код-ревью не важно синьорити, опыт и так далее. у него совсем другие цели.

Это не фигня. Должен быть и более опытный участник, и менее опытные. Более опытный подскажет неожиданные грабли, которые автор кода не знает, а менее опытные будут учиться на примерах и особенно на обсуждениях. Одно замечание более опытного коллеги может быть ценнее десятков от коллег.

В качестве показательного примера — наиболее интересные технологические дискуссии в freebsd’шных ссылках рассылки много лет шли (по крайней мере пока я их читал) не в формально предназначенных для этого arch, hackers и так далее, а в {cvs,src}-{all,base,...} — куда роботы постят содержание коммитов, а на основе этих постингов завязываются дискуссии.

Целей у code review несколько — это
* оценка более опытными, указание на отклонения от норм, характерные грабли и т.п.;
* обучение менее опытных;
далее,
* ознакомление коллег с происходящим в зоне общей ответственности; взятие ими на себя оценки означает хотя бы грубое ознакомление с изменением;
* специализированная рабочая «социализация» в области целевого взаимодействия, с ознакомлением методами друг друга и совместным притиранием.

Хорошее построение ревью решает все эти задачи примерно в равной степени.

У нас, и мне этот подход нравится, ревью:
— проверить код
— проверить стандарты (тесты, покрытие, форматирование, и.т.д.)
— проверить как работает на спец сервере.
Да, долго, да, порой «ну я же занят», но очень полезно.
Увы, все равно не исключает ошибки и особые случаи.

Тесты (прогон), покрытие, форматирование и т. п. оценивать живому человеку очень дорого и не эффективно: «Вкалывают роботы — счастлив человек» ©

проверить — посмотреть на цифры. разумеется, сборки и подсчеты идут автоматически.

Знакомый с курсов говорит, что у них Code Review, это когда кто-то из менторов/преподователей через онлайн смотрит твой код и дает по ходу просмотра коментарии.
Это ближе всего.

У нас используется www.reviewboard.org как часть процесса, никуда ничего нельзя закоммитить без code review. В процессе code review нужно получить минимум два ’Ship It’ от двух разных людей одного профиля для того, чтобы закоммитить свой код.

Из плюсов — довольно таки много ньюансов всплывает во время добросовестного code review и это очень даже здраво. Второй плюс — практически нереально протащить разные хаки, бекдоры, закладки и т.п.

Из минусов — если код сложный и твоя команда занята чем-то срочным, то ты хрен закоммитишь свой код. Второй минус — это наличие ’ship it-guy’ в офисе, которые жмут ship it не читая код, с такими клиентами приходится бороться и не всегда успешно. Третий минус — возможен сговор трёх (иногда более) людей, которые делают «code review» друг друга в основном тоже не читая код, пропуская откровенную лажу. Когда терпение лопается, то начинаешь оставлять замечания и открывать баги в JIRA для каждой закомиченной лажи. Два раза в год проводится внешний аудит по процессу и такие лажи всплывают, вот только за них сильно не бьют, а зря. Возможно не будет повышения зарплаты или премии. Со вторым и третьим минусом борятся в основном изгнанием из команды, если не доходит по-хорошему. Четвертый минус — это отнимает много времени, поэтому часто выставляется не маленький кусочек кода для просмотра, а большой законченный кусок, для которого, чтобы посмотреть и понять его, нужно пол-часа — час времени, которые изымаются из рабочего времени сотрудников. Правило простое, жертвуй своим временем ради других и другие сделают для тебя тоже самое.

Тем не менее даже несколько плюсов покрывают все минусы в конечном счёте. Самое эффективное количество человек, которые могут делать ревью (фактически это одна команда разработчиков) — это около 8-16 человек. Если меньше, ревью просто не будет работать как надо, если больше, то это уже анархия.

Как справляетесь с ситуациями, когда нужно срочно закоммитить хотфикс,а люди которые имеют право это делать не доступны? (заболел,в на встрече з заказчиком, нет на месте). Команда при этом пол дня-день пьет чай.

так же, как если «права на перезапуск билда есть только у одного человека». например.
как и любые косяки организации, когда на ком-то одном завязано критичное что-то.
то есть, либо делегирование и разделение прав(может не только Миша, но и Петя)
либо надежда на авось(и Миша должен расшибиться, а найти во время отпуска в Катманду рабочий комп с интернетом)
либо забить и мучаться

Сравним две гипотетические ситуации:
1. Миша залил плохой код, все упало, все идут пить чай. Вася потратил 2 часа на починку, залил, все счастливы. Потери 150 человекХ2часаХрейт.
2. Петя залил плохой код(ибо у Пети права жеж), все сначала ж такие «у нас же ревью, у нас в репозитории ерунды нет». Ок, через 3 часа дружненько понимаем что проблема вот она и виноват Петя. Что бы пофиксить надо дописать строчку кода. Код дописан. Но Пети нет. И Сени тоже нет. А кроме Пети и Сени никто комитить не может. А еще случайно так как то получилось что это еще и на тестовую среду попало, и вообще все 150 человек идут пить чай весь день. Итого потери 8×150хрейт. + овертаймы если этот день надо срочно доганять.
Вопрос, а оно того стоит? я что то себе чем дальше, тем чаще задаю этот вопрос. Качество кода важно, безспорно, но code review точно не silver bullet.

Но Пети нет. И Сени тоже нет.
представим ситуацию без код-ревью.
просто надо запустить новый билд.
но права у Пети и у Сени. а их нет.
КуСи печалятся и пьют чаи.
Проблема ли это применения CI как подхода и набора тулзовин в частности?
Надо ли от него отказываться?
Проблема ли это применения CI как подхода и набора тулзовин в частности?
Надо ли от него отказываться?
В какой то момент своего профессионального пути вы возможно осознаете, что программист это такая же профессия как и все остальные. По сути задача ваша такая же как и у любого другого сотрудника — достижение бизнес-целей. Не написание хорошего кода или что-то там еще, а именно бизнес цели.
И даже если мы пишем отличный код, но при этом у нас простои, разработка идет медленно и мы выбиваемся из графика значит мы плохие сотрудники. То есть да, в том числе это и проблема выбранного нами подхода или программных средств или чего-то там еще. За всеми этими review, qa, ci и прочим стоят конкретные цели,а подавляющем большинстве случаев это именно cost savings. А если пьют чаи-это не cost savings
p.s. Конечно, могут быть и неадекватные цели, но это уже больше вопрос к вам, почему вы выбрали такого работодателя.
По сути задача ваша такая же как и у любого другого сотрудника — достижение бизнес-целей.
Есть разработка софта на заказ фактически для одного заказчика и есть продуктовая разработка. Бизнес цель одна — срубить бабла, вот только в продуктовых компаниях, у которых десятки тысяч, а то и миллионы клиентов достигается разными подходами. Когда мы пилим новую killer фичу в продукте, мы не экономим средства (в пределах разумного, конечно), когда же пилим киллер фичу в заказной разработке, то тут желание по быстрее написать и забыть и переключиться на слудующую.

У обычных сотрудников коммерческой организации, хоть программистов, хоть бухгалтеров, хоть уборщиц нет задач достижения бизнес-цели — это задачи менеджмента. Менеджмент ставит им задачи их предметной области, которые помогут (по мнению этого менеджмента), достигать бизнес-цели. Программисту ставят задачу «реализовать фичу (которая увеличит конверсию)», а не «увеличить конверсию (путем реализации фичи)», уборщице ставят задачу «в помещении программистов должно быть чисто (чтобы программисты работали эффективнее)», а не «улучшить эффективность работы программистов (путём уборки)». Задачи увеличения конверсии и эффективности работы обычных сотрудников решает менеджмент. Обычные сотрудники должны решать задачи своей предметной области согласно принятым (часто неявно) в организации стандартам на качество и скорость работы, при этом они могут не знать каких бизнес-целей достигают своим трудом, а в особо тяжелых случаях могут быть сознательно введены руководством в заблуждении (скажем, в аутсорсе, истинная бизнес-задача фирмы, в которой работает программист, может быть «подсадить на иглу» заказчика, а ему говорят «мы решаем такие-то бизнес-задачи заказчика»).

Чем лучше вы понимаете что клиенту(или боссу) реально надо-тем более вы незаменимый сотрудник. Смотрите за

задачи их предметной области, которые помогут (по мнению этого менеджмента), достигать бизнес-цели.

А кто сказал, что цель — стать незаменимым сотрудником? Тем более, что для бизнеса это повышенные риски.

По сути задача ваша такая же как и у любого другого сотрудника — достижение бизнес-целей.
оу. тут я согласен.
только бизнес-цель разработчика будет отличаться от бизнес-цели заказчика.
иллюстрация: овертаймить бесплатно(без выиграша в деньгах) на скучном(без развития) проекте — играть на бизнес-цели собственника проекта. но ничего не делать для своих целей. именно бизнес-целей(у каждого свое — получать много денег за мало времени, вырасти до вершин убер-специалиста, наработать связи для создания собственной компании), а не "просто целей жизни"(семья, дом, уют, etc)
с этой точки зрения, даже корявые процессы могут играть против бизнес-целей собственника проекта, но играть на руку разработчику

Хорошо подмечено про разные цели. В идеале программист, решая поставленную техническую задачу, достигает сам или способствует достижению множеством других лиц от непосредственного начальника до пользователей продукта и государства огромного множества личных, коммерческих и общественных целей. Часть из них достигается самим фактом, что он написал нужный код вовремя и качественно, часть — использованием результатов работы этого кода.

Просто отдавать на ревью нужно работающий код (хоть может быть и хреново написанный). Я не выставляю код на ревью, пока сам не потестирую, не отошлю заказчику экспериментальный билд и не получу от него одобрение.

Как это не смешно звучит, но в рамках SOA не всегда есть возможность досконально проверить как код себя поведет за пределами машины разработчика. Начинаю осознавать что мои проблемы довольно далеко от большинства других разработчиков :)

А теперь представьте, что делаете драйвер, например, для SGX GPU, существует три десятка различных ip core и более пяти сотен различных устройств, которые используют один из трёх десятков ip core. Практически любое изменение способно поломать драйвер для трети устройств и пофиксить что-то другое для двух третьих других устройств. Протестировать это всё просто нереально.

Слабо понимаю о чем вы, но проблема в целом ясна. И как решается? Street magic?

Ну он же сказал — владелец конкретного устройства должен подтвердить работоспособность.

Мне больше интересно, что будет сделано, если такое подтверждение окажется ложным (исполнитель владельца, например, отнёсся к проверке халатно и формально). Если неверный код попадает в общий бранч?

Мне почему-то кажется, тест драйверов должен хорошо поддаваться автоматизации, и все что нужно исполнителю — выслать лог.

Если бы было всё так просто... Думается, что QA, который предложит хорошее решение просто озолотится.

Если неверный код попадает в общий бранч?
Если будет много шума, то обнесут 100500ым #ifdef до полного разбирательства, если шума нет, то значит и проблемы нет. Для этого и существует консорциум, где не одна компания, а несколько десятков делают ревью коду и проводят тестирование.

Да, фактически каждый владелец устройств должен подтвердить, что всё хорошо. Есть обычно две ветки скучная stable и experimental, которая может содержать проблемы. Например, а автомобилях применяется stable ибо на сервис за обновлением мало кто часто ездит, в телефонах можно и экспериментальную засунуть, чтобы была бестрее, больше маркетинговых плюшек, а стабильность — по выбору производителя.

Если Revert плохого коммита не требует аппрува Пети или Сени, тогда проблемы нет. Если автобилд сломался — коммит откатывается и 150 человек продолжают работать дальше.

Сравним две гипотетические ситуации:
1. Миша залил плохой код,

Сначала CI отказался пропускать этот код, потому что упали автоматические тесты. В течение двух дней Мишу пинали все ревьюеры со словами «у тебя молоко убежало тест не проходит, чини или отменяй своё предложение». Наконец он воткнул версию, которая прошла тесты, ревьюеры подключились и начали его пинать. Только 15-й патчсет прошёл одобрение всех ревьюеров и попал... правильно, не в master, а в devel. Из которого до master надо ещё пройти два cherry-pick: сначала в testing, только потом в master. Альтернативно — да, он попал в master, но до пользователей не дошёл, потому что им с утра экспортируется ветка release-52-15, которую отфоркнули на основании предыдущего release engineering цикла.

все упало, все идут пить чай.

Не упало, потому что код Миши никуда не пришёл.

2. Петя залил плохой код(ибо у Пети права жеж), все сначала ж такие «у нас же ревью, у нас в репозитории ерунды нет». Ок, через 3 часа дружненько понимаем что проблема вот она и виноват Петя. Что бы пофиксить надо дописать строчку кода. Код дописан. Но Пети нет. И Сени тоже нет. А кроме Пети и Сени никто комитить не может.

Сначала умудрились пропустить плохой код через ревью (которого не было?), потом потеряли возможность закоммитить вовремя... ну опять-таки вопрос в DevOps и в стиле release engineering. Потому что если закоммиченное немедленно попадает к клиентам, то должен быть метод и исправления посреди ночи (как именно он сделан — уже административный вопрос; обычно должен быть какой-то дежурный разработчик). Если есть посредники в виде QA+DevOps — это их задача откатить или исправить на месте.

ачество кода важно, безспорно, но code review точно не silver bullet.

По-моему, никто не настаивал, что оно будет панацеей от всех проблем, включая паршивое администрирование процессов разработки. Но моя практика показывает, что оно существенно помогает избавиться от множества ляпов, которые видны другим, но не автору, независимо от квалификации этого самого автора.

Вашу мысль я понял, к сожалению, в силу ряда объективных и субъективных причин у нас процесс построен не так и так, как вы описываете, его построить не представляется реальным.

Он может быть построен множеством методов, но каждый из них должен быть внутренне согласован. Ну а если это не так... тогда, действительно, «не кран надо менять, а всю систему».

Заливается фикс и создается пост-коммит ривью

Нас в команде дюжина, в принципе любой может заменить любого, ревью имеют право делать все программеры в команде, и уж три человека обязательно будут на месте из 12. Если изменение очень срочное, то два наименее загруженных человека для ревью выделяются тимлидом в обязательном порядке.

обычно у джунов, новых разработчиков в команде и всех остальных говнокодеров забирают права на коммит. Криворукий пушит коммит, но он не применяется, пока кто-то из серьезных дядек не проревьювит его и не нажмет кнопочку Reject | Approve. Этот механизм борьбы с потенциальными багами называется — пул реквестов. Занимает эта гадость у серьезных дядек кучу времени и если разработчик не растет в профессиональном направлении, то, как правило, через несколько месяцев может оказаться на улице. Жестоко, но справедливо. Оттак от.

называется — пул реквестов
скорее pull request — запрос/просьба вытянуть твои изменения себе

Pull request в общий бранч просто удобный момент для code review, по-этому часто термины используются взаимозаменяемо.

Тогда не «пул реквестов», а «реквест на пулл» :)

И так, и так не по-русски

собрания вживую когда написавший код рассказывает что он делал
это митинг (совещание) --- просто каждый рассказывает в общих чертах, чем занимался и чем планирует заниматься. цель: синхронизировать командно-мыслительные процессы, чтобы было общее и единое понимание движения проекта
когда кто-то из менторов/преподователей через онлайн смотрит твой код и дает по ходу просмотра коментарии
вот это и есть код ревью --- т.к. кто-то другой (необязательно начальник; в некоторых компаниях практикуется одноранговая модель, когда коллеги ревьювят друг друг) смотрит в код, делает комментарии/замечания/вопросы. цель: получить дополнительную пару глаз и дополнительные идеи

еще моменты:
1. грань между онлайн и вживую стёрта. совещание можно провести в чяте, а ревьювить код сидя в двоем рядом (см. например парное программирование)
2. код ревью --- это идея, а не строго зафиксированная технология; а потому каждый внедряет ее как хочет (например, начальник просматривает каждый патч в ветке, а потом мержит (объединяет) с общим кодом; либо патч должны просмотреть не менее двух коллег и все должны дать добро на его применение)

в чяте
Полностью согласен. Только в чате.

Есть ещё вариант, что если проект большой, то код-ревью делает не только ментор или коллеги, но и ответственный за подсистему, которая затронута веткой.

В разных компаниях разные правила игры. Где-то без code review код вообще не может попасть в мастер, где-то .-. и в продакшн.

Я обычно смотрю код (делаю Code Review) если
1. человек только входит в проект
2. ранее не работал с технологией и только начинает ее использовать
3. коммит затрагивает какие-то критичные для всего проекта классы

В кратце: кто то как то смотрит твой код.
Практическая реализация зависит от политики, выбранных программных средств и еще кучи чего.
Как по мне,практическая ценность сего несколько переоцененна.

практическая ценность сего несколько переоцененна
эм? это что значит?
без присваивания, например, тоже можно обойтись.
значит, его практическая ценность — тоже переоценена?

code review is not a silver bullet.
толпа джунов делающая ревью друг другу врядли родит что-то хорошее

толпа джунов делающая ревью друг другу врядли родит что-то хорошее
Есть что-то пошлое в этой фразе :)

то есть, в случае жопы не спасет?
в таком случае, практическая ценность программирования переоценена точно так же.
есть пример, когда код ревью мешает обмену опытом?
когда ухудшает качество кода? в крайнем случае, как мне кажется, качество кода не меняется — когда все просто так, не глядя, подтверждают

Лично мое скромное мнение, что при достаточном уровне специалистов и прозначными coding conventions ревью не дает ожидаемого выхлопа. Если специалисты не очень, тогда да, возможно с точки зрения качества кода имеет смысл

А вот это и поможет проверить, какие из джунов уже достаточно выросли.

Ожидания: поскольку в случае моей компании неудачный коммит может заблокировать работу ~100 человек многоэтапные кодревью обязательны практически для всех. Во избежание
Практически: Для некоторых они все таки не обязательны. Или за неимением времени ревью может быть чисто формальным. И вообще все люди и учесть всего невозможно. No silver bullet. Ситуации с блокировкой процесса случаются с достаточной регулярностью, а скорость разработки тормозится очень даже. Ибо не всегда есть время на ревью, а без него нельзя.
Ревью специалистами одного уровня(при достаточном уровне оных) бессмысленная трата времени на большом проекте. Элементарные ошибки вылавливаются крайне редко, а архитектурного характера ошибки без знания картины в целом пропускаются.

No silver bullet
не, если с точки зрения «code review не заменит квалификацию», то тут я согласен. но я и не говорю, что можно разработать проект, если десяток обезьянок будут хреначить по клавишам, а один спец — ревьювить.
Или за неимением времени ревью может быть чисто формальным
Ситуации с блокировкой процесса случаются с достаточной регулярностью
как оно у вас случается-то одновременно?
---
1. у всех разный уровень знаний в технологии. никогда не встречал братьев-близнецов, чтоб на половине строчки второй мог продолжить один-в-один. очень вероятно. критично.
2. у всех разный набор знаний о проекте. и ситуация «о, такой код я встречал в хелпере А, что вчера зафигачили» более чем вероятен. вероятно. критично.
3. человеческая лень — не зависит от уровня компетенции. сам факт, что кто-то, да будет ревьювить может сдержать от «ну, сделаю так, а позже исправлю — работает же». или не сдержит. но есть шанс, что костыль не пройдет незамеченным. маловероятно, не особо критично.
4 . вопрос читаемости. как раз не-автор вероятнее, что скажет «слушай, а что это за хрень непонятная? может, комментов добавить?». маловероятно, но критично.
5. и последнее, но не по значению — знакомство с проектом. как ни организовывай работу, всегда будет так, что один пилил функционал А, а теперь в нем надо сделать дополнительно изменения, а другой разработчик впервые видит. если не забивать на код-ревью, все имеют шанс получить «новости проекта» довольно пассивным способом.
---
решает ли это все возможные проблемы? нет, не говорил я такого.
важно ли это? как на меня — очень даже.
что с этим всем делать, если убрать код-ревью?
неудачный коммит может заблокировать работу ~100 человек
а как же автотесты?
в моем понимании, код ревью это про другое. про структуру кода, про костыли, про понятность, про структуру и алгоритмические решения.
а как же автотесты?
Прошли. Скажите плохие тесты? Возможно. Но далеко не все возможно покрыть автотестами. Заблокировало работу 100 разработчиков имелось в виду.
Масштаб решения таков, что отловить все невозможно. Вы просто не обладаете всеми знаниями. Тоже самое к п.1,2,5 вашего ответа. В большинстве случаев подвергнуть ревью именно логику решения сложно, только code style.
как оно у вас случается-то одновременно?
Внезапно берет и случается.
1. у всех разный уровень знаний в технологии. никогда не встречал братьев-близнецов, чтоб на половине строчки второй мог продолжить один-в-один. очень вероятно. критично.
Двух одинаковых людей не бывает или все одинаковые, в зависимости от ваших убеждений.
3. человеческая лень — не зависит от уровня компетенции.
Не все объяснимо ленью. Внешние обстоятельства могут не позволять уделять процессу ревью достаточно времени.
4 . вопрос читаемости. как раз не-автор вероятнее, что скажет «слушай, а что это за хрень непонятная? может, комментов добавить?». маловероятно, но критично.
Вы только на собеседованиях такого не говорите, ок? Если код не понятен без комментариев-это плохой код. Коментарии в 99.9% не приводят в соответствии с изменениями и потом они только путают и вводят в заблуждение. Если код нуждается в комментариях, потому что он не понятен-его необходимо рефакторить. Никаких комментариев подобного рода.
что с этим всем делать, если убрать код-ревью?
Все зависит от конкретных нюансов. Я всего лишь о том говорю, что конкретно в моем случае код ревью не оправдывает возлагаемых на него ожиданий. Вы ставите вопрос о code style, меня же интересует больше вопрос поддержания целостности сложного масштабного решения в любой момент разработки.
Но далеко не все возможно покрыть автотестами.
так не бывает.
бывает что «покрывать всё долго», «покрывать все дорого», «покрывать всё — и мы не успеем». то есть «не целесообразно».
надеюсь, вы не планировали сказать, что «автотесты — это не серебряная пуля»?
Заблокировало работу 100 разработчиков имелось в виду.
btw, а что мешает откатить такой поломанный коммит? вне зависимости от результатов автотестов. неужели, нерабочая система — это лучше, чем откат коммита?
В большинстве случаев подвергнуть ревью именно логику решения сложно, только code style.
в какой это технологии из кода не видно логики?
Если код не понятен без комментариев-это плохой код.
хм. пример.
code.jquery.com/jquery-1.11.3.js
сделайте поиск «workaround» по комментам кода.
предположите хоть одно место, где надо выкинуть код и сделать рефакторинг.
Вы только на собеседованиях такого не говорите, ок?
если на собеседовании мне скажут, что комментарии не нужны, мы все равно не сработаемся. так что норм.
btw, а что мешает откатить такой поломанный коммит?
Прав нет и религия не позволяет.
так не бывает.
Ок, нельзя = тесты не имеют смысла, так как целиком зависят от данных, которые меняются постоянно. Поддержание же их в рабочем состоянии будет сложнее ручного тестирования каждый раз. А если не поддерживать-тесты просто не несут в себе полезной нагрузки.
Очевидно,что тут вопрос еще и о том, что именно вы тестируете.
в какой это технологии из кода не видно логики?
пример.
Ок, объясняю. Вы смотрите с точки зрения front-end. Это одно, javascript он такой... Отрефакторить то можно, но такой рефакторинг будет не очень с точки зрения js.
Теперь припустим вы в back-end. Вам на ревью приходит следующее.
1. Получить данные от сервисов A,B,C.
2. Обработать результат, составить запрос к сервису D.
3. Запросить повторно от данные от сервиса А.
4. Отправить запрос к сервигу G.
5. Profit.
Код правильный, все красиво. Вот только на шаге 1 надо было запросить данные от F,B,C. И на шаге 3 надо бы еще данные повторно в сервис K отправить, иначе в другом не связанном куске кода у Васи(код которого вообще не у нас на ревью) ничего не заработает. Но не зная всей бизнес-логики вы эту ошибку не найдете. А разбираться на каждом ревью в бизнес-логике подобных задач — никакого времени не хватит.
если на собеседовании мне скажут, что комментарии не нужны, мы все равно не сработаемся. так что норм.
На Java, C#, C++, Python и т.п. хороший код должен быть понятен без комментариев. Язык это позволяет. Когда я говорил в первом своем ответе что "
Практическая реализация зависит от политики, выбранных программных средств и еще кучи чего.
" я и это,в том числе, имел в виду.
Отрефакторить то можно, но такой рефакторинг будет не очень с точки зрения js.
ага, язык виноват. давайте, посмотрим на java.
возьмем багтрекер bugs.java.com
поищем, например, баги связанные с отработкой строк. например, вот:
bugs.java.com/...iew_bug.do?bug_id=8020986
JDK-8020986 : Space or \\s at the end of Regular Expression fails
как будет выглядеть попытка обойти эту проблему? насколько она(попытка-то) будет очевидна?
---
или Spring framework. Не может же там быть не очевидных воркэраундов, правда? (на самом деле, может)
---
может, все дело в воркэраундах?
окей. надуманный пример.
наверняка, вы в курсе, что у разных алгоритмов сортировки разные характеристики. Ну, что кроме скорости есть потребление памяти(порядок и размерность), устойчивость и тыды.
Вот, представляете, в определенном куске кода реализуется конкретный алгоритм сортировки.
Без комментариев, так как код же должен быть и так понятен, да?
а потом приходит другой человек. новый. того, кто писал код — нету уже на проекте.
как понять — обоснован ли выбор алгоритма? по имени переменной?
Код правильный, все красиво. Вот только на шаге 1 надо было запросить данные от F,B,C.
вы сами себе противоречите. код не правильный, если надо было реализовать флоу А, а реализовали некое В. и да, рассчитывать, что это будет отловлено на этапе код-ревью странно. и я такого не предлагал — заменять тестирование на соответствие требованиям просмотром кода.
поищем, например, баги связанные с отработкой строк. например, вот:
Это не показательный пример.
или Spring framework. Не может же там быть не очевидных воркэраундов, правда?
Может. И там можно было обойтись без комментариев. Еще один метод
private String getWorkarroundWeblogis10_3(string fullAddres){..
вы сами себе противоречите. код не правильный, если надо было реализовать флоу А, а реализовали некое В. и да, рассчитывать, что это будет отловлено на этапе код-ревью странно. и я такого не предлагал — заменять тестирование на соответствие требованиям просмотром кода.
.
Вы не предполагаете, что возможна ситуация когда вы работаете в условиях отсутствия всей необходимой информации? Требования сформулированы расплывчато, или неактуальны или не правильны в целом. Тем не менее, задачу delivery в срок никто не отменяет...
Это не показательный пример.
«не показательный» — в каком смысле? нет идеальных языков, нет идеальных компиляторов, идеальных фреймворков и даже архитектур(что вообще суть — абстракция). значит, в принципе, всегда будет возникать необходимость в неочевидных решениях. которые надо объяснять.
отказываетесь от комментариев? самодокументируемый код? да, пожалуйста.
но я в таком мире жить не хочу.
Может. И там можно было обойтись без комментариев. Еще один метод
private String getWorkarroundWeblogis10_3(string fullAddres){..
ага, который нифига не поясняет, что именно обходит этот воркэраунд. и почему оно вообще работает. соответственно, превращается в священную корову «я не знаю, почему, но оно работает», не доступную для рефакторинга, изменения или даже переноса в другое место — вдруг, сломается?
Требования сформулированы расплывчато, или неактуальны или не правильны в целом. Тем не менее, задачу delivery в срок никто не отменяет...
иииии? то есть, вы такое тестировать не станете в принципе, так как нет требований и не понятно, что должно получаться в результате каких шагов. какое ж это деливери?
олсо, вернемся к вопросу — как проблема требований вообще связано с целью код-ревью?
код-ревью может ли решить проблему отсутствия требования? уверен, что нет. а вы?
может ли код-ревью быть заменой функциональному тестированию? уверен, что нет. а вы?
значит ли это, что код-ревью бесполезно, если оно не решает эти проблемы? как на меня, даже вопрос звучит глупо.
«не показательный» — в каком смысле?
В том смысле что это багрепорт. Естественно, если что без комментариев там никак.
значит, в принципе, всегда будет возникать необходимость в неочевидных решениях. которые надо объяснять.
Где грань между неочевидным решением и костылем? Пример с weblogic плохо пахнет. К примеру, завтра выйдет версия 10.4 где нужен будет другой workarround? Вместо того что бы реализовать стратегию, мы по вашему допишем еще операторов ветвления и кучу комментариев.
отказываетесь от комментариев? самодокументируемый код? да, пожалуйста.
но я в таком мире жить не хочу.
Я отказываюсь от плохого кода с комментариями оправдывающими оный.
ага, который нифига не поясняет, что именно обходит этот воркэраунд.
Оригинальный пример этого не поясняет. Там просто идет констатация факта что это workarround.
соответственно, превращается в священную корову «я не знаю, почему, но оно работает», не доступную для рефакторинга, изменения или даже переноса в другое место — вдруг, сломается?
Unit test как средство документирования. Во первых, сразу очевидно как кусок кода должен был работать, во вторых, всегда легко проверить не сломали ли мы чего-то.
Unit test как средство документирования.
мы все еще обсуждаем воркэраунды для специфических проблем? как можно написать unit test на то, что на определенном окружении код действительно отработает, как надо?
Оригинальный пример этого не поясняет. Там просто идет констатация факта что это workarround.
окей. то есть, вы уже подошли к тому, что комментарий можно было бы написать лучше.
не делать сноски в непредсказуемых местах документации, а написать понятный комментарий в конкретном участке кода.
Вместо того что бы реализовать стратегию, мы по вашему допишем еще операторов ветвления и кучу комментариев.
а если у того ж weblogic 10.3 есть несколько проблем, сделаем фабрику стратегий?
а если у того ж weblogic 10.3 есть несколько проблем, сделаем фабрику стратегий?
Вы сами предложили. Альтернативы? Спагетти-код с кучей комментариев?
окей. то есть, вы уже подошли к тому, что комментарий можно было бы написать лучше.
Я подошел к тому, что вами приведен пример кода который дурно пахнет.
мы все еще обсуждаем воркэраунды для специфических проблем? как можно написать unit test на то, что на определенном окружении код действительно отработает, как надо?
Это вообще не задача unit test’ов. Мы опишем лишь то, как мы ожидаем что код отработает. Соответственно, если данные тесты не пройдут мы можем утверждать что наш код не работает так, как мы того ожидали.
Вопросами работы кода в окружении занимаются другие виды тестирования.
Вы сами предложили. Альтернативы? Спагетти-код с кучей комментариев?

Ваша альтернатива оказывается спагетти-фабрикой. Это хуже, чем спагетти-код, за счёт лишнего уровня абстракции.

Вопросами работы кода в окружении занимаются другие виды тестирования.

Вы сами перед этим предложили юнит-тесты для решения обсуждаемых проблем. В котором из этих сообщений, по-вашему, вы ошиблись?

Я отказываюсь от плохого кода с комментариями оправдывающими оный.

По-моему, этого никто не предлагал.
А ещё заранее обычно непонятно, какой код станет плохим через год.

Unit test как средство документирования. Во первых, сразу очевидно как кусок кода должен был работать, во вторых, всегда легко проверить не сломали ли мы чего-то.

По стандартному определению unit test — это функциональный тест мельчайшего доступного элемента кода (функции, метода). В таком случае он неспособен, например, отловить кубическую зависимость от значения некоторого параметра вместо требуемой линейной. Если же мы говорим о статистических характеристиках, то тут не сработает даже тысяча юнит-тестов.

На Java, C#, C++, Python и т.п. хороший код должен быть понятен без комментариев. Язык это позволяет.

Никакой язык без комментариев не позволяет объяснить вещи типа «мы тут применили алгоритм Мбонга, а не метод Зенона-Пупкина, потому что алгоритм Мбонга имеет квадратичную зависимость вместо кубической от X и линейную от Y; а его факториальная зависимость от Z нам не важна, потому что Z<3 по определению нашей задачи».
Вы можете сказать, что такие вещи уходят в ТЗ и внешнюю документацию, но практически их всё равно лучше держать в комментариях. Не зря придумали doxygen с аналогами — подобные обоснования прямо в исходниках удобно читать на месте, не переключаясь, а возможность оторвать из них и собрать техническую документацию логически завершают такое средство.

Аналогично, в комментариях место для всяких FIXME и TODO.

Но не зная всей бизнес-логики вы эту ошибку не найдете. А разбираться на каждом ревью в бизнес-логике подобных задач — никакого времени не хватит.

Это всего лишь говорит, что ревью не абсолют, но не о его бессмысленности.

блиииин, как тут поддержать трижды?

Вы можете сказать, что такие вещи уходят в ТЗ и внешнюю документацию, но практически их всё равно лучше держать в комментариях.
Такие вещи лучше держать в документации, так на практике поддерживать актуальность комментариев нет времени/недосуг большинству разработчиков. Не всегда комментарии понятны или адекватны.
Вдумайтесь, вы описываете некий алгоритм в широком смысле средствами языка программирования. Почему это описание, по вашему, должно сопровождаться еще какими то описаниями? Может вы просто плохо описываете?
Нет, нельзя сказать что в 100% случаев без комментариев можно обойтись, однако же обоснование их использование должно быть более внятно чем «это код непонятен без комментариев».
Не зря придумали doxygen с аналогами
Обычно подобные вещи используются когда документацию писать недосуг.

я правильно понимаю, программистам некогда/лень обновлять коменты, зато за вопросом «а почему именно такой подход?» им не лениво продираться сквозь заросли не привязанной к коду документации? еще и актуализировать её?
P.S. а как там с актуализацией имён переменных, методов, классов, файлов? или тоже — нафига, достаточно документации и так все понятно?

я правильно понимаю, программистам некогда/лень обновлять коменты, зато за вопросом «а почему именно такой подход?» им не лениво продираться сквозь заросли не привязанной к коду документации? еще и актуализировать её?
Вот потому то крайне желательно что бы единственной и достаточной документацией к коду был именно сам код. И писать его правильно. В документацию и комментарии я лично уже давно не верю.
Вот потому то крайне желательно что бы единственной и достаточной документацией к коду был именно сам код.
так. еще раз. кейс о выборе подходящего требованиям алгоритма.
как должен выглядеть самодокументируемый код без комментариев, чтоб было понятно, почему выбрали алгоритм А, а не В?

Поиграем в “жидівську кнайпу”.
Меня больше интересует вопрос зачем вообще это должно быть освещено в комментариях к коду. Это не диссертация, что бы доказывать преимущество решения 1 над всеми остальными возможными с подробными выкладками и доказательствами.
Да даже и в документации, честно говоря...
Вы обосновываете везде выбор алгоритмов, структур данных и т.д. в комментариях к коду?

Вы обосновываете везде выбор алгоритмов, структур данных и т.д. в комментариях к коду?
если я считаю, что это _может_ быть не очевидным тому, кто придет колупать этот код — да.
Поиграем в “жидівську кнайпу”.
круто. поиграем. это как? что мне должен был выдать гугл, кроме адреса кафе во Львове?
если я считаю, что это _может_ быть не очевидным тому, кто придет колупать этот код — да.
То есть вы не уверенны в своем решении и оправдываетесь? :) Как-то так получается.
Зайдем с другой стороны. Припустим, что для решения некоторого кейса вам приходиться решать TSP задачу. Или мне. И я выбираю для ее решения tabu search.Или greedy 2-OPT. Не важно, впрочем.
Вопрос, мне скопировать все 300 с гаком страниц из книги по данной проблеме(всего лишь одной из) в комментарии или считать свой выбор очевидным?
круто. поиграем. это как?
Это оборот речи популярный в определенных кругах. Означает что дальнейшая дискуссия будет напоминать традиционный еврейский торг.
Если вас это так заинтересовало, то можете почитать хотя бы про то самое заведение во Львове:
arttutorials.livejournal.com/6051.html
или просто его посетить
Вопрос, мне скопировать все 300 с гаком страниц из книги по данной проблеме(всего лишь одной из) в комментарии или считать свой выбор очевидным?

Написать одну строчку с указанием причин выбора конкретного алгоритма. Можно вторую — с указанием на книгу, где оно описано, если быстрый опрос коллег показал, что эта книга не находится у каждого на столе или хотя бы в библиотеке фирмы.

>>Меня больше интересует вопрос зачем вообще это должно быть освещено в комментариях к коду.

Хотя бы чтобы не тратить на повторение исследований иногда очень большое время другого программиста, которому показалось, что другой (уже опробованный и отброшенный алгоритм) будет в данном случае работать эффективней.

Такие вещи лучше держать в документации, так на практике поддерживать актуальность комментариев нет времени/недосуг большинству разработчиков.

Я неоднократно встречаю это утверждение, увы. С моей точки зрения комментарий — не менее важная часть кода, чем исполняемая часть, и если разработчик правит исполняемую часть, но принципиально отказывается держать комментарии адекватными (или слишком часто забывает их обновить), то он профессионально непригоден по той же причине, по которой он в этом случае не заметит нарушения логических соответствий с другими частями кода.

Вдумайтесь, вы описываете некий алгоритм в широком смысле средствами языка программирования. Почему это описание, по вашему, должно сопровождаться еще какими то описаниями? Может вы просто плохо описываете?

Именно потому, что в исполняемом коде присутствует только финальный результат текущей работы в виде указания, что должно быть сделано и, до достаточных подробностей, как именно, но не говорится ничего про то, почему было принято именно такое решение среди десятков альтернатив. Любое описание должно каким-то образом называть эти альтернативы и описывать их свойства, а это уже такие данные, которые в принципе не должны исполняться компьютером! То есть по своему определению эти данные не должны попасть в исполняемый код.

А дальше, если это понято и осознано, начинается выбор, каким именно образом эти данные должны быть при исполняемом коде, достаточно близко к нему, лёгким образом доступны программисту, работающим с этим кодом. И вот тут они начинают распадаться на несколько составляющих:
1. Данные, которые каким-то логическим образом привязаны именно к конкретным частям кода, вплоть до строк и частей строк. И во многих случаях (а до появления SCM средств — считаем, единственный удобный вариант) эти данные привязываются именно как комментарии.
2. Данные, которые лучше всего привязываются к изменениям, но хуже — к конкретным строкам (например, переполняют комментарии, или объединяют множество мелких изменений в разных частях кода) — такие идут в данных с временно́й привязкой: сообщениях коммитов SCM, записях в тикетах, отчётах о проделанном, разделах changelog в документации.
3. Данные, которые объединяют многие части кода, но относятся не к изменениям, а к состояниям после изменений. Эти данные лучше всего держать в отделённой документации, от ТУ+ТЗ до разнообразных руководств пользователя и админа.

Каждый из этих трёх видов данных не может быть явно сформулирован в исполняемом коде. Если кто-то полагает, что для понимания программы достаточно чтения её исполняемого кода, то он совершает принципиальную логическую ошибку — он читает не то, что было задумано или реализовывалось, а то, что состоялось, и делает реконструкцию исходного замысла по коду. Во многих случаях такая реконструкция осмысленна или даже единственно полезна, но для дальнейшей работы дорабатывающий будет фактически строить те же уровни у себя в голове, даже если это не осознаёт.

И да, я считаю, что тип 1 лучше всего держать непосредственно при коде, а не отделённо; и не я один так считаю (см. literate programming от Кнута).

Обычно подобные вещи используются когда документацию писать недосуг.

Предположу, что Вы имели в виду только документацию пользователя кода как библиотеки или интерфейса. В этом случае, да, средства типа doxygen позволяют заменить существенную часть работы по оформлению отдельной документации; но они тем и хороши, что, будучи полностью реализованными, дают возможность читающему легко восстановить отсутствующие части, как изначальная задача. В сочетании с надёжной привязкой к собственно исполняемому коду они дают резкое уменьшение числа случаев, когда документация отстаёт, просто за счёт совместного пребывания исполняемого кода и описания. По-моему, это только плюс, а не минус.

Звучит это все очень правильно. Очень. Однако же, в подавляющем большинстве случаев из жизненного опыта ни комментариям ни документации верить нельзя совершенно. Всегда у всех не хватает времени и желания это поддерживать, или за это просто не платят. Итого, код единственная вещь которая представляет актуальную документацию системы. И огромное спасибо, если еще есть хоть какое то описание бизнес логики. Ну как бы можете мне еще рассказать про космические корабли, я послушаю.

Однако же, в подавляющем большинстве случаев из жизненного опыта ни комментариям ни документации верить нельзя совершенно.

Да, когда никто за этим принципиально не следит. С моей точки зрения это примерно такая же позиция, как у панка многолетняя немытость, и отношение к ней у цивилизованных людей — такое же.

Ну как бы можете мне еще рассказать про космические корабли, я послушаю.

Знаете, я готов рассказывать, но не тем, кто считает своей принципиальной позицией сидение в куче мусора.

Да, когда никто за этим принципиально не следит.
Да, именно потому что никто за этим принципиально не следит.
Я просто не считаю что в такой ситуации мне стоит плевать против ветра.
Сначала «новый интересный проект, award-winning бла бла бла», а потом как заглянешь в код
youtu.be/7fx-uTH2El8?t=25s
верить
опять вопрос веры?
так не верьте.
Итого, код единственная вещь которая представляет актуальную документацию системы
нет.
код — это далеко не всегда отображает решаемую задачу. иначе, откуда берутся функциональные баги?
код — далеко не всегда представляет собой лучшее в указанных условиях решение задачи. иначе не было бы нефункциональных багов.
дока как раз дает конструктивную информацию — к чему надо стремиться.
и комментарий — как частный случай документации.
код только показывает, как это в настоящий момент реализовано. стартовая информация. не более.
опять вопрос веры?
Доверия.
дока как раз дает конструктивную информацию — к чему надо стремиться.
и комментарий — как частный случай документации.
comuedu.ru/...mdEditor/d4c332ca58e8.jpg
И вообще, что вы мне тут про документацию и требования?? У Вас еще не Agile? Мне прийти? :)))))))))))) Waterfall’ищи недобитые...
И вообще, что вы мне тут про документацию и требования?? У Вас еще не Agile?
В Scrum’e есть такой артефакт. Называется story. Описывает, как должен работать и что должен давать проекту реализуемый функционал.
Если это не документация, то что же?

Вот так вот тихо начали с

отделённой документации, от ТУ+ТЗ до разнообразных руководств пользователя и админа
сообщениях коммитов SCM, записях в тикетах, отчётах о проделанном, разделах changelog в документации.
И закончили user story «as a user i want to have a fucking great button for everything» .
Ок, допустим, документация есть.
Стоит написать
// this algorithm is completely awesome
В качестве обоснования выбора алгоритма решения поставленной задачи или не надо?
Стоит написать
// this algorithm is completely awesome
В качестве обоснования выбора алгоритма решения поставленной задачи или не надо?
думаю, не надо.
с таким подходом — и код писать не надо.
все равно там будет что-то типа:

function power(base, exponent) {
if (1 == exponent) return base;
if (0 == exponent) return 1;
return base * power(base, exponent — 1);
}
и надо будет все равно переписывать через время.

И как же вы себе представляете документацию к проекту сделанному по Agile? ( не скрамом единым, канбан тоже использовал).
P.s. если бы не проблемы с размером стека, то О(n) сложность в общем случае (не для конкретного алгоритма) — просто прекрасно.

И как же вы себе представляете документацию к проекту сделанному по Agile?
смотря, что вы называете документацией.
в моем понимании, комментарии — контекстная документация, равно как и стори. в вашем понимании, судя по всему, нет. прежде чем я всерьез буду отвечать на откровенно провокационный вопрос, давайте, вы уточните, что вы вкладваете в него.
P.s. если бы не проблемы с размером стека, то О(n) сложность в общем случае (не для конкретного алгоритма) — просто прекрасно.
отлично. будем исходить из того, что тестирование не выявило ошибок.
давайте, обсудим вопрос документирования на этом, явно синтетическом примере.
где, с вашей точки зрения, мне(как разработчику, который хочет реюзать этот код) надо смотреть ответы на следующие вопросы:
1. намеренно или нет входящие параметры не проверяются на корректность?
2. намеренно или нет игнорятся дробные и отрицательные значения?
3. могу ли я дополнить реализацию с учетом пунктов 1 и 2? — чтоб можно было реюзать в другом месте, где мне определенно надо больше возможностей.

Этот пример на каком языке? JavaScript?

эм. если тяжело, могу на другом переписать.
предлагайте свой.


int power(int base, int exponent) {
if (1 == exponent) return base;
if (0 == exponent) return 1;
return base * power(base, exponent — 1);
}
дополнительно вопрос:
3а) почему не использовались template’ы? — где мне смотреть информацию об этом?

Это вы меня спрашиваете? Это же ваша функция.

Стоит написать
// this algorithm is completely awesome
В качестве обоснования выбора алгоритма решения поставленной задачи или не надо?

Смотря какие требования к коду... может, он предназначен не работать, а ублажать взор начальства :)

жестко у вас с джунами. видимо действуете по принципу «волки-санитары леса»

1. Ставим тулзу на выбор en.wikipedia.org/..._of_tools_for_code_review
(гитхабы/гитлабы/битбакеты и прочие открытые репо поддерживают написание комментов «из коробки» сразу при просмотре исходников на сайте репо)
2. просматриваем исходный код, пишем комменты к непонятным/некрасивым/ненужным местам.
3. Автор получает тонну уведомлений на емейл о написанных комментах.
4. В зависимости от иерархии и субординации автор вступает в комментах в полемику или исправляет свои ошибки.
5. ???
6. PROFIT!

Подписаться на комментарии