Not a kernel guy

… in the Windows kernel team

Wednesday, February 21, 2007

Рецензирование кода (code review).

Рецензирование кода (перевод подсмотрел у Лебедева) – это на мой взгляд одна из полезнейших и при этом наиболее легко внедряемых практик разработки надёжного кода. Основная идея рецензирования заключается в систематической (пере)проверке кода с целью найти ошибки, допущенные при его написании. И поскольку рецензирование кода относится к ранним этапам разработки, найденные ошибки «ценнее», чем, скажем, ошибки, найденные при формальном тестировании.
Я не буду останавливаться на подробном описании процедуры рецензирования. В Интернете можно найти массу материалов по теме. Вот, например, страница из Википедии. Я же просто хочу поделиться своими наблюдениями.

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

Ещё одно положительное качество – внедрение рецензирования открывает дорогу другим техникам разработки надёжного кода. Скажем, написание юнит-тестов традиционно откладывается «на потом» и в конце концов тест так и остаётся ненаписанным. Рецензент же всегда может попенять автора кода за отсутствие юнит-тестов.

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

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

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

Во втором случае, автор посылал diff изменений рецензенту, который посылал свои комментарии, просмотрев код в удобное ему время. В этом случае рецензент мог полностью сосредоточится на логике кода. Результативность в этом случае была гораздо выше. Редкий код оставался без комментариев.

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

Posted at 1:24 am •

RSS feed | Trackback URI

11 Comments »

Comment by Alex Lebedev — February 21, 2007 @ 4:19 pm

Спасибо, хорошая статья.

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

 
Comment by Not a kernel guy — February 21, 2007 @ 8:36 pm

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

 
Comment by ryfm — February 21, 2007 @ 10:24 pm

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

из вчерашнего :)

IDictionaryEnumerator TN = csyndbf.GetEnumerator();
string tablefilt = "";
bool flag = true;
while (TN.MoveNext())
{
    if (flag)
    {
        flag = false;
        tablefilt += String.Format("@Dict = '{0}'", TN.Key);
    }
    tablefilt += String.Format(" or @Dict = '{0}'", TN.Key);
}

вылилось в

foreach (DictionaryEntry entry in csyndbf)
{
    tablesFilter +=
        String.Format("@Dict = '{0}' or ", entry.Key);
}

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

 
Comment by Not a kernel guy — February 21, 2007 @ 10:26 pm

 

тем более в таком случае сидя рядом с автором быстрее понимаешь код.

Так в этом всё и дело. Код должен быть таким, чтобы автор для его понимания не требовался.

 
Comment by G2 — February 22, 2007 @ 12:56 am

вылилось в

foreach (DictionaryEntry entry in csyndbf)
{
    tablesFilter += String.Format(”@Dict = ‘{0}’ or “, entry.Key);
}

Фильтр будет правильно работать, если в конце будет ‘’…or @Dict = ‘key’ or “

 
Comment by Dmitry — February 22, 2007 @ 1:58 am

В последнем абзаце говорится о удобных инстументах, может назовете парочку для примера?

 
Comment by ryfm — February 22, 2007 @ 2:16 am

to G2
там просто еще несколько циклов добавляющих условие фильтрации, а в конце лишний OR просто отрезается.

 
Comment by ryfm — February 22, 2007 @ 2:18 am

 

Так в этом всё и дело. Код должен быть таким, чтобы автор для его понимания не требовался.

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

 
Comment by ryfm — February 22, 2007 @ 2:21 am

2 Dmitriy
программ Диффов в общем - то навалом, мне например нравится связка
SVN + TRACK, и версионность и просмотр дифов и багтрекинг и планирование, плюс еще можно к TRACKу много чего наворотить.

 
Comment by Not a kernel guy — February 22, 2007 @ 9:09 am

 

Так в этом всё и дело. Код должен быть таким, чтобы автор для его понимания не требовался.

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

Но если код “такой, какой он есть”, то код не пройдет review. Автор конечно может наплевать на мнение рецнгзента и не исправлять такой код, но на моеё памяти такого не бывало.

 
Comment by Not a kernel guy — February 22, 2007 @ 9:10 am

 

2 Dmitriy
программ Диффов в общем - то навалом, мне например нравится связка
SVN + TRACK, и версионность и просмотр дифов и багтрекинг и планирование, плюс еще можно к TRACKу много чего наворотить.

+ 1 за Subversion & Trac. Я как раз пишу следующий пост о том, как организовать рецензирование в Trac.

 

Your Comment (smaller | larger)

You can use these tags: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>

Powered by WordPress