Перегляд коду Кращі практики

Інтернет надає багато матеріалів про огляди коду: про вплив оглядів коду на культуру компанії, на офіційні огляди безпеки, коротші посібники, довші контрольні списки, гуманізовані огляди, причини першочергового огляду коду, найкращі практики та інші найкращі практики, статистика щодо ефективності перегляду коду для лову помилок та приклади оглядів коду пішли не так. О, і звичайно, є і книги. Якщо коротко розповісти, у цій публікації в блозі представлено відгуки про Палантир щодо коду. Організації з глибоким культурним небажанням рецензувати, можливо, захочуть ознайомитись з чудовим нарисом Карла Е. Вігерса про гуманізацію рецензій, перш ніж намагатися слідувати цьому посібнику.

Ця публікація скопійована з посібників з найкращих практик нашого ланцюжка інструментів якості Java Code, Baseline та охоплює такі теми:

  • Чому, що і коли робити огляди коду
  • Підготовка коду до огляду
  • Виконання огляду коду
  • Приклади огляду коду

Мотивація

Ми проводимо огляд коду (CR), щоб покращити якість коду та отримати користь від позитивного впливу на культуру команди та компанії. Наприклад:

  • Комітети мотивовані поняттям про набір рецензентів, які розглядатимуть запит на зміну: комітет прагне очистити вільні кінці, консолідувати TODO та, як правило, покращити виконання. Визнання досвіду кодування за допомогою однолітків - це гордість для багатьох програмістів.
  • Обмін знаннями допомагає командам розвитку декілька способів:
    - CR чітко повідомляє додану / змінену / видалену функціональність членам команди, які згодом можуть спиратися на виконану роботу.
    - Комітет може використовувати техніку або алгоритм, з якого рецензенти можуть вчитися. Загалом, огляди коду допомагають підняти планку якості для всієї організації.
    - Рецензенти можуть володіти знаннями про методи програмування або базою коду, які можуть допомогти покращити або закріпити зміни; наприклад, хтось інший може одночасно працювати над подібною функцією або виправляти.
    - Позитивна взаємодія та спілкування зміцнюють соціальні зв’язки між членами команди.
  • Послідовність кодової бази полегшує читання та розуміння коду, допомагає запобігти помилкам та полегшує співпрацю між звичайними та мігруючими видами розробників.
  • Розбірливість фрагментів коду важко судити для автора, чий це дитина, і легко судити для рецензента, який не має повного контексту. Розбірливий код є більш багаторазовим використання, відсутній помилок та не потребує майбутнього.
  • Випадкові помилки (наприклад, помилки помилок), а також структурні помилки (наприклад, мертвий код, помилки в логіці чи алгоритмі, проблеми з роботою чи архітектурою) часто набагато простіше помітити критичним рецензентам із зовнішньої точки зору. Дослідження виявили, що навіть короткі та неофіційні огляди коду мають значний вплив на якість коду та частоту помилок.
  • Відповідність та регуляторне середовище часто вимагають перегляду. CR - це чудовий спосіб уникнути загальних пасток безпеки. Якщо ваша функція чи середовище мають значні вимоги до безпеки, виграєте (і, можливо, знадобиться) перегляд вашими місцевими коштами із безпеки (посібник OWASP - хороший приклад цього процесу).

Що переглянути

На це питання не існує вічно правдивої відповіді, і кожна команда розробників повинна погодитися на власний підхід. Деякі команди вважають за краще переглядати кожну зміну, об'єднану в основну галузь, а інші мають поріг «дрібниці», під яким огляд не потрібно. Компроміс - це між ефективним використанням часу інженерів (як авторів, так і рецензентів) та підтримкою якості коду. У певних регуляторних середовищах перегляд коду може знадобитися навіть для дрібницьких змін.

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

Коли переглядати

Перегляд коду повинен відбуватися після того, як автоматичні перевірки (тести, стиль, інші CI) успішно завершені, але до того, як код злиється з основним відділенням сховища. Зазвичай ми не проводимо офіційний огляд коду сукупних змін з моменту останнього випуску.

Для складних змін, які повинні зливатися в гілку основної лінії як єдине ціле, але занадто великі, щоб вписатись в один розумний CR, розгляньте складену модель CR: Створіть функцію первинної гілки / big-особливість та ряд вторинних гілок (наприклад, функція / big-feature-api, feature / big-feature-testing та ін.), що кожне інкапсулює підмножину функціональності та підлягає індивідуальному перегляду коду у відповідності з гілкою функції / big-feature. Після того як всі вторинні гілки об'єднані в особливі / великофункціональні, створіть CR для об'єднання останніх у основну гілку.

Підготовка коду до огляду

Відповідальність автора - представити КР, які легко переглядати, щоб не витрачати час та мотивацію рецензентів:

  • Обсяг і розмір. Зміни повинні мати вузьку, чітко окреслену, автономну сферу, яку вони вичерпно охоплюють. Наприклад, зміна може реалізувати нову функцію або виправити помилку. Коротші зміни є кращими, ніж більш довгі. Якщо CR вносить істотні зміни до більш ніж ~ 5 файлів, або на написання потрібно більше 1–2 днів, або для перегляду знадобиться більше 20 хвилин, подумайте про поділ його на декілька автономних CR. Наприклад, розробник може внести одну зміну, яка визначає API нової функції з точки зору інтерфейсів та документації, та другу зміну, яка додає реалізації для цих інтерфейсів.
  • Подавайте лише комплектні, самоперевірені (за різними показниками) та самоперевірені КЗ. Щоб заощадити час рецензентів, протестуйте подані зміни (тобто запустіть тестовий набір) і переконайтесь, що вони проходять усі збірки, а також усі тести та перевірку якості коду, як локально, так і на серверах CI, перш ніж призначити рецензентам.
  • Зміни, що переробляються, не повинні змінювати поведінку; навпаки, зміни, що змінюють поведінку, повинні уникати змін, що змінюються, та форматування коду. Для цього є кілька вагомих причин:
    - Зміни рефакторингу часто торкаються багатьох рядків і файлів, і тому вони будуть розглянуті з меншою увагою. Ненавмисні зміни в поведінці можуть просочитися в базу коду, не помічаючи ніхто.
    - Великі зміни рефакторингу порушують збирання вишні, звільнення та іншу магію контролю над джерелами. Скасувати зміну поведінки, яку було введено як частину рефакторингу на всій сховищі, дуже важко.
    - Дорогий час перегляду людини повинен бути витрачений на логіку програми, а не на стилі, синтаксис або дебати форматування. Ми вважаємо за краще влаштовувати такі автоматизовані інструменти, як Checkstyle, TSLint, Baseline, Prettier тощо.

Введіть повідомлення

Нижче наводиться приклад хорошого повідомлення про виконання зобов’язань за широко цитованим стандартом:

Пропис з великим написом, короткий (80 знаків або менше) резюме
Більш детальний пояснювальний текст, якщо це необхідно. Оберніть його приблизно на 120 символів. У деяких контекстах - перший
рядок розглядається як тема електронної пошти, а решта тексту як основна частина. Порожній рядок, що відокремлює
резюме з організму є критичним (якщо ви повністю не опустите тіло); такі інструменти, як Rebase, можуть заплутатися при запуску
двоє разом.
Напишіть повідомлення про фіксацію в обов'язковому порядку: "Виправити помилку", а не "Виправлена ​​помилка" чи "Виправлена ​​помилка". Ця конвенція відповідає
з повідомленнями про фіксацію, згенерованими такими командами, як git merge та git return.
Подальші абзаци надходять після порожніх рядків.
- Точки з кулями теж добре

Спробуйте описати і те, що зміниться, і як це відбувається:

> BAD. Не робіть цього.
Зробіть компіляцію ще раз
> Добре.
Додайте jcsv залежність, щоб виправити компіляцію IntelliJ

Пошук рецензентів

Зазвичай комітер пропонує одного або двох рецензентів, які знайомі з кодовою базою. Часто одним з рецензентів є ведучий проекту або старший інженер. Власники проектів повинні розглянути можливість передплати своїх проектів, щоб отримувати повідомлення про нові КР. Огляди коду серед більш ніж трьох сторін часто непродуктивні або навіть контрпродуктивні, оскільки різні рецензенти можуть запропонувати суперечливі зміни. Це може свідчити про принципову незгоду щодо правильної реалізації та має бути вирішена поза кодом з огляду коду на форумі з більшою пропускною здатністю, наприклад особисто або на відеоконференції з усіма зацікавленими сторонами.

Виконання огляду коду

Огляд коду є точкою синхронізації між різними членами команди і, таким чином, має можливість блокувати прогрес. Отже, огляди коду повинні бути оперативними (на порядок годин, а не днів), а члени команди та керівники повинні бути обізнані з часовим зобов'язанням та відповідно визначати пріоритет часу перегляду. Якщо ви не вважаєте, що зможете пройти огляд вчасно, повідомте про це учасника, щоб вони могли знайти когось іншого.

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

Як рецензент, ви несете відповідальність за дотримання стандартів кодування та підтримання рівня якості. Перегляд коду - це більше мистецтво, ніж наука. Єдиний спосіб навчитися цьому - це зробити; досвідчений рецензент повинен розглянути питання щодо змін інших менш досвідчених рецензентів і дозволити їм зробити огляд першим. Якщо припустити, що автор дотримується наведених вище вказівок (особливо щодо самоперевірки та забезпечення виконання коду), ось список рецензентів, на який слід звернути увагу рецензента:

Призначення

  • Чи відповідає цей код цілі автора? Кожна зміна повинна мати конкретну причину (нова функція, рефактор, помилка тощо). Чи реально поданий код досягає цієї мети?
  • Задавати питання. Функції та класи повинні існувати з причини. Коли рецензент не зрозумів причину, це може бути свідченням того, що код потрібно переписати або підтримати коментарями або тестами.

Впровадження

  • Подумайте, як би ви вирішили проблему. Якщо це по-іншому, чому це так? Чи обробляє ваш код більше (крайових) справ? Це коротше / простіше / чистіше / швидше / безпечніше, але функціонально еквівалентне? Чи є якийсь базовий зразок, який ви помітили, який не захоплений поточним кодом?
  • Чи бачите ви потенціал для корисних абстракцій? Частково дублюваний код часто вказує на те, що більш абстрактний або загальний фрагмент функціональності можна витягти та повторно використовувати в різних контекстах.
  • Думайте, як противник, але будьте приємні. Постарайтеся «зловити» авторів, які беруть ярлики або відсутні відмітки, придумуючи проблемні конфігурації / вхідні дані, які порушують їх код.
  • Подумайте про бібліотеки або існуючий код продукту. Коли хтось повторно реалізує існуючу функціональність, частіше за все це просто тому, що вони не знають, що вона вже існує. Іноді код або функціональність дублюється за призначенням, наприклад, щоб уникнути залежностей. У таких випадках коментар до коду може уточнити наміри. Чи введена функціональність вже надана наявною бібліотекою?
  • Чи зміна відповідає стандартним зразкам? Створені бази коду часто демонструють шаблони навколо конвенцій іменування, декомпозиції програмної логіки, визначень типів даних тощо. Зазвичай бажано, щоб зміни реалізовувалися відповідно до існуючих шаблонів.
  • Чи додає зміна залежність часу складання або часу виконання (особливо між підпроектами)? Ми хочемо підтримувати нашу продукцію вільно поєднаною з якомога меншими залежностями. Зміни залежностей та системи побудови слід ретельно перевіряти.

Розбірливість та стиль

  • Подумайте про свій досвід читання. Ви зрозуміли ці поняття за розумну кількість часу? Чи було нормальним потоком і чи легко змінюватись назви змінних методів і назв методів? Чи вдалося вам відстежувати декілька файлів чи функцій? Ви були відкладені непослідовним називанням?
  • Чи дотримується код правил кодування та стилю коду? Чи відповідає код проекту з точки зору стилю, конвенцій API тощо? Як було сказано вище, ми віддаємо перевагу врегулюванню дискусій стилів за допомогою автоматизованих інструментів.
  • Чи має цей код TODO? TODO просто накопичуються в коді і з часом стають несвіжими. Запропонуйте авторові подати квиток на GitHub Issues або JIRA та додайте номер видання до TODO. Запропонована зміна коду не повинна містити коментований код.

Технічне обслуговування

  • Прочитайте тести. Якщо тестів немає і повинні бути, попросіть автора написати деякі. По-справжньому непереборні функції зустрічаються рідко, тоді як неперевірена реалізація функцій, на жаль, поширена. Перевірте самі тести: чи охоплюють вони цікаві випадки? Чи читаються вони? Чи знижує показник CR загальне покриття тесту? Подумайте про способи розбиття цього коду. Стандарти стилів для тестів часто відрізняються від основного коду, але все ще важливі.
  • Чи вводить цей CR ризик порушити тестовий код, встановити стеки або тести інтеграції? Вони часто не перевіряються як частина попередніх перевірок / злиття, але їх зменшення є болючим для всіх. Конкретні речі, на які слід звернути увагу: видалення тестових утиліт або режимів, зміна конфігурації та зміни в макеті / структурі артефактів.
  • Чи порушує ця зміна зворотна сумісність? Якщо так, то чи добре об'єднати зміни на цьому етапі чи її слід перенести у більш пізній випуск? Порушення можуть включати зміни в базі даних або схеми, зміни в публічному API, зміни в робочому процесі користувача тощо.
  • Чи потрібен цей код інтеграційних тестів? Іноді код не може бути адекватно перевірений лише одиничними тестами, особливо якщо код взаємодіє із зовнішніми системами або конфігурацією.
  • Залиште відгуки про документацію на рівні коду, коментарі та повідомлення про фіксацію. Зайві коментарі захаращують код, а лайливі повідомлення виконуватимуть містифікацію майбутніх учасників. Це не завжди застосовується, але якісні коментарі та повідомлення про прихильність оплачуватимуть себе внизу. (Подумайте про час, коли ви побачили чудове або справді жахливе повідомлення про коментар чи коментар.)
  • Чи було оновлено зовнішню документацію? Якщо ваш проект підтримує README, CHANGELOG або іншу документацію, чи було оновлено, щоб відобразити зміни? Застаріла документація може бути більш заплутаною, ніж жодна, і виправити її в майбутньому буде дорожче, ніж оновлювати її зараз.

Не забудьте похвалити стислий / читабельний / ефективний / елегантний код. І навпаки, відхилення або несхвалення CR не є грубим. Якщо зміна є зайвою або неактуальною, відмовте її від пояснення. Якщо ви вважаєте це неприйнятним через одну чи декілька фатальних вад, відхиліть це, знову ж таки, із поясненням. Іноді правильним результатом КР є "давайте робити це зовсім по-іншому" або навіть "нехай цього не робити зовсім".

Будьте з повагою до рецензентів. Хоча змагальне мислення зручно, це не ваша особливість, і ви не можете приймати всі рішення. Якщо ви не можете домовитися з рецензентом з таким кодом, перейдіть на спілкування в реальному часі або зверніться до третьої думки.

Безпека

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

Коментарі: лаконічні, доброзичливі, діючі

Відгуки повинні бути стислими та писати нейтральною мовою. Критикуйте код, а не автора. Коли щось незрозуміле, попросіть пояснення, а не припускайте незнання. Уникайте присвійних займенників, зокрема в поєднанні з оцінками: "мій код працював до зміни", "ваш метод має помилку" тощо. Уникайте абсолютних суджень: "це ніколи не може працювати", "результат завжди неправильний".

Спробуйте розмежувати пропозиції (наприклад, "Пропозиція: метод вилучення для поліпшення розбірливості"), необхідні зміни (наприклад, "Додати @Override") та точки, які потребують обговорення чи уточнення (наприклад, "Це дійсно правильна поведінка? Якщо тому, будь ласка, додайте коментар, що пояснює логіку. "). Подумайте про надання посилань чи покажчиків для поглиблених пояснень проблеми.

Коли ви закінчите з переглядом коду, вкажіть, в якій мірі ви очікуєте від автора відповіді на ваші коментарі та чи хотіли б ви повторно переглянути КР після внесення змін (наприклад, «Не соромтеся злитися після відповіді. до декількох незначних пропозицій "проти" Будь ласка, врахуйте мої пропозиції та дайте мені знати, коли я можу поглянути ще раз. ").

Відповідаючи на відгуки

Частина мети огляду коду - вдосконалити запит на зміну автора; отже, не ображайтесь на пропозиції рецензента і сприймайте їх серйозно, навіть якщо ви не погоджуєтесь. Відповідайте на кожен коментар, навіть якщо це лише простий "ACK" або "зроблено". Поясніть, чому ви прийняли певні рішення, чому існує якась функція і т. Д. Якщо ви не можете домовитися з рецензентом, перейдіть на реальну- час спілкування або пошук зовнішньої думки.

Виправлення мають бути висунуті до тієї ж гілки, але в окремий комітет. Знімання на сквош під час огляду рецензенту важко стежити за змінами.

Різні команди мають різну політику злиття: деякі команди дозволяють об'єднувати лише власників проектів, а інші команди дозволяють учаснику об'єднатися після позитивного перегляду коду.

Особисті огляди коду

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

Приклади

У наступних прикладах запропоновані коментарі до огляду вказуються // R: ... коментарі в кодових блоках.

Невідповідне називання

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

Невідповідні методи підписів

інтерфейс MyInterface {
  / ** Повертає {@link Необов’язково # порожній}, якщо її неможливо отримати. * /
  загальнодоступний  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?
for (int i = 0; i <= numIterations; ++ i) {
  ...
}

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

otherService.call (); // R: Я думаю, що нам слід уникати залежності від OtherService. Чи можемо ми обговорити це особисто?

Автори

Роберт Ф (GitHub / Twitter)