О проблемах с code reviews
?????-???? ? ????????????? ??? ??????...
---
??-??, ????... ????? ???????? ???????? ?? code reviews (??????? ????), ???????? ? ???? ??? ??? ?????????????? ???? ?? ?? ??? ???????????? ????????, ?? ?????????? ? ??????? ????? ??????? ?? ??????... ??? ???, ????????? ??????? ?????, ? ??? ???????!
????... ? ?? ??????, ??? ??????? ???? – ??? ?????. ?????? ??? ? ????? ??????? ???? ????? ???? ???????????? ? ??????????. ??? ??? ???????? ?????????? ????????? ?????? ??????? – cons et pros. ??? ???, ? ????? ?? ???????? ???? ???????? ?? ????????? con ??????? ????, ??????? ?????? ?? ??????????? ?????...
??????????? ????, ?? ????????? ? ????????? ??????? ? ?? ??? ????? ?????? ???????? ????? ???????. ???????? ????????: «????? ??????». ? – ???????????, ?? ?????? ??? – ??? ? ? ????? ?????? ????????, ? ??? ???? ????. ? ??? – ??? ????? ?????, ??????? ???? ??????. ??? ???????, ?? ????....
??????, ?????? ?? ???????. ??? ?? ?????? ?????? ???? ????????? ? ????, ???? ?????? ??? ????????? ??? ??? ??????: ???????? ? ???????????. ??, ??, ??, ???? ??? ????????? «?????? ?????? ???», ?? ?? ?? ????? ?????????? ??? ?????, ??????? ??????, ??????? ?? ???????? ? ??? ? ??????, ????? ??????????? ? ?? ????? ? ???? ?????????. ? ??? ??? – ?????????? – ????? ???-???? ????. ????, ???????? ??? ????????????
???, ??? ?? ???????, ??? ???????? ?????????? ???????? ??????????? ?????? ??-??, ???? ??????, ????? ???????? – ??? ?????? ???????. ????????, Quick Fix Engineering – ????? ????? ????????? ???????? ???????????? ???????????? ? ??????????? ???????? ????? ?????? ?????. ?????? ?????? – ????? ??????? ??? ????? ??????, ? ??? ??? ?? ?????? – ??? ??????? ??? ??? ????? ??????. ??, ?, ???????, ????????, ????? ?????? ???? – ??? ????? ??????? ????. ??????, ????? ?? ??????????? ???? ??? ???????????? ???????? ?? ?????? ???????... real time… ? ????? ??? ??????? ????? ?????????? ? ?????? ???????? ??????, ???????? ?????-????? ????????? ???????? ?? ... ??? ????? ?????? ???????.
?????? ? ??????????? ??????? ? ?? ????????, ??? ? ???????? «???????? ???????????» ??????????? ?????? ??????????? ????????. Well… ?? ?????? ???????????... ? ??? ?? ??? ???????... «??? ??? ????»! ??, ??????. ????? ????????? ?? ???????.
???????? ???????? ?????? ??????, ??? ???????? ???????? ???????? ??????????? ??????, ?? ???-????, ?????? ?????????, ??? ??? ???????. ????, ??????????? ????, ??? ? ??? ???? ????? ?????????????????(), ??????? ?????????? ? ??????, ????? ????????? ???????? ????? ?????, ? ??????? ????????, ??? ?? ?????? ?????? ???????? ?????????? ?????. ? ??? ?? ????????, ??? ???? ?? ????? ?????? ??????? ? ??? ?????-?? ?????????? ????????? ???. ???, ????? ???????????, ??? ??? ?? ?????????? ???????????, ???? ?? ?? ?? ???????? ?? ? ?????????????????(), ? ? ??????????????????????(). ??, ??? ????????, ????????? ????????????. ?? ? ??????????, ? ??? ????? ?? ???? ?????:
1. ?????? ????????? ??? ?? ?????????????????() ? ??????????????????????(). ??? ????? ?????????. ??????????? ???????? ????????? ????? ???????????? ????-???? ???? ? ?????.
2. ??????? (1) ? ????????????? ?????????????????() ? ????????????????????????????????(), ???, ??????????, ??? ??????? ? ????????, ??? ????? ???????????? ???????? ?????? ??? ? ????????. ??????????? ??????? ??????-?????? ??????.
??????, ?? ?????????, ???? ??????? ????????????? ??????. ?????? ???? ????? ???????? code review (??????? ????) ??????, ??? ?????? (1) ??? ??? ?????? (2)? ???. ??, ??. ?????. 1. ??? ??????? (2), ????? ?????? ???????? ?? ?????? ?????????? ?????? ? ??? ?? ??????? ? ??????. ?????? ?? ??????? ???????. ? ??? ?????????? ?? ? ???? ??????? ??? ????-??-??????? ?? ????? ?????? ? ?????? ? ???????, ?????? ???-???.
? ????? ?????????? ????????? ? ???, ??? ??? ??????? ??????? ????????, ? ?? ???????????. ? ????? ?????? ??????, ???-?? ?????? ????? ????????? ? ???? ????? ?????????????????(), ? ?? ?????? ??????? ????????? ???-???????... ??????, ???? ????? ??, ? ?? ? ??????...
?? ????, ??????, ??? ??? ????????? ? ? ??????? ? ???-?? ????????? ??????, ??? ??? ??????????
Comments
Anonymous
January 01, 2003
А-а-а, вот о чем. Проблема в том, что перетасовывание кода - это и есть тот самый рефакторинг, который трудно делать. Менеджменту нужны фичи и не инженерное мумбо-юмбо. А людей и так перетасовывают постоянно, что только держись, причем по совершенно другим причинам и критериям.Anonymous
January 01, 2003
Ну, да, а комментарии TODO в 20 файлах хоть как-то изменят ситуацию по сравнению с изменением названия в 20 файлах? А главное - моя забота не что делать мне, бедному, а что, блин, мои коллеги будут делать в такой ситуации? До вас еще не дошло? Я этот блог пишу с точки зрения менеджера проекта, а не участника. Как участник я и сам знаю что делать, и все это делаю. Меня интересует с чем столкнется мой менеджер, учитывая, что не все такие умные как я (скромно похлопывая себя по плечу :-) :-) :-) ) или вы!Anonymous
January 01, 2003
Иван: :-) Би: я предлагаю ответственность без писем, которая и так все равно должна быть. И без дешевого выпендрежа, вроде вашего. Под такими лозунгами перестройку делали...Anonymous
January 01, 2003
The comment has been removedAnonymous
January 01, 2003
The comment has been removedAnonymous
August 23, 2008
>А каков результат? Результат в том, что ваш продукт получит заплатку, а не рефакторинг. И через неделю другую, кто-то другой опять вляпается в этот самый ЗагрузкаЗакончена(), и вы будете править следующий баг-близнец... Хорошо, если такой же, а то и похуже... Что мешает при создании заплатки, добавить в комментарии TODO с объяснением, что этот кусок надо рефакторить с объяснением почему?Anonymous
August 23, 2008
поржал. это не проблема code review, это проблема "сначала делаем, потом думаем" и как следствие неправильной организации труда. пишите тесты к архитектуре, показывающие, как правильно вашим кодом пользоваться. дополняйте список обратных несовместимостей интерфейсов, если пришлось что-нибудь поменять. пишите @deprecated на функции, которые не рекомендуется использовать. а code review вам помог не сделать ещё больших глупостей -- не коммитить необдуманный код.Anonymous
August 23, 2008
Получается проблемма только в том, что нельзя сделать коммит, пока код не прошел проверку? Помоему достаточно в такой ситуации создавать отдельный бранч в свн для непроверенного кода и коммитить туда.Anonymous
August 23, 2008
А что тут такого? О том, что в таких случаях не время заниматься рефакторингом еще господин Фаулер в "Refactoring: Improving the Design of Existing Code" писал, если мне память не изменяет. Тут надо, IMHO, например, комментарии TODO вставлять о том, что будет сделано (2), что должно исключить повтора ситуации.Anonymous
August 24, 2008
Проблема, безусловно, есть. Если хотя бы часть ревьюеров будет пинать за использование заплатки вместо рефакторинга, это помогает.Anonymous
August 24, 2008
Абсолютно. Я несколько раз сабмитил код без ревью, поскольку ревью большого кода никто не хотел делать. Фактически, раза три-четыре. Именно столько, сколько было возможностей что-то переписать. Как несколько строчек, так завсегда пожалуйста. А как большой серьёзный кусок, так нет. И правильно, в принципе. В нём же разбираться надо почти столько же сколько я над ним думал и писал. А разбираться в чужом коде никто не любит даже если время есть. А с багами, я это называю пофиксить добавив код и пофиксить сократив. Очень часто имеем такой странный выбор. Очевидно, что лучше выкинуть код, чем добавить. Но добавить проще и, главное, безопаснее. Баг в коде? Добавим лишнюю проверку снаружи. Проблемы с многопоточностью? Добавим критических секций. Да хоть бы и одну, статическую, во все методы. Производительность потом пофиксим, когда разберёмся. В следующей версии. Когда или шах, или осёл, или сам в другую команду.Anonymous
August 24, 2008
Может, стоит использовать "хирургические бригады"? Чтобы один кусок кода писали от силы двое разработчиков, они уж друг с другом договорятся. А кривые интерфейсные функции нужно депрекатить как можно раньше, вот и всё. 20 файлов используют функцию? Подумаешь, пожар в урне, намылить уведомление разработчикам и забыть. Это же ваши файлы. Вот если мне вздумается так похулиганить с библиотекой, которая лежит на каждом втором маке в мире -- меня уволят раньше, чем правка дойдёт до релиза, и не посмотрят на былые заслуги; и правильно сделают :)Anonymous
August 24, 2008
Задача менеджера заключается в данной ситуации в том, чтобы все, кто метод ЗагрузкаЗакончена() правят или используют, а так же те, кто участвуют в code review ИХ кода (кода тех, кто правит и использует метод ЗагрузкаЗакончена() - во завернул!), учитывали Ваше TODO к методу ЗагрузкаЗакончена(). TODO вы вставляете в один файл, благо современные инструменты (включая IDE) позволяют без проблем видеть такие TODO к методу в местах ВЫЗОВА метода.Anonymous
August 25, 2008
> Иван: не понял, о чем это вы? Если переименование одной несчастной внутренней функции превращается в проблему, значит разработчиков и/или код надо сгруппировать как-то по другому. Возможно, тогда они будут и чуть-чуть меньше заняты. Возможно, тогда они смогут решить и более серьёзную проблему. (Перетасовывание бригады, конечно, не помогает, когда просто надо заменить Мартышку, Осла, Козла и Мишку на более умных разработчиков, но к известным мне проектам Мелкософта это не относится, ибо служба борьбы с персоналом у вас нормальная)Anonymous
August 26, 2008
> Менеджменту нужны фичи и не инженерное мумбо-юмбо. Если Большой Менеджер не понимает, что энтропия растёт, и иногда нужно пылесосить код, то техлидер должен ему это разъяснить, и ещё на этапе планирования выбить под это дело дырки в ганнттограмме. Самое простое -- заранее объявить, что в пятницу вечером (и в выходные :) каждый разработчик может делать то, что сам считает нужным.Anonymous
August 31, 2008
The comment has been removedAnonymous
September 04, 2008
Вы предлагаете что бы он без ревью зачекинил этот самый рефакторинг из дюжины-другой файлов? Нет времени у других на ревью? Ну дык это проблема не с code reviews, а с другим. Пишите Менеджеру письмо, мол так и так, вы возьмете на себя ответственность за то что я сделаю так как делать не надо? Ну и в копию менеджера вашего менеджера.Anonymous
October 10, 2008
Да, проблема очень известная, пример характерный. Позвольте мне добавить и обсудить те идеи/решения к которым пришел я.
- Codereview необходим. Это не только выявление потенциальных ошибок, но и защита от уникального владения кодом одного разработчика(автора).
- Coderoview должен быть встроен в workflow. А это значит, что ревизия кода должна проводится не по желанию "кем-получится" и "когда-получится", а в четко установленном порядке. EldarM: перестройка тут ни при чём. "Ответственность без писем" возможно только тогда, когда у договаривающихся а)безупречная память; б)они доверют друг другу 100%; и в) эта ответственность не перекладивается в будущем на кого-то третьего. Незнаю как у вас, а наша комманда не проходит по первому пункту. Поэтому, либо каждый автор должен четко знать, кто обязан(!) проверять его код. А ответственность за код переходит как раз на ревьювера.
- Codereview не должен снижать скорость разработки. Девелопмент вклячает в себя: а. исследование задачи/проблеммы; b. поиск решения; c. имплементация; Ревью проверяет только b. и с. Поэтому надо исходить из того, что время разработки/фикса всегда больше времени затрачиваемому на то, чтобы этот код прочитать и вникнуть. Если происходит наоборот, значит при девелопменте заведомо были упущены какие-то моменты, которые необходимо было происследовать. Плюс ко всему, codereview должен избавить от явных ошибок в поиске решения и написании кода, что позволит избежать временных затрат на исправление в будущем. Из утверждения 3 следует, что, если по предварительным оценкам, инспекция кода будет занимать время более трети времени затраченному на написание этого кода, то необходимо совместное участие автора и ревьювера при:
- поиске решения и имплементации (дополнительный codereview ненужен)
- самом codereview, где автор комментирует свои решения. Все ошибки, найденные при codereview, отправляются автору, который, после исправлений, отправляет свои изменения на очередной codereview. PS/ когда рабочий процесс налажен, "правильный" codereview не составляет проблемы. Все это ускоряется, когда есть возможность почаще привлекать разработчиков к совместной разработке/инспекции кода.