Преглед на кода Най-добри практики

Интернет предоставя богат материал за прегледи на кодове: за ефекта на кодовите прегледи върху фирмената култура, за официалните прегледи на сигурността, по-кратките ръководства, по-дългите контролни списъци, хуманизираните отзиви, причините за извършване на прегледи на кодове на първо място, най-добрите практики и още най-добрите практики, статистически данни за ефективността на преглед на код за улавяне на бъгове и примери за прегледи на кодове се объркаха. О, и разбира се има и книги. Накратко, тази публикация в блога представя отзивите на Palantir за кодове. Организации с дълбоко културно нежелание за партньорски проверки може да искат да се консултират с отличното есе на Карл Е. Вигерс за хуманизиране на партньорските оценки, преди да се опитат да следват това ръководство.

Тази публикация е копирана от ръководствата за най-добри практики на нашата верига инструменти за качество на Java Code, Baseline и обхваща следните теми:

  • Защо, какво и кога да се правят прегледи на кодове
  • Подготовка на код за преглед
  • Извършване на кодови прегледи
  • Примери за преглед на код

мотивиране

Ние извършваме прегледи на кодове (CR), за да подобрим качеството на кода и да се възползваме от положителните ефекти върху културата на екипа и компанията. Например:

  • Комитетите се мотивират от идеята за набор от рецензенти, които ще разгледат искането за промяна: комисионерът е склонен да почисти свободни цели, да консолидира TODO и като цяло подобри ангажимента. Разпознаването на експертни познания за кодиране чрез връстници е гордост за много програмисти.
  • Споделянето на знания помага на екипите за развитие по няколко начина:
    - CR изрично съобщава добавена / променена / премахната функционалност на членовете на екипа, които впоследствие могат да надграждат извършената работа.
    - Комитетът може да използва техника или алгоритъм, от които рецензенти могат да се научат. По-общо, прегледите на кодове помагат да се повиши лентата за качество в цялата организация.
    - Рецензентите могат да притежават знания за техниките на програмиране или кодовата база, които могат да помогнат за подобряване или затвърдяване на промяната; например, някой друг може едновременно да работи върху подобна функция или поправка.
    - Положителното взаимодействие и комуникация засилва социалните връзки между членовете на екипа.
  • Последователността в кодовата база прави кода по-лесен за четене и разбиране, помага за предотвратяване на грешки и улеснява сътрудничеството между редовни и мигриращи видове разработчици.
  • Четивостта на кодови фрагменти е трудно да се прецени за автора, чийто мозък е дете, и е лесно да се прецени за рецензент, който няма пълния контекст. Четливият код е по-многократно използваем, без грешки и бъдещи доказателства.
  • Случайните грешки (напр. Печатни грешки), както и структурните грешки (напр. Мъртъв код, грешки в логиката или алгоритъма, проблеми с производителността или архитектурата) често са много по-лесни за откриване на критични рецензенти с външна перспектива. Проучванията са установили, че дори кратките и неофициални прегледи на кодове оказват значително влияние върху качеството на кода и честотата на грешките.
  • Спазването и регулаторната среда често изискват прегледи. CR са чудесен начин да избегнете общи капани за сигурност. Ако функцията или средата ви имат значителни изисквания за сигурност, ще се възползвате (и вероятно изисквате) преглед от местните криминалисти по сигурността (ръководството на OWASP е добър пример за процеса).

Какво да прегледаме

Няма вечно верен отговор на този въпрос и всеки екип за развитие трябва да се съгласи на своя собствен подход. Някои отбори предпочитат да преразглеждат всяка промяна, обединена в основния клон, докато други ще имат праг на „тривиалност“, под който не се изисква преглед. Компромисът е между ефективно използване на времето на инженери (както автори, така и рецензенти) и поддържане на качеството на кода. В определени регулаторни среди може да се изисква преглед на кода дори за тривиални промени.

Прегледите на кодове са безкласни: това, че сте най-възрастният човек в екипа, не означава, че кодът ви не се нуждае от преглед. Дори ако в редкия случай кодът е безупречен, прегледът предоставя възможност за менторство и сътрудничество и, минимално, диверсифицира разбирането на кода в кодовата база.

Кога за преглед

Прегледите на кодове трябва да се извършват след като автоматичните проверки (тестове, стил, други потребителски интерфейси) приключат успешно, но преди кодът да се слее с основния клон на хранилището. Обикновено не извършваме официален преглед на кода на съвкупните промени след последната версия.

За сложни промени, които трябва да се слеят в основния клон като едно цяло, но са прекалено големи, за да се поберат в един разумен CR, помислете за подреден модел CR: Създайте първичен клон / big-характеристика и редица вторични клонове (например, функция / big-feature-api, feature / big-feature-testing и др.), които всеки капсулират подмножество от функционалността и които се преглеждат поотделно на кода спрямо клона на функцията / big-feature. След като всички вторични клонове се обединят в характеристика / голяма функция, създайте CR за сливане на последния в основния клон.

Подготовка на код за преглед

Отговорност на автора е да представи лесни за преразглеждане CR, за да не губи времето и мотивацията на рецензенти:

  • Обхват и размер. Промените трябва да имат тесен, добре дефиниран, самостоятелен обхват, който те обхващат изчерпателно. Например, една промяна може да внедри нова функция или да поправи грешка. По-късите промени са за предпочитане пред по-дългите. Ако CR направи съществени промени в повече от ~ 5 файла или отнема повече от 1-2 дни, или ще отнеме повече от 20 минути за преглед, помислете за разделянето му на няколко самостоятелни CR. Например, програмистът може да изпрати една промяна, която определя API за нова функция по отношение на интерфейсите и документацията, и втора промяна, която добавя реализации за тези интерфейси.
  • Изпращайте само пълни, самопрегледани (по различен начин) и самопроверени CR. За да спестите време на рецензенти, тествайте изпратените промени (т.е. стартирайте тестовия пакет) и се уверете, че те преминават всички компилации, както и всички тестове и проверки на качеството на кода, локално и на CI сървърите, преди да зададете рецензенти.
  • Рефакторинг промените не трябва да променят поведението; обратно, промените, които променят поведението, трябва да избягват промените в рефакторинга и форматирането на кода. Има няколко добри причини за това:
    - Промените в рефакторинга често докосват много редове и файлове и следователно ще бъдат преглеждани с по-малко внимание. Неволните промени в поведението могат да изтекат в кодовата база, без никой да забележи.
    - Големите промени в рефакторинг нарушават магията за бране на череши, освобождаване и други магии за контрол на източниците. Много е трудно да отмените промяна в поведението, която беше въведена като част от рефакторинг-ангажимент в целия репозиторий.
    - Скъпото време за преглед на човека трябва да се изразходва за програмната логика, а не за дебатите за стил, синтаксис или форматиране. Предпочитаме да уредим тези с автоматизирани инструменти като Checkstyle, TSLint, Baseline, Prettier и т.н.

Ангажиране на съобщения

Следва пример за добро съобщение за ангажиране след широко цитиран стандарт:

Заглавие с главни букви, кратко (80 или по-малко) обобщение
По-подробен обяснителен текст, ако е необходимо. Увийте го до около 120 знака или така. В някои контексти първият
ред се третира като тема на имейл, а останалата част от текста - като основна. Празната линия, отделяща
обобщението от тялото е критично важно (освен ако не пропуснете изцяло тялото); инструменти като rebase могат да се объркат, ако стартирате
двете заедно.
Напишете съобщението си за ангажиране в императивното: „Fix bug“, а не „Fixed bug“ или „Fixes bug.“ Тази конвенция съвпада
със съобщения за ангажиране, генерирани от команди като git merge и git revert.
След празните редове идват други параграфи.
- Точките на куршума също са наред

Опитайте се да опишете какво се променя ангажимента и как го прави:

> BAD. Не правете това.
Направете компилация отново
> Добре.
Добавете jcsv зависимост, за да коригирате компилирането на IntelliJ

Намиране на рецензенти

Обичайно е комитетът да предложи един или двама рецензенти, които са запознати с кодовата база. Често един от рецензенти е ръководителят на проекта или старши инженер. Собствениците на проекти трябва да обмислят да се абонират за проектите си, за да получават известия за нови КР. Прегледите на кодове сред повече от три страни често са непродуктивни или дори контрапродуктивни, тъй като различните рецензенти могат да предложат противоречиви промени. Това може да показва фундаментално несъгласие относно правилното изпълнение и трябва да бъде разрешено извън преглед на код във форум с по-широка честотна лента, например лично или във видеоконференция с всички участващи страни.

Извършване на кодови прегледи

Прегледът на код е точка за синхронизация между различни членове на екипа и по този начин има потенциал да блокира напредъка. Следователно, кодовите прегледи трябва да бъдат бързи (подредени по часове, а не по дни), а членовете на екипа и ръководствата трябва да бъдат запознати с ангажираността във времето и съответно да определят приоритетно времето за преглед. Ако не мислите, че можете да завършите преглед навреме, моля, уведомете комисията, за да могат да намерят друг.

Прегледът трябва да бъде достатъчно задълбочен, за да може рецензентът да обясни промяната на разумно ниво на детайлите на друг разработчик. Това гарантира, че подробностите за кодовата база са известни на повече от един човек.

Като рецензент, ваша отговорност е да наложите стандартите за кодиране и да поддържате лентата за качество. Прегледът на кода е повече изкуство, отколкото наука. Единственият начин да го научите е да го направите; опитен рецензент трябва да помисли как да постави други не толкова опитни рецензенти за техните промени и да ги накара да направят преглед първо. Ако приемем, че авторът е спазил указанията по-горе (особено по отношение на самопреглед и осигуряване на изпълнение на кода), ето списък на нещата, на които рецензентът трябва да обърне внимание при преглед на код:

Предназначение

  • Този код изпълнява ли целта на автора? Всяка промяна трябва да има конкретна причина (нова функция, рефактор, грешка и т.н.). Подаденият код всъщност постига ли тази цел?
  • Задавайте въпроси. Функциите и класовете трябва да съществуват с причина. Когато причината не е ясна за рецензента, това може да е индикация, че кодът трябва да бъде пренаписан или подкрепен с коментари или тестове.

изпълнение

  • Помислете как бихте решили проблема. Ако е различно, защо е така? Кодът ви обработва ли повече (крайни) случаи? По-къса / по-лесна / по-чиста / по-бърза / по-безопасна, но функционално еквивалентна? Има ли някакъв основен шаблон, който сте забелязали, който не е заснеман от текущия код?
  • Виждате ли потенциал за полезни абстракции? Частично дублираният код често показва, че по-абстрактна или обща функционалност може да бъде извлечена и след това повторно използвана в различни контексти.
  • Мислете като противник, но бъдете приятни за това. Опитайте се да „хванете“ авторите, които правят преки пътища или липсващи случаи, като излезете с проблемни конфигурации / входни данни, които разбиват техния код.
  • Помислете за библиотеките или съществуващия код на продукта. Когато някой повторно внедрява съществуваща функционалност, по-често, отколкото не, просто защото не знае, че вече съществува. Понякога кодът или функционалността се дублират нарочно, например, за да се избегнат зависимости. В такива случаи коментарът с код може да изясни намерението. Въведената функционалност вече ли е осигурена от съществуваща библиотека?
  • Промяната следва ли стандартните модели? Създадените кодови бази често показват модели около конвенции за именуване, декомпозиране на програмната логика, дефиниции на типа данни и т.н. Обикновено е желателно промените да се прилагат в съответствие със съществуващите модели.
  • Добавя ли промяната зависимости от време на компилиране или време на изпълнение (особено между подпроекти)? Искаме да поддържаме продуктите си свободно свързани, с възможно най-малко зависимости. Промените в зависимостите и системата за изграждане трябва да бъдат внимателно проучени.

Четливост и стил

  • Помислете за вашето четене. Разбрахте ли понятията в разумен период от време? Беше ли нормален потокът и дали променливите и имената на методите са лесни за следване? Успяхте ли да следите чрез множество файлове или функции? Отблъснахте ли се от непоследователно назоваване?
  • Кодът спазва ли указанията за кодиране и стила на кода? Кодът съответства ли на проекта по отношение на стил, конвенции на API и т.н.? Както споменахме по-горе, предпочитаме да уреждаме дебатите в стила с автоматизирано инструментално оборудване.
  • Този код има ли TODO? TODO просто се натрупват в код и с времето стават неясни. Накарайте автора да представи билет за GitHub Issues или JIRA и прикачи номера на изданието към TODO. Предложената промяна на кода не трябва да съдържа коментиран код.

ремонтопригодност

  • Прочетете тестовете. Ако няма тестове и трябва да има, помолете автора да напише някои. Наистина неустановими функции са рядкост, докато непроверените реализации на функции за съжаление са често срещани. Проверете самите тестове: покриват ли интересни случаи? Четени ли са? CR понижава ли общото покритие на теста? Помислете за начините, по които този код може да се разбие. Стандартите за стил на тестовете често са различни от основния код, но все още са важни.
  • Този CR въвежда ли риск от счупване на тестов код, поставяне на стекове или тестове за интеграция? Те често не се проверяват като част от проверките преди обединяване / сливане, но намаляването им надолу е болезнено за всички. Специфични неща, които трябва да се търсят са: премахване на тестови помощни програми или режими, промени в конфигурацията и промени в оформлението / структурата на артефактите.
  • Тази промяна нарушава ли обратната съвместимост? Ако е така, добре ли е в този момент да се слее промяната или трябва да бъде натиснато в по-късно издание? Прекъсванията могат да включват промени в базата данни или схеми, промени в публичния API, промени в работния процес и т.н.
  • Необходим ли е този код за интеграционни тестове? Понякога кодът не може да бъде тестван адекватно само с единични тестове, особено ако кодът взаимодейства с външни системи или конфигурация.
  • Оставете обратна информация за документация на ниво код, коментари и съобщения за ангажиране. Излишните коментари претрупват кода и кратките съобщения за ангажиране мистифицират бъдещите сътрудници. Това не винаги е приложимо, но качествените коментари и съобщенията за ангажиране ще се изплащат сами по себе си. (Помислете за време, когато сте видели отлично или наистина ужасно, обвързващо съобщение или коментар.)
  • Актуализирана ли е външната документация? Ако вашият проект поддържа README, CHANGELOG или друга документация, актуализиран ли е той, за да отразява промените? Остарялата документация може да бъде по-объркваща от никоя и ще бъде по-скъпо да я поправите в бъдеще, отколкото да я актуализирате сега.

Не забравяйте да похвалите краткия / четим / ефективен / елегантен код. Обратно, отказът или неодобрението на CR не е грубо. Ако промяната е излишна или без значение, отхвърлете я с обяснение. Ако смятате, че е неприемливо поради един или повече фатални недостатъци, неодобрете го, отново с обяснение. Понякога правилният резултат от CR е „нека направим това по съвсем различен начин“ или дори „нека изобщо да не правим това“.

Бъдете уважителни към рецензираните. Въпреки че състезателното мислене е удобно, това не е ваша функция и не можете да вземате всички решения. Ако не можете да постигнете съгласие със своя рецензиран човек с такъв код, преминете към комуникация в реално време или потърсете трето мнение.

Сигурност

Проверете дали крайните точки на API изпълняват подходящо разрешение и удостоверяване, съвместими с останалата част от кодовата база. Проверете за други често срещани слабости, например слаба конфигурация, злонамерен потребителски вход, липсващи събития в журнала и др.

Коментари: кратък, приятелски настроен, изпълним

Рецензиите трябва да са кратки и написани на неутрален език. Критикувайте кода, а не автора. Когато нещо не е ясно, поискайте разяснения, а не допускайте невежество. Избягвайте притежателни местоимения, по-специално във връзка с оценки: „кодът ми работи преди вашата промяна“, „вашият метод има грешка“ и т.н. Избягвайте абсолютни преценки: „това никога не може да работи“, „резултатът винаги е грешен“.

Опитайте се да разграничите предложенията (напр. „Предложение: метод за извличане за подобряване на четливостта“), изискваните промени (напр. „Добавяне на @Override“) и точки, които се нуждаят от обсъждане или пояснение (напр. „Това наистина ли е правилното поведение? Ако така че, моля, добавете коментар, обясняващ логиката. "). Помислете за предоставяне на връзки или указатели за задълбочени обяснения на проблема.

Когато приключите с преглед на кода, посочете до каква степен очаквате авторът да отговори на вашите коментари и дали бихте искали да преразгледате CR след изпълнение на промените (напр. „Чувствайте се свободни да се слеят след отговор към няколкото малки предложения “срещу„ Моля, помислете за моите предложения и ме уведомете кога мога да погледна друг път. “).

Отговор на отзиви

Част от целта на прегледа на кода е подобряване на заявката за промяна на автора; следователно, не се обиждайте от предложенията на вашия рецензент и ги приемайте сериозно, дори ако не сте съгласни. Отговорете на всеки коментар, дори ако това е само обикновено „ACK“ или „готово“. Обяснете защо сте взели определени решения, защо съществува някаква функция и т.н. Ако не можете да постигнете съгласие с рецензента, преминете към реално- времева комуникация или потърсете външно мнение.

Поправките трябва да бъдат преместени към същия клон, но в отделен ангажимент. Пускането на ангажименти по време на процеса на преглед затруднява рецензента да следи промените.

Различните екипи имат различни политики за сливане: някои екипи позволяват само собственици на проекти да се сливат, докато други екипи позволяват на сътрудника да се слее след позитивен преглед на код.

Лични кодови рецензии

За повечето прегледи на кодове асинхронните инструменти, базирани на разликата като Reviewable, Gerrit или, GitHub са чудесен избор. Сложните промени или прегледите между страни с много различен опит или опит могат да бъдат по-ефективни, когато се извършват лично, пред същия екран или проектор, или дистанционно чрез VTC или инструменти за споделяне на екрана.

Примери

В следващите примери, предложените коментари за преглед са обозначени с // R: ... коментари в кодовите блокове.

Несъответстващо именуване

клас MyClass {
  private int countTotalPageVisits; // R: имена променливи последователно
  private int uniqueUsersCount;
}

Несъответстващи подписи на метод

интерфейс MyInterface {
  / ** Връща {@link по избор # празен}, ако s не може да бъде извлечен. * /
  обществено по избор  extraString (String s);
  / ** Връща нула, ако {@code s} не може да бъде пренаписан. * /
  // R: трябва да хармонизира връщащите стойности: използвайте и по избор <> тук
  public String rewriteString (String s);
}

Използване на библиотеката

// R: премахнете и заменете с MapJoiner на Guava
String joinAndConcatenate (Карта  карта, String keyValueSeparator, String keySeparator);

Личен вкус

int dayCount; // R: nit: Обикновено предпочитам numFoo над fooCount; зависи от вас, но трябва да го поддържаме последователно в този проект

Бъгс

// R: Това изпълнява numIterations + 1 повторения, това умишлено ли е?
// Ако е така, помислете за промяна на семантиката на numIterations?
за (int i = 0; i <= numIterations; ++ i) {
  ...
}

Архитектурни проблеми

otherService.call (); // R: Мисля, че трябва да избягваме зависимостта от OtherService. Можем ли да обсъдим това лично?

Автори

Робърт F (GitHub / Twitter)