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

Стандарты инспекции кода

Основная цель инспекции кода в Google – добиться того, чтобы общая работоспособность кодовой базы со временем возрастала. Все инструменты и процессы, из которых складывается процедура, подбираются исходя из этого. Чтобы добиться намеченной цели, необходимо выдержать определенный баланс компромиссов.

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

С другой стороны, тот, кто проводит инспекцию, берет на себя обязанность следить за качеством каждого списка изменений – оно должно быть таким, чтобы работоспособность кода в долгосрочной перспективе не падала. Это часто оказывается непростым делом: обычно кодовая база портится постепенно, качество снижается от одного упущения к другому. Особенно это характерно для команд, которым приходится работать в жёстких временных рамках – им представляется, что срезать углы необходимо, чтобы достичь нужных результатов.

К тому же проводящий инспекцию несет ответственность за код, который ему передали. Он обязан удостовериться, что в кодовой базе не появится никаких неувязок, что код можно будет поддерживать – в общем, во всем, о чем говорится в разделе «На что смотреть при инспекции кода».

Таким образом, в качестве стандарта, на который нужно ориентироваться при инспекции, мы выводим следующее правило:

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

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

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

Тем не менее, рецензент всегда имеет право оставить комментарий с указанием на то, что можно улучшить в коде. Однако если это не имеет критического значения, лучше предварить его какой-нибудь пометкой (например, Nit — nitpick, придирка), чтобы автор понял, что это просто рекомендация по шлифовке, на которую необязательно реагировать.

Примечание: ничто из сказанного в этом руководстве не оправдывает тех, кто пропускает изменения, которые определенно вредят общему состоянию кода в системе. Единственное, чем это можно извинить – экстренная ситуация.

Обучение

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

Принципы

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

Разрешение конфликтов

В любом конфликте, который возникает в процессе инспекции, автор и рецензент должны первым делом попытаться прийти к согласию своими силами. Рекомендации на этот счет, кроме данного текста, можно найти в статьях The CL Author’s Guide и Reviewer Guide.

Когда достигнуть согласия оказывается особенно сложно, разрешению конфликта может помочь личная встреча участников в противовес обмену комментариями. Если вы применяете этот метод, не забудьте в паре слов обобщить ваш консенсус в комментарии к коду – это пригодится будущим читателям.

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

На что смотреть при инспекции кода

Примечание: обязательно следите за соблюдением названных выше стандартов при работе с каждым из этих аспектов.

Архитектура

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

Функциональность

Действительно ли список изменений делает то, что предполагал разработчик? Станет ли пользователям, для которых пишется код, лучше от этой задумки? Под пользователями мы в общем случае понимаем как конечного пользователя (если изменения как-то скажутся на его опыте), так и разработчиков, которым придется работать с кодом в будущем.

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

В некоторых случаях вы можете одобрить список изменений – важнее всего отслеживать, в какое поведение он выливается, тогда, когда дело касается элементов, обращенных к пользователю, вроде корректировок в интерфейсе. Рассматривая изменения такого рода, вы можете попросить у разработчика, чтобы он продемонстрировал, как все работает, если делать патч и смотреть самому неудобно.

Другой случай, когда при инспекции особенно важно думать о функциональности – код, который предполагает параллельное программирование и теоретически может вызвать взаимную блокировку или состояние гонки. Подобные проблемы очень сложно выявить простым запуском кода, обычно нужно, чтобы кто-то (то есть и разработчик, и рецензент) хорошо все продумали и прикинули, не приведут ли изменения к проблемам. Нужно заметить, что разумнее вообще не использовать модели с параллелизмом в случаях, когда есть риск взаимной блокировки или состояния гонки – они сильно усложняют и понимание, и проверку кода.

Сложность

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

Одну частную разновидность усложнения кода называют over-engineering – применение слишком обобщенных решений или внедрение функциональности, которая в данный момент в системе не нужна. На этот счет нужно проявлять особенную бдительность. Подталкивайте разработчиков к решению тех проблем, которые стоят перед ними прямо сейчас, а не тех, которые, возможно, возникнут в будущем. За эти будущие проблемы следует браться по мере поступления, когда они обретут конкретную форму в материальном мире и трансформируются в конкретные требования.

Тесты

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

Убедитесь, что все тесты, корректны, разумны и полезны. Тесты сами себя не тестируют, а тесты для тестов мы пишем редко, поэтому следить за тем, чтобы тесты были приемлемого качества, должны люди.

Точно ли будет завален тест, если что-то в коде сломается? Если в код, с которым он работает, внесут изменения, не приведет ли это к ошибкам первого рода? Прописаны ли для всех тестов простые и целесообразные утверждения? Проведено ли разделение по методам тестирования?

Помните, что тесты – тоже код и тоже нуждаются в поддержке. Не допускайте лишней сложности в тестах только потому, что они не относятся к main binary.

Названия

Удалось ли разработчику подобрать для всего правильные названия? Хорошее название достаточно длинное, чтобы в полной мере отразить, что представляет собой тот или иной элемент и для чего он нужен, но не настолько, чтобы его тяжело было воспринимать.

Комментарии

Оставлял ли разработчик комментарии, написанные удобочитаемым языком? Все ли они реально необходимы? Как правило, комментарии призваны объяснить, зачем нужен какой-то фрагмент кода, а не рассказывать, что именно он делает. Если последнее не самоочевидно просто из чтения, значит, код нужно упрощать. Существует ряд исключений (скажем, регулярные выражения и сложные алгоритмы только выигрывают от пояснений об их назначении), но в целом комментарии рассчитаны на те случаи, когда необходимую информацию никак невозможно прописать прямо в коде – например, почему было принято определенное решение.

Полезно бывает также просмотреть комментарии, которые добавлялись до последних изменений. Возможно, там обнаружится TODO, которое теперь уже можно удалить, или доводы против некоторых изменений.

Учитывайте, что комментарии – не то же самое, что документация классов, модулей или функций. В документации формулируется назначение определенного фрагмента кода, прописывается, как им следует пользоваться и какие события должны при этом происходить.

Стиль

У компании Google есть руководства по стилю для всех распространенных языков, да и для большей части не слишком распространенных тоже. Удостоверьтесь, что в списке изменений все соответствует правилам, прописанным в руководстве по стилю вашей команды.

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

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

Документация

Если в результате изменений пользователи должны будут работать с кодом – править его, тестировать, релизить, как-то еще взаимодействовать – иначе, чем раньше, проверьте, чтобы соответствующая документация была обновлена, в том числе README, g3doc и другие справочные документы. Когда какие-то фрагменты кода удаляются или объявляются нежелательными к использованию, посмотрите, не нужно ли убрать что-то и из документации. Если в документации чего-то не хватает, отправьте запрос на дополнение.

Каждая строка

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

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

Если в коде вам все понятно, но вы не чувствуете себя достаточно компетентным, чтобы оценить отдельные части, позаботьтесь о том, чтобы список проинспектировал еще кто-нибудь, кто хорошо разбирается в нужных темах. Особенно это важно, если речь идет о сложных вопросах, связанных с безопасностью, параллелизмом, доступностью, локализацией и так далее.

Контекст

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

Также неплохо бы задуматься, как предлагаемые изменения встраиваются в общую систему. Улучшают ли они функциональность кода или усложняют его, делают более непредсказуемым и так далее? Не принимайте изменения, которые ухудшают состояние системы в целом. В большинстве систем путаница складывается из множества мелких изменений, которые постепенно накапливаются, так что важно вылавливать даже мелкие небрежности в каждом новом списке.

Удачные решения

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

Если обобщить, при инспекции кода вам нужно обращать внимание на следующие вещи:

• Все ли в порядке с архитектурой
• Полезна ли функциональность пользователям
• Разумны и привлекательны ли изменения в интерфейсе
• Не приведет ли к проблемам параллельное программирование
• Не слишком ли сложно написан код
• Не занимается ли разработчик тем, что, возможно, будет актуально в будущем, но не нужно сейчас
• Есть ли необходимые юнит-тесты
• Хорошо ли спроектированы тесты
• Для всего ли подобраны понятные названия
• Отличаются ли комментарии ясностью и информативностью, объясняют ли они смысл кода вместо того, чтобы его описывать
• Имеется ли вся необходимая документация (как правило, в виде документов g3doc)
• Соблюдены ли правила из руководства по стилям

Как работать со списком изменений

Теперь, когда вы знаете на что обращать внимание, рассмотрим наиболее эффективный метод рецензирования списка изменений, который охватывает несколько файлов.

1. Понятен ли смысл списка изменений? Есть ли у него внятное описание?
2. Для начала изучите самые ключевые части списка. Хорошо ли он спроектирован в целом?
3. Просмотрите остальные части списка в нужной последовательности.

Шаг первый: Составьте общее представление об изменениях

Посмотрите на описание изменений и на их предназначение в целом. Есть ли во всем этом какой-то смысл? Если подобные изменения не нужны в принципе, немедленно свяжитесь с автором и объясните, почему этого делать не нужно. Когда вы отклоняете изменения на таких основаниях, неплохо бы также предложить разработчикам альтернативные варианты, которые они могли бы взять в работу.

Например, вы можете сказать что-то в духе: «Слушай, я вижу, что ты серьезно над этим работал, спасибо. Но на самом деле, мы сейчас идем к тому, чтобы отказаться от системы FooWidget, которую ты здесь модифицируешь, поэтому стараемся ее пока не трогать. Может, вместо этого займешься рефакторингом нового класса BarWidget?».

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

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

Шаг второй: Изучите ключевые части списка изменений

Найдите тот файл или группу файлов, которые являются «главной» частью списка. Зачастую есть один файл, который содержит в себе наибольшее количество логических изменений и является ключевым. Сосредоточьтесь сначала именно на этих основных частях. Это поможет сформировать контекст для более мелких фрагментов и ускоряет процесс инспекции в целом. Если список настолько обширен, что вам сложно разобраться, какие части следует считать основными, обратитесь к разработчику либо с вопросом, с чего лучше начать, либо с просьбой разбить список на несколько.

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

Отправлять подобные отзывы безотлагательно необходимо по двум причинам:

1. Часто бывает так, что разработчик отсылает список изменений и, пока ждет отклика, тут же приступает к другой работе, которая опирается, в том числе, и на эти изменения. Если в коде, который вы рассматриваете, обнаруживаются проблемы на уровне архитектуры, то ему придется впоследствии что-то менять и в написанном позже. Лучше остановить его, пока он не надстроил слишком много на сомнительной архитектуре.
2. Основательные изменения занимают больше времени, чем мелочи. Почти всем разработчикам ставят дедлайны; чтобы их соблюдать и при этом выдавать качественный код, они должны иметь возможность браться за крупные правки как можно раньше.

Шаг третий: просмотрите остальные части списка в нужной последовательности

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