Хороший, поганий код: як code review рятує проєкт

Всім привіт! Мене звати Богдан Чупіка, я працюю в Edtech-стартапі Mate academy на позиції Team Lead/ Java Coach. Серед моїх обовʼязків є рев’ю коду нашої команди девелоперів, перевірка коду студентів і проведення співбесід в команду.

Під час лайв кодингу на співбесідах (один з обовʼязкових етапів) я зустрічав дуже багато випадків, коли кандидати пишуть не ок код. Навіть після прохання привести їх до вигляду, який буде запушений в мастер. При чому помилки бувають як в джунів, так і в сеньйорів (у меншій кількості, але все ж).

У цій статті я зберу не тільки власний весь досвід, а і досвід команди з понад 70 девелоперів і менторів у нашій компанії. І, головне — відповім на питання: Як потрібно якісно писати код? Звичайно ж, з прикладами і порадами. Текст буде корисний і тим, хто пише код, і тим, хто його читає.


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

НЕ якісний код НЕ повинен потрапити в мастер (мейн) гілку. Для цього існує процес code review.

Code review

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

Чи можливий якісний код без код рев’ю? На мою субʼєктивну думку — ні. Робочий код — так, якісний — ні. Якщо не робити код рев’ю — з часом підтримка такого коду буде занадто складною і дорогою для бізнесу. Сприймайте код рев’ю, як допоміжний (я б навіть сказав — обовʼязковий) інструмент, що дозволить зробити код кращим.

Для того, щоб код рев’ю був зрозумілим і для контриб’ютора і для рев’ювера — пропишіть певні правила, як потрібно робити, а як — ні. На допомогу прийдуть гайди, які варто встановити у межах проєкту/ компанії.

Style guides

Стайл гайди — основа для системного рев’ю. Як описано в збірці стайлгайдів від Google:

«Style» covers a lot of ground, from «use camelCase for variable names» to «never use global variables» to «never use exceptions».

Крім згаданого вище гугловського стайлгайду, існує досить багато інших публічних стайлгайдів. Ось, наприклад, дуже крутий і якісний стайлгайд для JS від Airbnb. Для Python, окрім гугловського, можна юзати Black code style (мені він більше подобається) або PEP8.

Впевнений, що для вашої мови програмування теж можна досить легко знайти свій style guide. Плюс, кожен проєкт/ компанія можуть мати власні style guides.

До речі, свого роду style guides є і в дизайні. Ось приклад типових чек-лістів, яких варто дотримуватися при розробці дизайну.

Один з найбільших бенефітів від стайлгайдів полягає в тому, що їх можна імпортувати у свою IDE (приклад для Intellij Idea) і після автоформатування коду ви отримаєте готовий до пушу код в мастер. Майже))

Тулзи для code review

Тижпрограміст! Автоматизуй свою роботу!

Правила, описані в код стайлах — це круто, але ще краще, коли їх одразу застосовуєш в автоматичному режимі під час пул реквестів. Для цього ми можемо скористатися типовими тулзами, які вирішують цю проблему. Такі тулзи називають статичними аналізаторами коду. Найпопулярніший — це SonarQube. Існує багато альтернатив типу DeepSource, але ми використовуємо Apache Maven Checkstyle Plugin для Java code i ESLint для TypeScript. Як то кажуть KISS.

Зручно, коли ви інтегруєте ці автоматичні тулзи в свої CI pipelines і автоматичні перевірки будуть проганятися автоматично під час пулреквестів. Ось приклад, як насетапити ESLint за допомогою GitHub actions.

Чому style guide це круто, але недостатньо

Style guides дозволяють регламентувати синтаксис, але не логіку. Особливо не логіку неймінгу змінних, методів або класів, які можуть залежати від контексту задачі.

Щоб краще це побачити — давайте полайвкодимо.

Live coding

Для прикладу візьмемо задачу Length of the Last Word з Leetcode. Уявіть собі, що ви на співбесіді і перед вами пустий клас в IDE. Задача звучить наступним чином: «Реалізуйте метод, що приймає на вхід рядок, який може складатися з одного і більше слів. Вам потрібно повернути довжину останнього слова».

З точки зору інтерв’юера мене цікавлять наступні моменти:

  • назва методу;
  • назва вхідного параметру;
  • назва змінних всередині методу;
  • пунктуація і синтаксис;
  • читабельність коду;
  • перформанс.

Отож, почнемо. Зазначу, що тут і далі код буде реалізований мовою Java і я буду посилатися на Google style guide для Java.

Все починається зі створення класу і методу, в якому в нас буде реалізація:

class a {
      public int method(String s){ return null;    }
}

І тепер розберімося з проблемами, які є в цьому коді.

1. Назва класу a неінформативна. Але в нас недостатньо контексту, щоб зрозуміти, для яких цілей буде використовуватися наш метод. Тому я задаю наступний контекст: дане рішення нам потрібно тільки у межах інтервʼю і воно не буде використовуватися у production коді. При цьому, виходячи з правил style guide, назва класу повинна бути іменником або іменниковим сполучником та бути написаною у UpperCamelCase. Виходячи з такого контексту, я б запропонував назву класу Solution (у Leetcode це стандартна назва класів).

2. Назва методу неінформативна. Назва методу повинна описувати дію, що відбувається в цьому методі і бути дієсловом або починатися з дієслова. З огляду на умови завдання, я б назвав метод getLengthOfLastWord.

3. Також декілька рекомендацій щодо неймінгу методів:

  • Якщо метод повертає тип даних boolean як результат виконання методу, то назву методу варто починати з дієслів, які відповідають на будь-яке питання тільки «так» або «ні». Наприклад: hasSomething(), isSomething(), canDoSomething().
  • Методи, які не повертають нічого — називаєте так, щоб описувалася дія. Наприклад, метод, який перевірятиме індекс масиву варто назвати void checkIndex(int index). Якщо потрібно перевірити валідність даних, то — void validate(User user), але не void isValid(User user). Якщо ви хочете назвати метод isValid(User user), то варто повертати тип даних boolean, як результат виконання методу boolean isValid(User user).
4. Назва вхідного параметру s не дуже інформативна, тому я б перейменував на input. Чому не inputString? Бо Java суворо типізована мова програмування і немає сенсу додавати тип змінної до її назви. Приміром:

a. int[] intArr я б перейменував у int[] numbers.
b. List<User> userList назвав би List<User> users.

5. Відступи біля фігурних дужок. Згідно з Google Style guide:
a. Line break after the opening brace.
b. Line break before the closing brace.

Також пам’ятаємо про правильно розставлені пробіли.

6. Indentations. В Google style guide відступи складають 2 пробіли (в Intellij Idea, наприклад, це 4 пробіли).

Після проміжного рефакторингу наш код може виглядати так:

class Solution {
  public int getLengthOfLastWord(String input) {
    return null;
  }
}

Тепер перейдемо до реалізації.

Я взяв одне з рішень цієї задачі з Leetcode і воно виглядає наступним чином:

class Solution {
    public int getLengthOfLastWord(String s) {

     
        String res = "";
        int n = s.length()-1;
        if(s.charAt(0) == ' ' && s.length() == 1 ) return 0;

        for(int i = n ; i >= 0;i--){
           if(s.charAt(i) != ' '){
               res = res + s.charAt(i);
           }
           if(res.length() > 0 && s.charAt(i) == ' ') break;
        }
return res.length();
    }

}

Пройдімося по проблемах:

  1. Рядок 2: indentation 4 пробіли замість 2-х (P.S. в цьому прикладі я посилаюсь на Google Style guide. У вас в проєкті без проблем можуть бути відступи у 4, 6, 8 пробілів)
  2. Рядок 2: назва змінної s неінформативна.
  3. Рядки 3 і 4. Дві пусті лінії підряд. З Google Style guide: Multiple consecutive blank lines are permitted, but never required (or encouraged).
  4. Рядок 5. Назва змінної String res = «„; не дуже інформативна. Краще не скорочувати назви, а дописати 2-3 символи, що покращить читабельність коду. Приміром, String result = “»; Також це рішення не є оптимальним з точки зору перформансу, краще використовувати StringBuilder замість конкатенації рядків.
  5. Рядок 6: int n = s.length()-1; немає пробілів навколо знаку -.
  6. Рядок 6: загалом нам тимчасова змінна n не потрібна в принципі. Тут насправді допущено декілька помилок:

    а) назва змінної n неінформативна;
    б) змінна оголошена достатньо далеко від її місця використання (а це рядок 9). Набагато краще оголошувати і ініціалізувати змінні якомога ближче до місця їх першого використання. У такому випадку змінну n варто було оголосити на рядку 8, оскільки перше (і єдине) місце використання змінної — це рядок 9;

  7. Рядок 7 і рядок 13 — два стейтменти в одному рядку. Має бути один стейтмент на рядок. Окрім того, return 0; i break; повинні бути обгорнуті в фігурні дужки. Дивись приклад тут.
  8. Рядки 7, 9, 10, 13 немає пробілу між if та ( або між for i (. Так само між ) i {.
  9. Рядок 10: що таке ` `? Це розділювач слів. Тому давайте так і назвемо цю змінну. Проте, це буде не просто змінна, а константа. Для констант є свої правила неймінгу.

Після проміжного рефакторингу даний код може виглядати наступним чином:

class Solution {
 public static final char WORDS_SEPARATOR = ' ';

 public int getLengthOfLastWord(String s) {
   StringBuilder result = new StringBuilder();
   if (s.charAt(0) == WORDS_SEPARATOR && s.length() == 1) {
     return 0;
   }

   for (int i = s.length() - 1; i >= 0; i--) {
     if (s.charAt(i) != WORDS_SEPARATOR) {
       result.append(s.charAt(i));
     }
     if (result.length() > 0 && s.charAt(i) == WORDS_SEPARATOR) {
       break;
     }
   }
   return result.length();
 }
}

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

class Solution {
 public static final char WORDS_SEPARATOR = ' ';

 public int getLengthOfLastWord(String s) {
   if (s.charAt(0) == WORDS_SEPARATOR && s.length() == 1) {
     return 0;
   }

   int wordLength = 0;
   for (int i = s.length() - 1; i >= 0; i--) {
     if (s.charAt(i) != WORDS_SEPARATOR) {
       wordLength++;
     }
     if (wordLength > 0 && s.charAt(i) == WORDS_SEPARATOR) {
       break;
     }
   }
   return wordLength;
 }
}

Перформанс цього рішення досить непоганий:

  • По часу:

  • По памʼяті:

P.S. Для того, щоб рішення пройшло на Leetcode — я змінив назву на lengthOfLastWord. У Leetcode можуть бути свої правила неймінгу методів 🙂.

Загалом перформас для нас важливий, але читабельність коду хотілось би покращити. Альтернативне рішення (коротше і елегантне) може виглядати так:

class Solution {
 public int getLengthOfLastWord(String input) {
   String[] arr = input.trim().split(" ");
   return arr[arr.length - 1].length();
 }
}

У даному рішенні мені особисто не подобаються наступні моменти:

  1. Назва змінної String[] arr. Назва неінформативна. Набагато краще, коли ви дивитеся на назву змінної і стає зрозуміло, для чого вона потрібна. З огляду на код`String[] arr` — це масив слів. Тому давайте так і назвемо цю змінну: String[] words.
  2. Аналогічно, ми маємо тут розділювач слів у вигляді <code style="background:none;font:inherit;padding:0;">” “. Тому це теж варто виправити.

Після рефакторингу наш код виглядатиме наступним чином:

class Solution {
 private static final String WORDS_SEPARATOR = " ";

 public int getLengthOfLastWord(String input) {
   String[] words = input.trim().split(WORDS_SEPARATOR);
   return words[words.length - 1].length();
 }
} 

Звісно, за показником памʼяті ми будемо програвати попередньому рішенню, але це рішення лаконічніше. Тому тут, як і будь-де, ми маємо робити trade off — щось отримати і чимось пожертвувати. Конкретно в цьому випадку я б пожертвував перформансом на користь лаконічності.

У цілому дане рішення досить зрозуміле і я готовий пушити такий код в мастер (мейн) гілку.

Перейдемо до підсумків

  • Code style на проєкті важливий. Наявність чітких правил написання коду покращує його читабельність і полегшує підтримку.
  • Використовуйте готові стайлгайди або сформулюйте власний.
  • Автоматизуйте перевірки правил, описаних в стайлгайдах і під час написання коду (через налаштування в IDE) і під час CI.
  • Автоматизація не покриє 100% кейсів, використовуйте код рев’ю. Звертайте увагу на неймінги полів, методів, класів. Під час неймінгу завжди тримайте в голові контекст задачі, над якою ви працюєте.

P. S. я не додавав у статтю посилання на Clean Code від Боба Мартіна, але рекомендую його прочитати тим, хто ще цього не зробив. Плюс, тут можна переглянути коротку вижимку основних пунктів. Сподіваюсь, це не раз стане вам в пригоді)

Підписуйтеся на Telegram-канал «DOU #tech», щоб не пропустити нові технічні статті

👍ПодобаєтьсяСподобалось27
До обраногоВ обраному5
LinkedIn
Дозволені теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Дозволені теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter

Для Джави реально лінтерів не існує?

class a {

Завжди так класи називаю, щоб рев’юери не звертали уваги на макарони, які понаписував нижче.

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

Як організувати PR в команді, щоб вони не були дуже довгими?
В такому PR дуже складно розбиратись.

А просто відписати: код дуже складний, розібратися неможливо.

Я ся здогадував, шо стаття буде про те, як придирацця до відступів у коді. Розчарований не був.

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

Кодревю здорової людини такий:
— в тебе тут O(n^3), а можна простіше
— ти наштампував копіпастою 25 фрагментів, які робить то само, шо функція makeZajebis(), яку Василь пів року вже як написав, і про яку ти міг не знати
— тут у тебе вхід від користувача як приходе, так ти його в бібліотеку й передаєш, ти шо, всім довіряєш?
— а це ти класно придумав, покажи ше іншим, хай знають теж

іншому корона з голови не впаде, якшо він це поправе

Так, але blame буде показувати не зовсiм релевантне iм’я

Ну то й шо? В нових (ну як нових, 4 роки мабуть вже як) версіях ґіту це фільтруєцця.

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

Хз, я старовiр.
Та й сквош, коли на бекендi показує iм’я фронтендера — якось не айс.

Але повернемося до нашiх баранiв, автоформат кожен може сам налаштувати i нiхто не повинен це пiдтирати.

Щодо iмен методiв та змiнних, я б дав автору пару задачок з обмеденим часом та подивився, що видасть

По максимуму автоматично форматуйте код ( наприклад на post-commit hook). Так, щоб можна було писати як попало, і все одно код був би відформатований. Форматування коду (не назви змінних, а саме форматування) зараз майже для всіх мов є засоби. Це дозволяє змістити обговорення PRів з пробілів/рядків, і відступів — до семантики (назви класів, структура коду e.t.c).
Тобто навіть не linter форматування, коли тобі б’ють по рукам за те,що ти неправильно відформатував, а автоформаттер.

Лінтер може ловити вже більш складні випадки, які автоматично не можна виправити (function complexity/naming styleguide etc).

Раджу пошукати я зробити скрипт який НЕ-форматований( або форматовиний криво ) код приведе до вашого стайл гайду.
Дуже економить час і виключає людський фактор.

Можу вам дати декілька прикладів з моєї професійної кар’єри, коли код ревью мало не по губив проект. На 100% погоджуюсь з Google — без Style Guide і попередньою згодою алгоритма код ревью це такий спосіб зробти в команді срачь, де будуть холіварити на предмет табів чи пробілів, як ставити скобки, в якому стилі кемел-кейс чи снейк кейс, host чи master чи trunk чи main і так далі. Ще є срака, коли тех.лід робить аналіз алгоритму вже тоді як усе написано взривається гнівом і починає митати грім та молнії, хоча насправді сам в тому і винен, заставляє усе переписувати. Та от найбільша срака, коли код ревью використовують в політичній та конкурентній боротьбі.

Економічний ефект від код-рев’ю суто негативний. Це 100% витратна частина з околонулевим ROI. Знову ж таки, є альтернативи, як покращувати якість коду та тримати чистоту без рев’ю.

код ревю повине бути і воно допомагає покращити якісь коду і розуміння проекту всередині команди . але ревю заради правильного якогось стилю схоже у таких команд забагато вільного часу і схоже на великий ентепрайз і аутсорс ))

код ревю повине бути

Нічого нікому не повинне.

і воно допомагає покращити якісь коду і розуміння проекту всередині команди

Воно допомагає урівняти якість коду на якомусь середньому рівні. Але цей рівень не кращий. А для розуміння проекту краще парне програмування використовувати. Або менторство.

повністю погоджуюся. але деколи краще щось чим нічого ))

забагато вільного часу і схоже на великий ентепрайз і аутсорс ))

Там якраз часу в притик дедлайн мало не завтра, а команда займається оцією ахінеєю і холіварами, замість того щоб подивитись, що роблять колеги і де воно потенційно конфліктує з тим, що робиш ти. У величезних командах завжди існує фактор внутрішньої конкуренції і далеко не завжди вона спортивна. Скажімо команда з одного офісу блочить код ревью команди з іншого — хоча ті і справді гімнокодять, тести не пишуть, пушать закоментовані блоки коду, без документації і взагалі як курка лапою. Та справа в тому що і ця перша команда яка блокує — сама теж гімнокодить щоб замовнику показати що вони в тричі більше роблять сторіпойнтів, бо менеджера зробили звідтіля і вони собі забрали усі бонуси, найкращі таски та іноземні відрядження. А по факту як дивишся в середину — це чистий холівар на предмет кодстайлу, по суті жодного зауваження. Ще існує феномен дідівщіни, який я свого часу припарився припиняти. Роблять ментором колишнього джуна якого токсичний ментор, якого діставало цим займатись — гнобив. І от у тебе пів дня цей, тепер вже мідл якого той ментор подав на підвищення через пару місяців щоб здихатись навантаження, займається тим що психологічно травить вже свого трейні. Замість менторства і допомоги «дід» — підзиває молодого і починає над ним насміхатись, прислужувати та принижувати — до того ж замість роботи по проекту. І доводиться замість роботи займатись цим дитячим садочком. При чому чим гірше гнобили самого молодого — тим більше він і сам починає гнобити. І так ділі. Ніж такий код ревью — краще ніякого код ревью, а ще краще поставити статичний код аналіз санітайзери і прописати стайлгайд. І 70% холіварів припиняться.

return input.trim().split(WORDS_SEPARATOR).slice(-1).length()

Якщо Жаба не вміє так, то шкода.

Це майже один в один останній фрагмент коду від автора. Але... при розв’язанні задач ти зазвичай орієнтуєшся на хоча б трошки оптимізований варіант. Як на мене, є дуже велика різниця між O(N), де N це довжина рядка та O(n + m), це n це довжина кінцевих прогаликів, m це довжина останнього слова. Теж саме по використанню пам’яті. Тобто цей варіант ще можна використовувати, якщо у нас є ліниві обчислення та rsplit який працює з кінця.

Але... при розв’язанні задач ти зазвичай орієнтуєшся на хоча б трошки оптимізований варіант

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

Ну... як раз тут я бачу необхідність: ми пишемо бібліотечну функцію, абсолютно не маємо жодних ідей стосовно того, де та як вона буде викликатися. Може це хитра евристика визначення HTML контенту шляхом перевірки чи останній тег </html> чи ні?

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

Тому моя точка зору більша так: якщо цей код десь у середині обробки одного рядку виводу команди, то я не проти. Але якщо ми виносимо його в бібліотеку та абстрагуємося від того, звідки може прийти рядок, то, як на мене, краще витратити 15 хвилин, але не створювати tech debt. Як раз це питання я би виніс на review. Бо часто це призводить то ситуації, коли у нас тормозить, але не щось конкретне, а усе відразу.

Може це хитра евристика визначення HTML контенту шляхом перевірки чи останній тег < /html> чи ні?

З точки зору задачі, яка стоїть перед розробником, не особливо важливо, що саме робить бібліотечна функція чи як швидко це вона робить. Тому що задачу можна вирішити взагалі в інший спосіб, без залучення бібліотек чи ще чогось. А оптимізація вже проведена, можливо в гіршу сторону, ніж треба. Зазвичай, оптимізований код не сильно легко читати, він не сильно інтуїтивно-зрозумілий.

Java чудово це вміє робити. Навіть дивно, що ніхто не написав такого коду:

String text = “a,b,cd”;
int length = Arrays.stream(text.split(",")).reduce((a,b) -> b)
.map(String::length).orElse(-1);

Дякую за статтю.

Мені цікаво, чи досліджували ви такі метрики (формально або неформально):

1. Rework: PR отримав апруви, для рев’юверів все виглядало добре, але проблеми знайшли ще на етапі тестування.
У скількох відсотках випадків цю проблему можна було побачити на етапі code review?

2. Regression: всі PRи змерджено, але тут хтось десь знаходить баг. Чи робили ви бінарний пошук коміту що вніс цю регресію, з аналізом як в п.1?

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

Наперед дякую.

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

А перевірку стилю треба автоматизовувати по-максимуму. Я на своїх проектах по можливості роблю так, щоб порушення стилю прирівнювались к помилкам компіляції, що дає найшвидшу реакцію на проблеми. Більш м’яким варіантом може бути фейл компіляції на CI. Якщо в вас є багато легасі коду, який ніколи виправляти — можна фейлити тільки якщо кількість порушень зростає (тобто додали новий код, який не відповідає стилю).

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

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

Дуже гарна стаття. Ми вже багато років робимо код рев’ю у компанії, робимо це дуже прискіпливо, можемо завернути гарний алгорітм за незрозумілі назви методів чи змінних, навіть якщо «проект горить» і треба вже зараз.

Маємо купу статичних аналізаторів коду, до яких дописані власні правила.

Також наші workflow завжди мають task щодо рев’ю тестів, і так — джун може завернути код тімліда якщо йому щось незрозуміле.

я б звільнив як би якись техлід через якісь назви методів або щось джун там не зрозумів
постійно делівері на тиждень два затримували

Скажіть назву компанії будь ласка, щоб не довелось стикнутися випадково.

Таск на ревью тестів можна частково замінити на Hard quality gate (білд червоний поки тести менше 70%, починали з 40%). Правда люди починють писати інтегрейшн тести на кожен чих, тому довелося прибивати ще обмеженим часом білду.

Так, в нас тест має завершитись за 3 хв. максимум, інакше fail

Якщо відкинути пару абзаців, то статтю можна перейменуватив — «Хороший, поганий код: як code style рятує проєкт»

А чи є реальний приклад з свого досвіду коли

code review рятує проєкт

? Бо в code review є і свої недоліки, так повністю згоден що повинен бути статичний аналізатор коду, це звісно плюс. Але скільки людей мають проревювити код? якщо 1 то вийде що весь свій час найдосвідченіший програміст буде те і робити що дивитися ПР, а коли у відпустці то починається хаос і анархія? :)

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

Як на мене статичний аналіз + тести це маст хев для ПР, а от мануальний апрув хіба щоб контролювати які фічі зараз заходять в релізний скоуп.

Але скільки людей мають проревювити код? якщо 1 то вийде що весь свій час найдосвідченіший програміст буде те і робити що дивитися ПР,

Від проекту залежить, буває що й 2–3. Взагалі, git blame відразу каже, хто останній працював з цим кодом, тому бажано запросити його. Також, зазвичай, інколи у проекті вже є певний функціонал, тому рев’ю допомагає зробити це більш одноманінтно. Також часто якщо є рев’ю, то ти ребейзиш свій код щоб коміти були більш зрозумілими, це також має велью, таким чином я знаходив багато помилок та dead paths навіть у своєму коді.

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

коли у відпустці то починається хаос і анархія

у нас працювало так: є овнер репо, він праймарі ревьювер. є 2 архітекти на 5 тімок, вони секондарі ревьюери, коли праймарі недоступний. Також вони роблять набіги наввіть коли він доступний, щоб виправляти його розуміння лінії партії.

найдосвідченіший програміст буде те і робити що дивитися ПР

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

мануальний апрув хіба щоб контролювати які фічі зараз заходять в релізний скоуп

Зазвичай ставлять різні правила на мердж в різні гілки, можна поставити 1 ревьюера на дев, і всіх зацікавлених на умовний реліз. Залежить від ціни помилки.
У нас 1 (лід) на дев (перевірка коду), 2(лід+арх) на стейдж (код just to be sure), реліз менеджер на уат (список фіч на реліз), сто на прод.

жесть , у вас що у тімах одні джуни чи аусорс наскільки обнаглів що архітектам платять за ревю коду ?)))

У мене аутсорс тім екстеншн (дешева робоча сила на додачу до каліфорнійської тіми). Архітекти каліфорнійські, за каліфорнійські гроші, ревьювають. Хуяк-хуяк і в продакшн в деяких доменах дорого обходиться, архітекти дешевші.

це скоріше всього так довіряють команді а не ***к ***к )) щоб не було ***к ***к, тести нормальні тре мати і не тільки юніт

Окей, від поганого коду закриємося тестами. Як закриємося від поганих тестів?
Я в деяких проектах бачив
//if we reached this point, no error, but stupid linter asks me to assert something
Assert.IsTrue(true);
Які є альтернативи рев’ю? Коли хтось наступного разу подивиться на цей код, гіт блейм, сказати атата чуваку? Так цей код буде жити до цього світлого моменту.

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

Ну і на тему код ревью я поки що не бачив кращого матеріалу ніж цей mtlynch.io/human-code-reviews-1

Погоджуся з усим окрім

чи правильно реалізований функціонал

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

Дозвольте не погодитись. З ПР-у має бути доступно що там і навіщо, опис має бути повним, якщо ревьюеру незрозуміла логіка, то він то питає. Я можу навіть продакта в коменти викликати, якщо бачу якусь діч. Стиль та ляпи нехай тести та статичний аналізатор ловлять.

В ідеалі так. Хотілося би щоб так було і щоб на рішення подивився хтось ззовні з таким самим рівнем розуміння проблеми. Але у реальному житті я такого не бачив жодного разу. Більше — менше наближене тільки. Тому що зазвичай бізнес схильний економити на ресурсах.

class Solution {
// между классом и полем всегда должна быть пустая строка
private static final String WORDS_SEPARATOR = " «;
// между классом и методом всегда должна быть пустая строка
public int getLengthOfLastWord(String input) {

Це я іноді зустрічав, а от цього я не зустрічав ніразу. Навіть в JDK такого стилю коду я не бачив

public int getLengthOfLastWord(String input) {
// эта строка также всегда должна быть пустая
String[] arr = input.trim().split(» ");
return arr[arr.length — 1].length();
}
якщо тіло самого методу велике

Тоді в цьому випадку краще декомпонувати метод, якщо це можливо, на кілька дрібніших. Або так, ставити пусті рядки між логічними блоками коду. Але навряд чи на початку реалізації методу

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

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

Знову ж таки назва змінної n достатньо інформативна, я бачив дуже багато кода, де n означало довжину. Тому жодної проблеми не бачу.

wordLength хм... якого слова довжина? Теж незрозуміло
lastWordLength? Але відразу хочеться написати theLastWordLength, так скоро можна дійти до коротких оповідань (indexInTheArbitraryInputString замість i), які я доречі також бачив, ідентифікатор назви функції 50 символів або 7 слів. Не кажучи про тавтологію з назвою функції. Знову ж так, я бачив мільйон разів назву res та ніколи не мав проблему зрозуміти, чи то результат, чи ресурс.

WORDS_SEPARATORS теж виглядає якимось оверкілом, мій стиль більше схильний до чогось такого:

char ch = s.charAt(i);
bool isSpace = ch == ' ';
if (isSpace) {
  ++result;
}
if (result > 0 && !isSpace) {
  return result;
}

Коротше та елегантне рішення це, як я розумію, добрий день, пане маляре Шлемієль: на рівному місці ми робимо O(n) та радіємо на гігабайті тексту, який закінчується на прогалик.

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

int last_token_len(const char * s, int len)
{
    int i = len;
    while (i-->0) {
        if (s[i] == ' ') {
            break;
        }
    }
    return len - i - 1;
}

На кажучи про спрощення

int last_token_len(const char * s, int len)
{
    int i = len;
    while (i-->0 && s[i] != ' '); /* Find the last space pos or -1 if no */
    return len - i - 1;
}

Дякую за коментар!

які я доречі також бачив, ідентифікатор назви функції 50 символів або 7 слів

Як щодо назви класів у Spring?

на рівному місці ми робимо O(n)

Я погоджуюсь що можна було проітерувати з кінця строки і дійти до першого символа і не використувати trim. Про Ваше рішення по перше на C++, а по друге не враховує кейс коли останнім символом може бути пробіл. Цікаво було б побачити його з врахуванням цього edge кейсу.

Знову ж так, я бачив мільйон разів назву res та ніколи не мав проблему зрозуміти, чи то результат, чи ресурс.

Стиль коду — це те, про що ви домовляєтеся в рамках команди/проєкту. Комусь зручно res, комусь ні. До всього можна звикнути, навіть до того, щоб спати стоячи)

Як щодо назви класів у Spring?

Я не знаю, що таке Spring, взагалі увесь світ Java мені здається незрозумілим та дивним, а усі поради щодо розробки дуже неоднозначними.

по перше на C++

Чистий Сі

не враховує кейс коли останнім символом може бути пробіл

Враховує, у нас буде i=len - 1 одразу після циклу, тому ми повернемо len - (len - 1) - 1 = len - len + 1 - 1 = 0.

Комусь зручно res, комусь ні.

А це питання взагалі залежить від галузі. От математики та фізики, наприклад, на поспішають переписувати усі підручники зі зрозумілими ідентифікаторами, а користуються буквами, позначками, індексами, ... Також це може стосуватися до алгоритмічного коду, бо там зрозуміти дії часто набагато важливіше, ніж слідкувати на чим вони відбуваються. Звісно, що у якомусь фронтенді, де треба перекласти дані зі JSON у базу, інші вимоги.

Тому різні Мартіни, які намагаються видавати локальні рекомендації зі свого болота за глобальні знання... Якщо брати Linux Kernel, там може бути порушено майже усе з того, що писав Мартін, але проект розвивається та доволі стабільний та успішний.

друге не враховує кейс коли останнім символом може бути пробіл.

Я зрозумів, що ти мав на увазі, скоріше за усе кінцеві прогалики не мають впливати на результат. Але тоді мені в принципі не подобається ідея робити лише один цикл з купою прапорців. Бо запитай любого як це розв’язувати, він відповість: «Прибираємо прогалики, рахуємо довжину останнього слова». По коду я потрапив у пастку, що не зрозумів цю особливість також. Тому два цикли, щоб було відразу вижно що за чим. Це призводить до досить прямолінійного коду:

int last_token_len(const char * s, int len)
{
   int i = len;
   while (i-->0 && s[i] == ' ');
   len = ++i;
   while (i-->0 && s[i] != ' ');
   return len - i - 1;
}

Тут в принципі ми бачимо дві окремі дії, та нам не треба розв’язувати ребус на рахунок кінцевих прогаликів, бо призначення першого циклу їх знищення.

Але, якщо подивитися на цей код, то ми бачимо, що два цикли виконують по суті одну й ту саму дію: з’їдають кінцеві символи. Тому наступний варіант, який проситься, це виділить цю функціональність в окрему функцію:

typedef int char_prop_op(int ch);
enum complement { JUST = 0, COMPLEMENT=1 };
static inline int rcut(
    const char * s, int len,
    char_prop_op prop, enum complement complement)
{
    int i = len;
    while (i-->0 && !prop(s[i]) == complement);
    return i + 1;
}

int last_token_len(const char * s, int len)
{
    len = rcut(s, len, isspace, JUST);
    int pos = rcut(s, len, isspace, COMPLEMENT);
    return len - pos;
}

або на більш сішний смак зі вказівниками:

typedef int char_prop_op(int ch);
enum complement { JUST = 0, COMPLEMENT=1 };
static inline const char * rcut(const char * begin, const char * end,
    char_prop_op prop, enum complement complement)
{
    while (end-->begin && !prop(*end) == complement);
    return end + 1;
}

int last_token_len(const char * s, int len)
{
    const char * const end = rcut(s, s + len, isspace, JUST);
    const char * const begin = rcut(s, end, isspace, COMPLEMENT);
    return end - begin;
}

В обох прикладах помітно брак функціональних можливостей Сі, заміть прапорця `enum complement` можна було б просто передавати композицію функцій not . isSpace

Підписатись на коментарі