CodeReview. Теория и практика.
От: kaa.python Ниоткуда РСДН профессионально мёртв и завален ватой.
Дата: 03.12.10 17:30
Оценка: 96 (11) +2
CodeReview – классная штука, не правда-ли? Если отвлеченно подумать об открывающихся возможностях и количестве потенциальных ошибок, найденных при проведении CodeReview, то начинает казаться что практика просто великолепна и мега полезна.
Обычно, путь к внедрению этой практики извилист и тернист, но ништяки, которые по легенде ожидают прошедшего, заставляют забыть о сложной дороге.
Как ни странно, первым делом, обычно, создается стандарт кодирования. Думаю, это вызвано тем что любая уважающая себя компания должна иметь собственный стандарт. В стандарте обязательно нужно прописать где ставить скобочки, а где пробелы, какие индентификаторы писать с прописной буквы, а какие со строчной. Стандарт просто необходимо согласовать, проревьювить раз надцать и утвердить. После этого весь код будет однородным и очень красивым, не зря же стандарт составляли лучшие из лучших компании!
К сожалению, после появления стандарта возникает ощущение незавершенности. Т.е. стандарт вроде и создали, но разработчики положили на него болт. И вот тут, кто-то наиболее начитанный вспоминает магическое слово CodeReview. Причем все понимают что это самое ревью не только охреневших разработчиков на место поставит, но еще и избавит от кривых алгоритмов, защитит от утечек памяти и, как говорят гуру CodeReview даже спасет от Экрана Смерти (ну либо от пиники ядра, это кому как повезет). Осознав все это, просто нельзя не сделать дружного рывка для внедрения столь необходимой практики.
Реализация процесса CodeReview обычно варьируется от компании к компании, но при этом общие черты остаются. Все максимально автоматизируется, на версионное хранилище вешается как можно больше триггеров, разыскиваются наиболее навороченные инструменты. Чисто теоретически, благодаря этому качество кода будет несравненно лучше, а код не соответствующий стандарту кодирования в репозиторий больше не попадет. Разве не прелесть?
Только вот реали, к сожалению, сильно отличаются от теории. Само собой, техническая сторона этого вопроса вносит наименьше количество проблем. Все необходимые инструменты уже написаны, а документацию по внедрению найдет даже ленивый. Проблему тут вносят люди, психологический аспект.
Начинается все с того, что ревьювер неожиданно для себя выясняет что выискивать на ревью реальные проблемы в коде очень тяжело, не всем дано да и тупо влом. А если вспомнить о том, сколько дел ждет… Ведь куда проще заняться поиском опечаток, лишних заголовков, несоответствий стандарту. Тем более что автоматическая система все стерпит, занес туда баг и хорошо, долг-то выполнен. Правда при этом 5-10% рабочего времени ревьювера и автора кода тратится на откровенную херню, но кого бы это волновало? Ведь практика как-бы внедрена, баги есть, и можно даже кого-нить наградить/наказать по результатам. Ну а то что реального выхлопа от этого практически никакого, так в этом участники ревью виноваты и как следствие ответ на один из извечных русских вопросов найден.
Хотя, в одной из компаний где мне довелось работать, я видел нормально поставленный процесс CodeReview. Проходил он потрясающе просто, без каких-либо технических ухищрений, систем контроля и и прочих плодов технического прогресса. Когда возникала необходимость в проведении ревью (например, собирались коммитить критичный код), автор и еще пара разработчиков собирались за одним компьютером и вместе пристально рассматривали новорожденный шедевр. Что интересно процессе обсуждения находили почти все закравшиеся косяки, ведь не будешь же живого человека исключительно неправильно расставленными скобками доставать, он же и нахер послать может. Да и о времени потраченном на ревью жалеть поздно, итак уже оторвался от рабочего места.
Все же основная ошибка, которая допускается при внедрении CodeReview – это игнорирование психологии людей. Общение с человеком напрямую, и заполнение страницы в сети воспринимаются людьми совершенно по-разному. Никогда нельзя забывать об этом.
Re: CodeReview. Теория и практика.
От: Carc Россия https://vk.com/gosha_mazov
Дата: 03.12.10 17:38
Оценка:
KP>Хотя, в одной из компаний где мне довелось работать, я видел нормально поставленный процесс CodeReview. Проходил он потрясающе просто, без каких-либо технических ухищрений, систем контроля и и прочих плодов технического прогресса. Когда возникала необходимость в проведении ревью (например, собирались коммитить критичный код), автор и еще пара разработчиков собирались за одним компьютером и вместе пристально рассматривали новорожденный шедевр. Что интересно процессе обсуждения находили почти все закравшиеся косяки, ведь не будешь же живого человека исключительно неправильно расставленными скобками доставать, он же и нахер послать может. Да и о времени потраченном на ревью жалеть поздно, итак уже оторвался от рабочего места.
Имхо, вышеуказанное и есть CodeReview — а все эти "скобочки, возведенные в абсолют" это ересь балласта, которым явно больше нечем заняться в команде.
Нет-нет, я не спорю — в стандартах кодирования есть свой толк и смысл, ну кто спорит что
a != 0
//банально читабельно лучше, чем слитно
a!=0

И вообще, в определенных правилах кодирования потребность и смысл есть безусловно. Но если вся эта "бюрократия" начинает мешать реальному пользе и качеству кода, то зачем такие правила!?!
Aml Pages Home
Re: CodeReview. Теория и практика.
От: mrTwister Россия  
Дата: 03.12.10 18:14
Оценка: +7
Здравствуйте, kaa.python, Вы писали:

Не понимаю, какие проблемы с неправильными скобочками и codestyle — они прекрасно отлавливаются автоматически специальными тулзами.
По поводу codereview — если в вашей команде не умеют их проводить, это еще не значит, что их никто не может проводить. У нас ревью работают неплохо. И реальные баги находятся регулярно, и велосипеды отсеиваются на ранних стадиях и знания шарятся и пр. Но нужно понимать, что ревьючить код — это отдельный навык. Надо уметь это делать и тренировать в себе этот навык. А по началу да, было тяжело. Было все, что ты описал.
лэт ми спик фром май харт
Re: CodeReview. Теория и практика.
От: Sinix  
Дата: 03.12.10 18:20
Оценка: 36 (3)
Здравствуйте, kaa.python, Вы писали:

Что ж, а теперь я поделюсь, как может проходить Code Review.

Для c# все грабли со стилем/практиками кодирования решаются двумя правилами:
1. Не делать отступлений от общепринятых стандартов, пока они нам не мешают.
2. Не вводить правил, которые не могут быть проверены/соблюдены автоматически.

Стиль кодирования? 90% — автоформатирование студии + 9% — CodeMaid + остаток вылавливается StyleCop'ом. Первые 2 отрабатывают при сохранении файлов, StyleCop запускается под настроение, в основном — перед коммитом, т.к. находит максимум 2-3 мелких косяка на солюшн.

Дизайн кода? Обязательное изучение Framework Design Guidelines + Code Analysis / FxCop с кастомными правилами. Code Analysis также запускается под настроение, в основном по завершению очередной фичи.

Юнит-тесты пишутся в основном как proof-of-concept для API + проверка соответствия реализации ожиданиям.
Практически весь код покрыт самописными ассертами; очень много ошибок отлавливается до того, как могут повлиять на состояние системы. Большая часть логики — stateless, особенно в вспомогательных библиотеках. Как итог, вместо того чтоб споткнуться и сдохнуть — регулярно приземляемся на все 4 лапы

Наконец, идёт вялотекущий переход на static сhecker от CodeContracts (с переиспользованием уже написанных ассертов). Вяло идёт не потому что не надо, наоборот! Увы, в текущей реализации контракты практически невозможно поддерживать; static checker требует покрытия контрактами каждого метода между публичным API и собственно логикой. Но даже в таком виде при первой же попытке внедрения нашлось очень много неожиданных моментов. Ждём и продолжаем капать на мозги саппорту на оффоруме.




А вот теперь, после того как мы освободились от нудятины, мы с чистой совестью можем использовать code review для полезной деятельности: свежим взглядом пробежаться по коду и увидеть, как можно его улучшить/дополнить для поддержки ближайших юз-кейзов. Как бонус — после code review свежим взглядом смотришь уже на код по текущей задаче.
Ты просто не умеешь их готовить
Re[2]: CodeReview. Теория и практика.
От: Andy77 Ниоткуда  
Дата: 03.12.10 19:21
Оценка:
Здравствуйте, Sinix, Вы писали:

S>... Ждём и продолжаем капать на мозги саппорту на оффоруме.


Тебе же там ответили уже, что Code Contracts нигде не полагается на то, что написанный код следует каким-то общепринятым паттернам или FDG, чего ты от них хочешь добиться?
Re: CodeReview. Теория и практика.
От: minorlogic Украина  
Дата: 03.12.10 20:30
Оценка:
Необходимо четко поставить цели код ревью. Вот и все.
... << RSDN@Home 1.2.0 alpha 4 rev. 1237>>
Ищу работу, 3D, SLAM, computer graphics/vision.
Re[3]: CodeReview. Теория и практика.
От: Sinix  
Дата: 04.12.10 05:44
Оценка:
Здравствуйте, Andy77, Вы писали:

A>Тебе же там ответили уже, что Code Contracts нигде не полагается на то, что написанный код следует каким-то общепринятым паттернам или FDG, чего ты от них хочешь добиться?


Очевидно, [ContractTransparent]

Основной смысл static checker'а — предупредить о нарушении предусловий, приводящему к исключению в рантайме. Очень часто метод A не может сам соблюсти все предусловия; он всего лишь передаёт аргументы дальше — в методы B-Z. Для того, чтобы передать ответственность вызывающему коду, предлагается скопировать все проверки из B-Z, и обновлять этот копипаст по мере изменения любого из методов. Увы, B-Z сами вызывают другие методы — идём по второму кругу.

Затраты на копипаст намного превышают затраты на написание юнит-тестов, благо для последних есть Pex. Что полезного остаётся от static checker'а?

В итоге, или мы чуть жертвуем абстрактной чистотой идеи, или получаем прекрасную в своей бесполезности сферофичу.
Re: CodeReview. Теория и практика.
От: MxMsk Португалия  
Дата: 04.12.10 08:50
Оценка:
Здравствуйте, kaa.python, Вы писали:

KP>Как ни странно, первым делом, обычно, создается стандарт кодирования. Думаю, это вызвано тем что любая уважающая себя компания должна иметь собственный стандарт. В стандарте обязательно нужно прописать где ставить скобочки, а где пробелы, какие индентификаторы писать с прописной буквы, а какие со строчной. Стандарт просто необходимо согласовать, проревьювить раз надцать и утвердить. После этого весь код будет однородным и очень красивым, не зря же стандарт составляли лучшие из лучших компании!

Хотел уточнить, чем не угодила практика введения стандартов кодирования?
Re[2]: CodeReview. Теория и практика.
От: kaa.python Ниоткуда РСДН профессионально мёртв и завален ватой.
Дата: 04.12.10 09:25
Оценка:
Здравствуйте, MxMsk, Вы писали:

MM>Хотел уточнить, чем не угодила практика введения стандартов кодирования?


Я разве хоть где-то написал что она мне чем-то не угодила? Я скорее не очень одобряю создание велосипедов в плане стандартов. Как ни крути, на данный момент существует море готовых стандартов — выбирай да пользуйся. Тем не менее, в большинстве случаев, люди верят что создадут что-то лучшее, все такое распрекрасное…
И все же, если просто не в моготу как хочется создать собственный стандарт, он должен базироваться на текущей кодовой базе, т.к. с ней жить и дальше. Но и этого обычно не делается, а исходят из чувства прекрасного: "так красивее", "так нагляднее", и совсем абсурдного пункта "так правильнее".
Re[3]: CodeReview. Теория и практика.
От: Sinix  
Дата: 04.12.10 09:35
Оценка: +1
Здравствуйте, kaa.python, Вы писали:

KP>Тем не менее, в большинстве случаев, люди верят что создадут что-то лучшее, все такое распрекрасное…

KP> ... Но и этого обычно не делается, а исходят из чувства прекрасного: "так красивее", "так нагляднее", и совсем абсурдного пункта "так правильнее".

А при чём здесь code review?
Re[4]: CodeReview. Теория и практика.
От: kaa.python Ниоткуда РСДН профессионально мёртв и завален ватой.
Дата: 04.12.10 09:39
Оценка:
Здравствуйте, Sinix, Вы писали:

S> А при чём здесь code review?


Конкретно тут – совершенно не при чем. Я отвечал на вполне конкретный вопрос не имеющий прямого отношения к review вообще-то…
Re[5]: CodeReview. Теория и практика.
От: Sinix  
Дата: 04.12.10 09:56
Оценка: +1
Здравствуйте, kaa.python, Вы писали:

KP>Конкретно тут – совершенно не при чем. Я отвечал на вполне конкретный вопрос не имеющий прямого отношения к review вообще-то…


Так и в исходном посте сабжу было уделено примерно столько же

Но да, волшебная формула вышла :

<Подставить>. Теория и практика.

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

элементарно доказывает полную непригодность аджиля, TDD, DDD, MSF, ещё сокращений добавить по вкусу, винды, линукса, метрической системы и губораскаточных машинок.
Re: Не затронут важный аспект
От: Ytz https://github.com/mtrempoltsev
Дата: 04.12.10 10:18
Оценка: +1
Code Review — это еще и инструмент обучения и обменом опыта, будучи джуниором я сильно поднял свой уровень за счет того, что обзор моего кода проводил хороший человек и специалист, который не скупился на комментарии. Сейчас я уже сам провожу обзор кода и убедился, что самый быстрый способ ввести новичка в продуктивное русло — смотреть его код. Естественно важную роль играет человеческий фактор и верно было подмечено, что человек не умеющий смотреть чужой код, скатится до проверки расстановки скобок, но это надо учитывать и давать право смотреть код людям подходящим для этого.
Re[2]: Не затронут важный аспект
От: kaa.python Ниоткуда РСДН профессионально мёртв и завален ватой.
Дата: 06.12.10 08:23
Оценка:
Здравствуйте, Ytz, Вы писали:

Ytz>Code Review — это еще и инструмент обучения и обменом опыта, будучи джуниором я сильно поднял свой уровень за счет того, что обзор моего кода проводил хороший человек и специалист, который не скупился на комментарии. Сейчас я уже сам провожу обзор кода и убедился, что самый быстрый способ ввести новичка в продуктивное русло — смотреть его код.


Мне думается, что этот аспект прилично работает только при личном контакте, т.е. когда можно все на месте обсудить и на пальцах рассказать. Автоматизированные средства врятли принесут хороший эффект.
Re[3]: Не затронут важный аспект
От: Ytz https://github.com/mtrempoltsev
Дата: 06.12.10 09:17
Оценка:
Здравствуйте, kaa.python, Вы писали:

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


Мой опыт говорит о другом. Например, однажды будучи джуниором написал примерно следущее:

std::auto_ptr<char> buffer(new char[BUFFER_SIZE]);


На что получил простой комментарий: "Use boost::scoped_array", так что характерно — до сих пор помню
 
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.