Хороший, поганий код: як 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)
.
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(); } }
Пройдімося по проблемах:
- Рядок 2: indentation 4 пробіли замість
2-х (P.S. в цьому прикладі я посилаюсь на Google Style guide. У вас в проєкті без проблем можуть бути відступи у 4, 6, 8 пробілів) - Рядок 2: назва змінної
s
неінформативна. - Рядки 3 і 4. Дві пусті лінії підряд. З Google Style guide: Multiple consecutive blank lines are permitted, but never required (or encouraged).
- Рядок 5. Назва змінної String res = «„; не дуже інформативна. Краще не скорочувати назви, а дописати
2-3 символи, що покращить читабельність коду. Приміром, String result = “»; Також це рішення не є оптимальним з точки зору перформансу, краще використовувати StringBuilder замість конкатенації рядків. - Рядок 6:
int n = s.length()-1;
немає пробілів навколо знаку-
. - Рядок 6: загалом нам тимчасова змінна
n
не потрібна в принципі. Тут насправді допущено декілька помилок:а) назва змінної
n
неінформативна;
б) змінна оголошена достатньо далеко від її місця використання (а це рядок 9). Набагато краще оголошувати і ініціалізувати змінні якомога ближче до місця їх першого використання. У такому випадку зміннуn
варто було оголосити на рядку 8, оскільки перше (і єдине) місце використання змінної — це рядок 9; - Рядок 7 і рядок 13 — два стейтменти в одному рядку. Має бути один стейтмент на рядок. Окрім того,
return 0;
ibreak;
повинні бути обгорнуті в фігурні дужки. Дивись приклад тут. - Рядки 7, 9, 10, 13 немає пробілу між
if
та(
або міжfor
i(
. Так само між)
i{
. - Рядок 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(); } }
У даному рішенні мені особисто не подобаються наступні моменти:
- Назва змінної
String[] arr
. Назва неінформативна. Набагато краще, коли ви дивитеся на назву змінної і стає зрозуміло, для чого вона потрібна. З огляду на код`String[] arr` — це масив слів. Тому давайте так і назвемо цю змінну:String[] words
. - Аналогічно, ми маємо тут розділювач слів у вигляді
<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 від Боба Мартіна, але рекомендую його прочитати тим, хто ще цього не зробив. Плюс, тут можна переглянути коротку вижимку основних пунктів. Сподіваюсь, це не раз стане вам в пригоді)
57 коментарів
Додати коментар Підписатись на коментаріВідписатись від коментарів