code review
Max Ischenko
Привет,
Практикуется ли в вашей команде разработчиков code reviews? Т.е чтение (анализ) кода других разработчиков? Если да, то в каком виде? Как вы вообще относитесь к этой идее?
Просто интересно.
ИМХО, идея здравая. Сама мысль о том, что твой код может попасть "на досмотр" дисциплинирует, что уже большой плюс. ;-) Конечно, можно довести и ее до абсурда, например, когда 5 человек полдня изучают один модуль, вместо того чтобы работать.
Alexander S.Tereshchenko
You wrote to <ua-de...@googlegroups.com> on Thu, 31 Mar 2005 14:03:13
+0300:
MI> Практикуется ли в вашей команде разработчиков code reviews? Т.е
MI> чтение (анализ) кода других разработчиков? Если да, то в каком виде?
В связи с жестким индивидуализмом индивидумов пишишух этот самый код о
такой практике у нас можно даже и не мечтать :) По крайней мере в ближайшее
время.
MI> Как вы вообще относитесь к этой идее?
Идея несомненно здравая. Но идеи - это только идеи, до тех пор пока не
воплощены в реальность... Так что возможны вырианты :)))
MI> ИМХО, идея здравая. Сама мысль о том, что твой код может попасть "на
MI> досмотр" дисциплинирует, что уже большой плюс. ;-)
Не всех. Тем кто _сидит_ на работе с 8:00 до 17:00 глубоко начхать на
всех...
MI> Конечно, можно довести и ее до абсурда, например, когда 5 человек
MI> полдня изучают один модуль, вместо того чтобы работать.
Или когда термин "code review" можно заменить термином "об...ри автора"
:)
з.ы. Не весеннее сегодня что-то настроение у меня :))))
With best regards, Alexander S.Tereshchenko. E-mail: alt...@malva.ua
Mike Gorchak
MI>> Практикуется ли в вашей команде разработчиков code reviews? Т.е
MI>> чтение (анализ) кода других разработчиков? Если да, то в каком виде?
AS> код о такой практике у нас можно даже и не мечтать :) По крайней мере в
AS> ближайшее время.
Вернее надо сказать забыли об этой практике, хотя она год была очень
действенной.
MI>> Как вы вообще относитесь к этой идее?
AS> не воплощены в реальность... Так что возможны вырианты :)))
Идея абсолютно здравая, но отнимающая очень много времени, в связи с чем я
ее и похерил.
MI>> ИМХО, идея здравая. Сама мысль о том, что твой код может попасть "на
MI>> досмотр" дисциплинирует, что уже большой плюс. ;-)
AS> на всех...
Саша, глубокое чхательство регулируется заработной платой. В контракте
кстате указано, что весь код должен соответствовать внутренним стандартам и
за их несоблюдение можно получить. Если тебе не нравится чей-то код, то
welcome с письменным заявлением к начальству, идею просмотра кода постоянно
хотят реанимировать, так что оно будет даже радо.
MI>> Конечно, можно довести и ее до абсурда, например, когда 5 человек
MI>> полдня изучают один модуль, вместо того чтобы работать.
AS> автора" :)
Когда это оговорено в контракте и есть стандарт кодирования, то это именно
code review.
AS> з.ы. Не весеннее сегодня что-то настроение у меня :))))
Угу, видно :)
With best regards, Mike Gorchak. E-mail: mi...@malva.ua
Alexander S.Tereshchenko
You wrote to <ua-de...@googlegroups.com> on Thu, 31 Mar 2005 14:34:55
+0300:
MI>>> Как вы вообще относитесь к этой идее?
AS>> Идея несомненно здравая. Но идеи - это только идеи, до тех пор
MG> Идея абсолютно здравая, но отнимающая очень много времени, в связи с
MG> чем я ее и похерил.
Мне кажется "code review" надо делать массовой процедурой (по типу того,
что предлагает XP). Хотя, я конечно, понимаю, что сколько человек столько и
мнений... Что скажет all по поводу этого?
MI>>> ИМХО, идея здравая. Сама мысль о том, что твой код может попасть
MG> Саша, глубокое чхательство регулируется заработной платой.
Интересно на основе каких критериев можно определить что Вася забил на
работу, а Петя пашет как буйвол? (Хотя, впрочем, это уже другая тема...)
MG> В контракте кстате указано, что весь код должен соответствовать
MG> внутренним стандартам и за их несоблюдение можно получить.
MG> Если тебе не нравится чей-то код, то welcome с письменным заявлением к
MG> начальству, идею просмотра кода постоянно хотят реанимировать, так
MG> что оно будет даже радо.
Че-то похоже на поведение "из-под палки". То, что активно людям
навязывается при ослаблении контроля рискует быть тут же забыто. Что и
произошло в нашем случае, IMXO.
MI>>> Конечно, можно довести и ее до абсурда, например, когда 5 человек
MI>>> полдня изучают один модуль, вместо того чтобы работать.
AS>> Или когда термин "code review" можно заменить термином "об...ри
AS>> автора" :)
MG> именно code review.
На бумаге, к сожалению, всех ньюансов не изложишь. Понятное дело 80%
кода подчиняются каким-то правилам (эмпирическим???). Но всегда остается еще
20%. Как резрешать конфликтные ситуации?
Mike Gorchak
....
[skipped]
MG>> В контракте кстате указано, что весь код должен соответствовать
MG>> внутренним стандартам и за их несоблюдение можно получить.
MG>> Если тебе не нравится чей-то код, то welcome с письменным заявлением
MG>> так что оно будет даже радо.
AS> Че-то похоже на поведение "из-под палки". То, что активно людям
AS> навязывается при ослаблении контроля рискует быть тут же забыто. Что и
AS> произошло в нашем случае, IMXO.
Смотря у кого как, кто с жадностью подхватил и использует, а кому и из
зарплаты вычитай - толку не будет :) Вот спроси у Кости соблюдает ли он
стандарты, в создании которых он принимал наибольшее участие :) Я могу
сказать что половина людей таки их придерживается без всяких напоминаний,
что уже есть гут. По поводу проверки качества - тут стандартом все не
опишешь - надо глядеть персонально, но тем не менее у нас дерьмовенький код
пишут единицы по отношению к числу пишущих :)
AS> На бумаге, к сожалению, всех ньюансов не изложишь. Понятное дело
AS> 80% кода подчиняются каким-то правилам (эмпирическим???). Но всегда
AS> остается еще 20%. Как резрешать конфликтные ситуации?
Время покажет кто был прав :)
DSennik
Под code review можно понимать как name convention
так и refactoring.
Обычно на code review для name convention
практически никогда не выделяеться
времени. Просто это должно быть как
факт, что код должен быть хорошо
оформлен. Если же этого нету, то любой
человек который потом будет с работать
с этим кодом может пожаловаться
начальству.
Но всёже code review надо делать для
новичков в компании, или для людей
незнакомых с name convention просто для того
чтоб приучить их. Но это не должно
входить в постоянную процедуру.
Касательно refactoring, то это должно быть
итерацией котоая должна выполняться
всегда, когда на это есть время или
надобность. Тоесть при исправлении
ощибки, при изменениях в проекте,
просто при разработке. Тоесть это
должна быть постоянная процедура.
Для name convention и для refactoring есть
множество утилит которые помогают это
делать. Для любого разработчика это не
будет сложным процессом. Но ведь
вправда всегда приятно взять красиво
написанный и продуманый код. Так что я
думаю за этим должен следить сам
владелец кода.
yurine
Про code style review тут уже написали,
достаточно хорошо.
Code review, в моём понимании (без ссылки на
авторов и источников) - проверка кода в
рамках QA, в целях выявления
а) проблем на ранних стадиях, как
архитектурных, так и просто багов;
б) средство повышения качества, т.к.
позволяет отловить баги, которые могут
не всплыть при тестировании.
Я имею небольшой опыт попыток
внедрения.
Главный фактор - участники (авторы кода
и ревьюверы) должны быть высоко
профессиональны и этичны, без
каких-либо (даже самых малых) намёков
на "щас я тебя уделаю/ну попробуй
уделай меня". Они должны думать о
повышении качества, надёжности,
прибыли (ну, вообще говоря, кому что
важнее).
Ну и сам код должен быть выше
определённого порога качества, ибо в
помойке ревью работать не будет.
Примеры отлавливаемых проблем:
а) потенциальная ошибочная ситуация
(NPE, array index out of bounds);
б) архитектурная проблема (загрузка в
память всего файла вместо
последовательной обработки,
неадекватная объектная модель,
неадекватная реляционная модель);
Ну и всякие другие мелочи, вплоть до
грамматических ошибок в
идентификаторах.
Следует понимать, что code review почти
всегда находится за гранью тех 20%
усилий, которые дают 80% результата.
Поразмышлял бы больше, но времени...
sergey....@gmail.com
соблюдение - практически пустая трата
времени - проще по мере как
сталкиваешься делать замечания.
Одного из наших разработчиков мы так и
не научили вставлять пробел после
запятой
А Code Review это не должен быть refactoring. Это
должно быть проверкой на соответствие
кода изначально
планированного/разработанного
дизайна и качества кода, например с
точки зрения устойчивости кода к
неправильно переданным параметрам и
будущим изменениям взаимосвязанных
компонентов. (например, лог не должен
валиться если Currentuser == null))
А вот если пришлось делать
рефакторинг, и при этом уровень
специалиста превосходил сложность
задачи, то это уже повод для разговора.
DSennik
многие аспекты разработки софта.
Как правильно было упомянуто должно
быть разделение:
- code review
- code style review
Про code style review тут уже говорилось что
на него не стоит тратить времени
поскольку это должен делать сам
девелопер если он таким являеться.
Если код заимствован, то на рынке
имееться довольно много продуктов
которые умеют форматировать его
автоматичкски.
Теперь касательно чистого code review.
Как было сказано выше
-------cut--------
Code review, в моём понимании (без ссылки на
авторов и источников) - проверка кода в
рамках QA, в целях выявления
а) проблем на ранних стадиях, как
архитектурных, так и просто багов;
б) средство повышения качества, т.к.
позволяет отловить баги, которые могут
не всплыть при тестировании.
-------cut--------
Тут можно выделить два этапа:
- тестирование;
- упрощение (улучшение) архитектуры
- повышение качаства кода;
Почему тестирование: Потому что было
написано выявлени NPE и прочих багов на
ранних стадиях. Тоесть по сути это
является тестирование. Моё мнение что
для этих целей, больше подходят Unit test.
Тоесть девелопер пишет тест (в том
числе и для NLP) и потом это использует.
Review являеться просмотром, и не может
дать 100% увереность в коде, а ! правильно
! написаный тест может. Более подробно
смотри XP, testdriving development, Unit test, JUnit.
Злые языки скажут, что если у
архитектора рукит крывые то и надо
глаз да глаз. Реально сразу
архитектуру построить сложно. Вот тут
как раз и помагает обсуждаемый нами code
review.
Может даже большне architecture review. Тут уже
применяеться тяжелая артилерия, в том
числе и refactoring.
Простой девелопер тоже аржитектор. Он
же и есть архитектор для процедур,
методов и т.д. и если он делает просмотр
своего кода и видит что метод на 4
экрана, то ему стоить подумать и
разбить его на более маленькие. Тоесть
моё мнение, что именно это можно
отнести к code review.
Что у нас получилось:
- code review
--- review for bug (testing)
--- review for architecture (refactoring)
--- clear code review
- code style review
Сорри, будет время отшлифую понятия.
P.S. Огромное спасиба создателям этой
группы, да и вообще всего этого
движения.
Mike Gorchak
D> Про code style review тут уже говорилось что
D> на него не стоит тратить времени
D> поскольку это должен делать сам
D> девелопер если он таким являеться.
D> Если код заимствован, то на рынке
D> имееться довольно много продуктов
D> которые умеют форматировать его
D> автоматичкски.
Не надо так буквально воспринимать code style как простое форматирование,
это и наименование и набившее уже оскому i++ vs ++i, strcpy vs strncpy и
т.д. и т.п.
Andrey Khavryuchenko
"MI" == Max Ischenko wrote:
MI> Практикуется ли в вашей команде разработчиков code reviews? Т.е чтение
MI> (анализ) кода других разработчиков? Если да, то в каком виде? Как вы
MI> вообще относитесь к этой идее?
Team code ownership + Test-driven development приводят к потрясающим
результатам после первого же случая, когда твой код отредактировал кто-то
другой :)
--
Andrey V Khavryuchenko http://www.kds.com.ua/
Silver Bullet Software Solutions http://www.livejournal.com/~akhavr
Max Ischenko
> Как я вижу маленькая тема затронула
> многие аспекты разработки софта.
А как же иначе. ж-)
> Теперь касательно чистого code review.
Мне кажется мы понимаем это немного по разному. В моем понимании,
review проводится другими разработчиками, а не автором оригинального
кода. В крайнем случае - совместно с другими разработчиками. (на то
оно и peer review).
То, что программист свой код сам смотрит и меняет это часть
нормального процесса разработки, но это не code review. (блин, есть
адекватный русский перевод этого термина?).
> Как было сказано выше
> -------cut--------
> Code review, в моём понимании (без ссылки на
> авторов и источников) - проверка кода в
> рамках QA, в целях выявления
> а) проблем на ранних стадиях, как
> архитектурных, так и просто багов;
> б) средство повышения качества, т.к.
> позволяет отловить баги, которые могут
> не всплыть при тестировании.
в) обнаружение дублирующейся функциональности. Зачастую разработчики,
независимо друг от друга реализуют похожую функциональность. Особенно
это касается вспомогательных классов/модулей. Как то работал я с
codebase где было несколько реализаций класса String, но это уже
совсем клиника. ;-))
г) самообучение. Читая чужой код, можно наткнуться на полезные приемы,
которые пригодятся в дальнейшем самому.
д) ...
Наверное, тут много чего можно еще придумать, однако на мой взгляд,
основная цель - повышение качества. И здесь главным становится найти
компромисс между стоимостью code review и отдачей от этого процесса.
Как обычно в sw.eng. точных цифр нет, поэтому полагаемся на опыт и
эмпирические правила. Собственно, именно об опыте других я и хотел
услышать, когда поднимал этот вопрос.
> -------cut--------
> Тут можно выделить два этапа:
> - тестирование;
> - упрощение (улучшение) архитектуры
> - повышение качаства кода;
"есть три типа людей - одни умеют считать до трех, другие нет". (с) ж-)
Но вообще согласен, это действительно вполне самостоятельные цели и
code review могут делать упор на какой-то одной из них.
> Почему тестирование: Потому что было
> написано выявлени NPE и прочих багов на
> ранних стадиях. Тоесть по сути это
> является тестирование. Моё мнение что
> для этих целей, больше подходят Unit test.
> Тоесть девелопер пишет тест (в том
> числе и для NLP) и потом это использует.
> Review являеться просмотром, и не может
> дать 100% увереность в коде, а ! правильно
> ! написаный тест может. Более подробно
> смотри XP, testdriving development, Unit test, JUnit.
Правда, гарантий что тест не содержит ошибку, опять же нет. Или что
тест стал out-of-date в связи с изменением в коде и теперь не
отлавливает новые граничные случаи, к примеру.
> Злые языки скажут, что если у
> архитектора рукит крывые то и надо
> глаз да глаз. Реально сразу
> архитектуру построить сложно. Вот тут
> как раз и помагает обсуждаемый нами code
> review.
> Может даже большне architecture review. Тут уже
> применяеться тяжелая артилерия, в том
> числе и refactoring.
Угумс. Я с большими проектами особо не работал, но мне кажется
refactoring (в понимании Фаулера) тут уже не поможет - это
макроуровень, а не микроуровень (куда по сути относится refactoring).
--
WBR, max
http://ischenko.blogspot.com/
Max Ischenko
> MI> Практикуется ли в вашей команде разработчиков code reviews? Т.е чтение
> MI> (анализ) кода других разработчиков? Если да, то в каком виде? Как вы
> MI> вообще относитесь к этой идее?
>
> Team code ownership + Test-driven development приводят к потрясающим
> результатам после первого же случая, когда твой код отредактировал кто-то
> другой :)
А подробнее?
:)
Max Ischenko
> D> Про code style review тут уже говорилось что
> D> на него не стоит тратить времени
> D> поскольку это должен делать сам
> D> девелопер если он таким являеться.
> D> Если код заимствован, то на рынке
> D> имееться довольно много продуктов
> D> которые умеют форматировать его
> D> автоматичкски.
>
> Не надо так буквально воспринимать code style как простое форматирование,
> это и наименование и набившее уже оскому i++ vs ++i, strcpy vs strncpy и
> т.д. и т.п.
ИМХО, просто нужно разделять эти понятия, иначе в голове у
разработчиков будет каша.
Есть (обычно) стандарт на форматирование кода, который легко
документируется и (обычно) легко поддерживается при помощи
инструментальных средств, достаточно иметь минимальный уровень
дисциплины.
Есть вопросы naming convensions. Тоже понятно.
Ну а есть то что называется (по крайней мере, я это так называю -
может оно никак и не называется ;-)) микроуровень. Сюда идут и
пресловутые strcpy/sprintf для С, RAII (для плюсов) и try/finally (для
Java) ну и прочая, прочая, прочая.
В принципе, code review может быть особенно эффективен для выявления
проблем на микроуровне и передачи опыта.
Andrey Khavryuchenko
"D" == DSennik wrote:
D> Почему тестирование: Потому что было написано выявлени NPE и прочих
D> багов на ранних стадиях. Тоесть по сути это является тестирование. Моё
D> мнение что для этих целей, больше подходят Unit test. Тоесть девелопер
D> пишет тест (в том числе и для NLP) и потом это использует.
NLP в этом контексте?
Andrey Khavryuchenko
"MI" == Max Ischenko wrote:
MI> Практикуется ли в вашей команде разработчиков code reviews? Т.е чтение
MI> (анализ) кода других разработчиков? Если да, то в каком виде? Как вы
MI> вообще относитесь к этой идее?
>>
>> Team code ownership + Test-driven development приводят к потрясающим
>> результатам после первого же случая, когда твой код отредактировал кто-то
>> другой :)
MI> А подробнее?
А что подробнее?..
Каждый может редактировать код каждого - это поддерживается и поощряется
культурой в команде. Если девелопер, который увидел твой кривой код будет
на взводе - ты огребёшь от него в аське/конференс колле сразу. Если он
будет сфокусирован на другом - ты сам будешь неприятно удивлён, увидев как
поменялся твой код.
Вкупе с непосредственным общением ("какого ты это сделал с моим кодом!" :)
общий командный стиль устаканивается очень быстро. При чём в нём участвует
_каждый_ разработчик, что и требовалось :)
Andrey Khavryuchenko
"MI" == Max Ischenko wrote:
MI> тест стал out-of-date в связи с изменением в коде и теперь не
MI> отлавливает новые граничные случаи, к примеру.
1. Дублировать тесты на разных уровнях (к примеру у меня, в некоторых
частях проекта один и тот же код тестируется четырьмя разными слоями
тестов; три - это вообще стандартно)
2. TDD - ВНАЧАЛЕ определяем цель, документируем её тестом, проверяем что
тест падает и лишь ЗАТЕМ меняем код, чтобы пофиксить багу :)
MI> Угумс. Я с большими проектами особо не работал, но мне кажется
MI> refactoring (в понимании Фаулера) тут уже не поможет - это
MI> макроуровень, а не микроуровень (куда по сути относится refactoring).
Смотря что рефакторить. Я, к примеру, последнюю неделю занимаюсь как раз
рефакторингом "требований" кастомера в вид, удобный для логарифмирования.
Тесты рефакторятся точно так же, как и любой другой код.
sergei.m...@gmail.com
:-), могу сказать, что code review - это очень и
очень хорошо. до недавнего времени у
нас это был обязательный элемент на
каждом цикле разработки. сейчас его
объявили optional, и соответственно все
сразу же забили, а жаль.
как правило, code review происходит во время
тестирования, т.е. каждый девелопер
знает, что ему, скажем, в течении след.
2х недель нужно потратить полдня-день,
чтобы просмотреть код другого
девелопера, и еще несколько часов,
чтобы пофиксить свой код в
соответствии с чужими замечаниями.
перед началом code review всегда громко
объявляется, что это не охота на ведьм,
и никаких мер по отношению к тем, к
чьему коду много претензий,
применяться не будет (конечно, если все
замечания будут учтены). кто и что
просматривает - назначается
начальством на уровне team leader-a или зам.
development director-a, отчеты сдаются project manager-y,
который вообще следит за выполнением
всех этапов и потоком документов. как
правило, команда гуи просматривают код
у себя, а backend - у себя, и вообще, на review
назначается тот, кто способен въехать
в контекст того или иного куска. нужно
или не нужно делать code inspection - не
обсуждается: нужно, у нас так принято.
возбухания отдельных личностей по
поводу того, что "это мой код"
пресекаются начальством немедленно и
жесточайшим образом - во-первых, код
принадлежит компании, а во-вторых,
править его может любой девелопер, по
определению.
результатом code review у нас является
стандартный вордовый документ, но, я
думаю, то же самое можно сделать
средствами багзиллы или другого
подобного софта. ключевой момент - на
каждое замечание обязательно должен
быть ответ девелопера - типа, да, есть
такой баг, уже пофиксил (или напр.
пофикшу в след. релизе), или же - нет, все
так и задумано, и вот почему. раньше
(давно) еще полагалось делать code review на
каждый багфикс во время product test-a, и в
resolution на каждый баг писать, что было
исправлено и кто этот код
проинспектировал, но это уже перебор
имно.
в целом же, имно, от code review сплошная
польза - главное, потому, что это
замечательный формальный способ
заставить девелоперов читать чужой
код, привыкать к тому, что их код тоже
читают и вообще обмениваться опытом
кодирования. ну и баги ловятся таким
образом очень хорошо, особенно такие,
которые во время простого
тестирования поймать крайне трудно
(напр. очень незначительный memory leak или
проезд по производительности).
кстати, пара слов про то, что
выявляется на этапе code inspection-a, а что
нет. практика показывает, что народ
смотрит код достаточно поверхностно, и
цепляется в основном по мелочам.
предложений по улучшению архитектуры
на этом этапе не поступает, т.к. все уже
оговорено, да и сильно менять текущий
релиз на этом этапе уже как правило
поздно. так что архитектурные
предложения перекочевывают на митинги
по обсуждению следующих релизов
проекта. в то же время, достаточно лего
отслеживаются несоответствия
реализации изначально принятому
дизайну. кроме того, моментально
обнаруживаются затычки типа // FIXME:
implement later, так что сачковать и
дописывать код во время финальных
стадий тестирования становится тоже
гораздо труднее. :) несоответствия coding
conventions тоже входят в отчет.
по поводу багов - как показывает
практика, лучше всего ловятся такие
вещи: в плюсах - exception safety, const correctness,
выбор STL алгоритмов и контейнеров,
отсутствие reserve() там где можно его
применить; в джаве - неудачный выбор
монитора синхронизации, неудачный
выбор контейнеров и опять же
отсутствие резервирования
контейнеров, вольное обращение с
ресурсами типа DB Connection. вообще много
всего вылезает между БД и приложением -
например, динамические запросы там,
где можно один раз сделать prepare statement,
вставка/изменение записей по одной
там, где можно сделать batch insert или
передать массив в хранимую процедуру и
т.п. в целом, code review помогает не столько
найти баги, сколько причесать код,
улучшить производительность и лишний
раз построить девелоперов. :))
hope this helps!
motus.
Max Ischenko
> т.п. в целом, code review помогает не столько
> найти баги, сколько причесать код,
> улучшить производительность и лишний
> раз построить девелоперов. :))
Ага. Совпадает с моими спекуляциями.
Спасибо!
yurine
> и лишний раз построить девелоперов. :))
В целом всё совпадает с моими
побуждениями и выводами.
Ещё раз отмечу, что я рассматриваю code
review только в смысле второй пары глаз.
Иначе эффекта не будет.
Поэтому касательно идеи заменить на unit
testing -- одна пара глаз в тесте может
натупить также как и в базовом коде.
Или развивая мысль -- бывает
самопроверка, а бывает независимая
проверка. Так вот любую технику
проверки (тестирования) можно делать
самому и независимо.
Народная мудрость гласит "тупят все". И
самое лучшее средство - независимая
проверка. Пусть даже не
специализированным проверяльщиком, а
просто незамутнённым взглядом и
мозгами, устроенными чуть-чуть иначе.
Касательно перевода. Глядя в Лингво,
становится ясно что английский
вариант не более расплывчат и неточен,
чем любой русский перевод: просмотр
кода, перепросмотр кода, проверка кода,
рецензия кода.
sergei.m...@gmail.com
> становится ясно что английский
> вариант не более расплывчат и
неточен,
> чем любой русский перевод: просмотр
> кода, перепросмотр кода, проверка
кода,
> рецензия кода.
официальный термин у нас - code inspection