Code Review

← →
Alkid © (2010-06-05 21:37) [0]

Коллеги!
Поделитесь опытом. Как у вас в компаниях организована инспекция кода?
1. Фокусируетесь ли вы на инспекции diff`ов отдельных check-in`ов или предпочитаете инспектировать код по модулям?
2. Практикуете ли обяхательный ревью пееред check-in?
3. Планируете ли инспекцию или соответствующий сотрудник выполняет её в фоновом режиме?
4. Практикуете senior review или peer review?
Ну и прочие тонкости.

← →
Sergey Masloff (2010-06-05 22:36) [1]

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

← →
инспектор (2010-06-06 00:34) [2]

Sergey Masloff (05.06.10 22:36) [1]

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

Иногда дороже переписывать при подходе "да пофиг, что написано, лишь бы работало". Особенно если нет стандарта разработки.

Alkid © (05.06.10 21:37)

> senior review или peer review?

Че за звери ?

> Как у вас в компаниях организована инспекция кода?

Голосом. В виде вопросов "кто автор", "кто написал такую хрень", "это вообще работает или нет"

← →
Kerk © (2010-06-06 00:46) [3]

У нас итоги каждого change request"а ревьюируются. Соответственно, такая работа всегда запланирована.

← →
Sergey Masloff (2010-06-06 09:37) [4]

Kerk © (06.06.10 00:46) [3]
"у нас" это "в ЭПАМ" или "в текущем проекте"?
Если первое то утверждение неверное про "всегда"
Я работал с тремя командами ЭПАМа про одну знаю точно что никакого ревью там не было, про вторую знаю что было. Про третью не интересуюсь

Вобщем бывает по разному.

← →
Kerk © (2010-06-06 12:36) [5]

> Sergey Masloff (06.06.10 09:37) [4]
>
> Kerk © (06.06.10 00:46) [3]
> "у нас" это "в ЭПАМ" или "в текущем проекте"?

Я не в EPAM сейчас работаю. Это все объясняет :)
Впервые работаю в "продуктовой" компании, интересный опыт.

← →
oldman © (2010-06-06 12:41) [6]

> Как у вас в компаниях организована инспекция кода?

Никак. Я выдаю продукт, а не сырье.

← →
Sergey Masloff (2010-06-06 13:11) [7]

Kerk © (06.06.10 12:36) [5]
понятно... Недавно сменил? Не секрет куда ушел?

← →
Alkid © (2010-06-06 15:20) [8]

> инспектор (06.06.10 00:34) [2]
> > senior review или peer review?
> Че за звери ?

senior review - это когда есть ведущий программист и он инспектирует код нескольких младших программистов. peer review - это когда программисты одного уровня квалификации смотрят код друг у друга.

> Голосом. В виде вопросов "кто автор", "кто написал такую
> хрень", "это вообще работает или нет"

А как код попадает на глаза подающему голос? :) Просто когда по делу приходится заглядывать?

> Sergey Masloff (05.06.10 22:36) [1]

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

> Kerk © (06.06.10 00:46) [3]

Именно change-request`a? Хм, это звучит интересно. То есть scope инспекции получается ограничен именно предментым требованием, заложенным в change request`е? Попробую применить эту идею у себя в команде.

> oldman © (06.06.10 12:41) [6]

А сколько человек пишут код?

← →
Kerk © (2010-06-06 18:35) [9]

> Sergey Masloff (06.06.10 13:11) [7]
>
> Kerk © (06.06.10 12:36) [5]
> понятно... Недавно сменил? Не секрет куда ушел?

Дык я ж в Питер уехал
Тут теперь - http://altec.ru/

← →
инспектор (2010-06-06 20:12) [10]

Alkid © (06.06.10 15:20) [8]

> senior review - это когда есть ведущий программист и он
> инспектирует код нескольких младших программистов. peer
> review - это когда программисты одного уровня квалификации
> смотрят код друг у друга.

Тогда у нас senior review

> А как код попадает на глаза подающему голос? :) Просто когда
> по делу приходится заглядывать?

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

← →
Sergey Masloff (2010-06-06 22:49) [11]

Alkid © (06.06.10 15:20) [8]
>Я склоняюсь к тому, что нужно. Писать write-only код только на первый >взгляд дешевле, чем качественный и высокосопровождабельный. Если >продукт предполагается поддерживать и развивать, то в итоге дешевле с >самого сначала контролировать качество.
Это неверное толкование! Я не говорю что не нужно всегда, просто есть код с разной степенью влияния на проект, и некоторый код по попределению смотреть вообще смысла не имеет. У любой мало-мальски грамотно спроектированой системы есть ядро, есть ключевой функционал и есть бантики. Бантики постоянно перерисовываются и вобщем-то смысла в ревью для них нет. Критичный функционал - дело другое. Но и там могут быть исключения - как я говорил иногда можно выделить изолированные блоки с четко описаным внешним интерфейсом. Если они не очень большие то и меняются редко то тоже можно забить а при необходимости переписывать с 0 под новые требования - иногда это дешевле.

У ревью есть очень большой отрицательный момент - это очень дорогой инструмент. Senior review - реально один синьор может смотреть код троих "рядовых" при этом будет тратить на это не менее 70% своего времени. То есть "мотор" проекта выключается из основной своей деятельности.
Цифра не с потолка - у нас достаточно мощная система учета и есть накомленнная статистика для анализа. К тому же при обсуждении этой проблемы с коллегами из других контор - выясняются что все пришли к этому соотношению 1:3 при этом 1 из созидательной работы почти выключается. Это допустимо только для действительно критичных участков.
Емли пиры друг друга мониторят - это полезно но тоже нужно знать меру (ИМХО)

← →
Kerk © (2010-06-06 23:27) [12]

Почему-то совершенно забыт тот аспект, что благодаря review (неважно peer или senior) с кодом всегда знакомо больше одного человека. А это очень полезно, ибо людям свойственно болеть или увольняться. С этим кодом все-равно однажды придется кому-то разбираться. Сейчас или через полгода - цена работы не изменится.

← →
Kerk © (2010-06-06 23:30) [13]

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

← →
Alkid © (2010-06-07 09:01) [14]

> Sergey Masloff (06.06.10 22:49) [11]

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

> Kerk © (06.06.10 23:27) [12]

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

← →
Kerk © (2010-06-07 09:39) [15]

Не понял. Какой смысл ревьюировать код, который и так знаешь?

← →
Sergey Masloff (2010-06-07 10:18) [16]

Kerk © (06.06.10 23:27) [12]
>Почему-то совершенно забыт тот аспект, что благодаря review (неважно >peer или senior) с кодом всегда знакомо больше одного человека.
Тут согласен - это большой плюс.

← →
Sergey Masloff (2010-06-07 11:02) [17]

Кстати еще про review - можно ж смотреть не все...
У нас (правда только для серверного кода на PL/SQL) прямо в систему контроля версий встроен функционал который при checkin считает метрики кода (halstead volume, cyclomatic complexity и так далее). Мерки сравниваются с прошлой версией, и есть заданные допустимые границы изменений. Если код "ухудшился" больше чем заложено границами - он кандидат на review автоматически.
Тогда еще приемлемо получается по затратам. Но для более сложных языков это плохо работает.

> У нас (правда только для серверного кода на PL/SQL) прямо
> в систему контроля версий встроен функционал который при
> checkin считает метрики кода (halstead volume, cyclomatic
> complexity и так далее).

С этого места подробнее, если не трудно.

← →
Sergey Masloff (2010-06-07 12:01) [19]

Игорь Шевченко © (07.06.10 11:25) [18]
Не трудно
система контроля версий своя (обертка над VSS)
есть основной сервер, сервер разработки и сервер тестирования.
На основном сервере метрики просчитаны для всех подулей (ну а точнее для всех версий всех модулей).
При check out модуль копируется на сервер разработки. При check in на сервере разработки срабатывает триггер который считает метрики по новому модулю и отправляет автору (а при плохих изменениях не только автору) сводку по изменениям. При этом модуль копируется на сервер тестирования.
Если с модулем связан инцидент или change request то он не попадет на основной сервер пока не будет закрыта автоматически генерящаяся задача на тестирование.
Если задачи нет то условием попадания на основной является успешная компиляция. Но при этом при заливке на продакшн (а это операция "ручная") администратор видит модули у которых дельты метрик вышли за диапазон.

Если речь про сами метрики то мы остановились на нескольких
- число statements (фактически число ";" для pl/sql)
- halstead volume (тут понятно ищется алгоритм за 1 минуту в яндексе)
- McCabe"s Cyclomatic Complexity - сколько линейно-независимых путей выполнения ( для PLSQL по простому через число «THEN», «LOOP», «EXIT [WHEN]», «ELSE», «RETURN», «GOTO» )
-Maintainability Index – показатель сопровождаемости программы, основанный на вышеописанных метриках и количестве строк комментариев, вычисляется по формуле:

171 - 5.2 * ln(V) - 0.23 * V(g) - 16.2 * ln (LOC) + 50 * sin(sqrt(2.4 * perCM)), где

• V = величина Halstead Volume
• V(g) = величина Cyclomatic Complexity
• LOC = количество строк кода
• perCM = процентное отношение числа русских слов комментариев к общему числу операндов и операторов

конечно это все грубо но в целом мусора меньше

Sergey Masloff (07.06.10 12:01) [19]

А метрики вы сами считаете или что-то готовое пользуете ?

← →
Sergey Masloff (2010-06-07 14:16) [21]

Sergey Masloff (07.06.10 14:16) [21]

Спасибо, идея сама неплохая, при check-in метрики автоматом считать. Надо будет взять на вооружение.

Code Review Найти похожие ветки