Сучасна диджитал-освіта для дітей — безоплатне заняття в GoITeens ×
Mazda CX 5
×

Стратегія якісного Code Review: підходи та інструменти

Привіт! Мене звати Інна, я займаюсь фронтенд-розробкою майже шість років. Останні два роки я працюю в R&D офісі Fiverr у Києві, з моменту його відкриття. У цій статті я поділюсь своїми спостереженнями про код-рев’ю у компанії, де працюють 25 девелопмент-команд. Розповім про підходи та інструменти, які ми використовуємо, а також про те, які проблеми виникають, коли над одним продуктом працює близько 150 програмістів.

Управлять многими — то же, что управлять немногими. Дело в организации.

«Стратегическая мощь», Сунь-цзы

Авторка ілюстрації Jane Linville

Intro

R&D поділяється на невеликі команди, кожна відповідає за окрему частину функціоналу — платежі, сторінка гіга чи попап реєстрації, або за групу таких систем. Такі команди називаються Task Force та складаються з різних спеціалістів — розробників (бек + фронт), продакт-овнерів, продакт-менеджерів, іноді — дизайнерів та бізнес-аналітиків. Вони утворюють вертикалі, або самостійні юніти із 5-10 осіб.

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

Навіщо ми робимо код-рев’ю

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

  • Консистентність

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

  • Навчати та навчатися

У різних розробників — різний досвід, і рев’ю — це чи не найефективніший спосіб ним поділитися. Курси, книги та туторіали сприяють професійному росту, але під час код-рев’ю є принаймні дві людини, що обмінюються думками та діляться враженнями від коду. Навіть якщо це якась дрібниця на кшталт «ще 5 способів вирівняти div по центру». В процесі написання коду я теж намагаюсь знаходити оптимальні та цікаві рішення, що може бути корисним для мого рев’юера.

  • Якість коду

Код-рев’ю не допомагає стовідсотково уникати багів на проді. Кожен може стомитись, випадково пролистати файл в PR-і тощо. Буває, що рев’юеру вдається помітити якусь грубу помилку в коді. Але баги частіше виникають не від грубих і очевидних помилок, а від незначних огріхів. Задача рев’юера скоріше в тому, щоб пересвідчитись, що код відповідає стандартам, дійсно корисний, читабельний, консистентний. Можна порадити, як його покращити: врахувати едж-кейси, запропонувати тести чи зручні патерни.

Платформа для код-рев’ю

Для рев’ю ми використовуємо Github в комбінації з різними автоматизаціями, про які трішки пізніше. В пригоді також стають і більш особисті комунікації — zoom або офлайн в офісі.

Проблеми і способи розв’язання

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

1. Вертикальність горизонтальність

В теорії всі наші команди вертикальні та незалежні. Але ми працюємо над реальним продуктом. Тому самостійні вони доти, доки не виникає потреба взаємодіяти з іншими командами (surprise). Наприклад, є logged out homepage — сторінка, яку бачать не зареєстровані користувачі. За неї відповідає окрема команда. Ця сторінка є ізольованою, тому що там не потрібні дані користувача чи платіжної сесії.

Домен Logo Maker вже не настільки ізольований: є сторінка і для зареєстрованих користувачів, і треба ініціалізувати платіжну сесію, і показати красиву сторінку замовлення (не така як у всіх, гарніша). Тому код-рев’ю мають робити і інші команди. А вони можуть бути в іншій часовій зоні, в когось свято і працювати гріх, а десь п’ятниця — це вихідний. Так і PR може загубитися. Зрештою, майже ніякого вербального зв’язку та лише сухі коментарі в github.

Бюрократія to the rescue

Розповім про досвід команд, до чиїх репозиторіїв часто роблять реквести зі змінами. В простих випадках буває достатньо Slack-каналу виду #team-name-prs, куди кожен може надіслати лінк на github з коротким описом своїх змін.

Коли цього не вистачає, і PR починають губитися в потоці повідомлень, створюють окремий борд в monday. Там є можливість додавати свої PR через спеціальну форму. Це дозволяє ассайнити різних людей на рев’ю тої частини системи, в якій вони більш обізнані. Можна одразу побачити хто більш вільний і зможе переглянути PR швидше. В monday є інтеграція зі Slack, і коли тікет з PR-ом посунули в Approved, людині приходить сповіщення.

В команді, де я працюю, сім розробників, але в нас досить рідко бувають зовнішні PR-и. Проте ми теж вирішили спробувати користуватися бордом. Наші внутрішні PR-и стали більш прозорими, рев’ю — активнішими. А після вихідних не треба шукати, де і що треба було переглянути.

Борд дозволяє впоратися з пріоритизацією. У когось PR на 200 строк, але він може почекати, а в когось — на 10, і це прям ну дуже важливий фікс. Звісно, коли бачиш перед очима весь скоуп PR-ів, то стає простіше грати в пріоритизацію (і вигравати).

2. Розмаїття

У команд можуть бути свої підходи до розробки, якісь свої традиції та обряди для написання коду.

Стандартизація

Наш онбординг триває 2-3 тижні і містить знайомство з документацією щодо код- рев’ю. Нові розробники дізнаються, який треба писати код, як він має виглядати (гайдлайни щодо стилю коду), як побудувати архітектуру чогось нового, як називати PR і т.п. Гадаю, це чи не найважливіший аспект — мати спільний mindset, бути on the same page, простими словами — дивитись в один бік.

«Йдеш» до сусідньої команди, а в них у репозиторії такі ж відступи в файлах, React-компоненти написані за схожим принципом, архітектура апки така сама і роути снейк_кейсом написані — почуваєшся наче в гостях у найліпшого друга.

3. Чужий

Можна я тут жарт з twitter додам?

Іноді отримуєш задачу і не розумієш, що це зачепить іншу команду та їх овнершіп. А зміна — велика. Або складна. Що робити — не ясно. Можна просто почитати код і спробувати додати свої зміни навмання. Але їх ще треба протестувати. А якщо не знаєш всієї логіки і там щось складне? А ти мови не знаєш навіть. Гаразд, це вже я перебільшую, просто хочу розповісти про те, як не факапити в чужому домені.

Інкапсуляція

Найбезпечніший спосіб зробити зміни в іншому домені — ізолювати їх.

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

Chelli Kircshenbaum, тімлідерка девелопмент команди caterpillars

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

Дизайн-рев’ю

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

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

Навіть якщо зміни інкапсульовано, DR є обов’язковою вимогою, адже ми відповідальні за надійність та стабільність нашого домену. Майже кожна фіча проходить DR або хоча б короткий синк. Його відсутність — це вже одразу red flag для мене, ми не в курсі запропонованих змін і маємо проходити DR постфактум. Іноді у людей бувають не найкращі рішення, тому що вони можуть не знати всіх особливостей роботи нашого домену, що їх зміни можуть впливати на перфоманс чи створювати баги.

Chelli Kircshenbaum, тімлідерка девелопмент команди caterpillars

Звісно, якщо це якась незначна зміна, апдейт пакету, або все і так зрозуміло з коду, то DR не є обов’язковим.

4. Час

Код-рев’ю — невід’ємна частина роботи, але на нього не завжди є час. І поки рев’юєр намагається завершити власну задачу, пулл реквест застаріває, з’являються мерж конфлікти, розробник вже забув, що хотів там зарелізити і т.п. Після рев’ю треба буде оновити PR, зарезолвити конфлікти, знову протестувати... Це марні витрати часу.

Автоматизація

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

Для найбільш «популярних» репозиторіїв github автоматично генерує чекліст в описі PR-у. З його допомогою людина може перевірити себе. Це дуже зручно, адже інакше рев’юеру довелось би писати одне й те саме декілька разів на день різним людям.

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

5. Без qa-спеціалістів

У нас нема QA спеціалістів. Спершу це було для мене шоком.

Овнершіп

Зате в нас є овнершіп — розробники є «власниками» своїх доменів.

Мені подобається працювати з девелопмент-командами, які повністю відповідають за свій домен, за те, як вони рев’ювлять та деліверять свій код. В такій команді я повністю можу покладатися на те, що розробники будуть підтримувати високий рівень коду, а це матиме вплив на якість самого продукту (що мене найбільше хвилює). Адже це не просто інвестиція в код, з яким зручно працювати і додавати нові фічі. Це також покращує UX, адже знижується ризик того, що код може зламатися в якомусь не очевидному місці.

Sharon Wolfson, продакт-менеджерка

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

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

Юрій Дадичин, тімлідер девелопмент команди spiders

Соціальні контракти

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

Mindset того, хто відкриває PR

Відповідальність як наслідок овнершіпу

Ми намагаємось мінімізувати подільність відповідальності, в ідеальному випадку це має бути одна особа. Це дуже корисно для розробників. Наприклад, якщо в команді є QA-спеціалісти, то вже вони стають відповідальними за те, що побачить продакшн. Тому якщо на прод піде якийсь баг, то це буде їхня провина. Такий підхід породжує недбальство.

Юрій Дадичин, тімлідер девелопмент команди spiders

Тестування

Перед відправкою PR на рев’ю, його треба обов’язково перевірити на стейджингу (на щастя, у нас їх дуже багато) /локально. Навіть якщо це зміна тексту. Навіть якщо одна буква. Одного разу я хедер ледь не зламала, тому що занадто великий текст на маленькому екрані виходив за рамки Всесвіту.

Завдяки відповідальності (я ж кажу, корисний mindset) я все-таки вирішила протестувати зміни, і побачила, що це просто катастрофа. Навряд чи найбільш топовий рев’юер помітив би це із одного рядка css в моєму PR-і.

Non-QA підхід дуже цікавий та корисний. Розробники відчувають більше відповідальності за те, що вони делірять на прод. Вони — овнери своїх фіч, і тому більше інвестують часу в якість та надійність свого коду — додають юніт-тести, інтеграційні, піклуються про якість свого домену, налаштовують моніторинг тощо.

Michelle Fried, тімлідерка девелопмент-команди pumas

Автоматичні перевірки

Їх робить після відкриття PR-у CircleCi. У нас він сам перевіряє код на відповідність стандартам, запускає переклади, перевіряє, чи код взагалі білдиться, чи є тести тощо. Все, що треба зробити — просто пересвідчитись, що вони пройшли успішно.

Простота

В команді, де я працюю, діє обмеження в 200 рядків на 1 PR. Це правило ми адаптували зовсім недавно (коли я статтю почала писати, ага), але вже дуже задоволені результатами. Тричі переглядати код в 200 рядків морально і фізично простіше, ніж в 600 рядків. Також зменшується кількість конфліктів, швидше можна щось зарелізити. Немає PR-ів, що мають зміни ще якоїсь фічі (яка там взагалі ні до чого). Від імені нашої команди дуже радимо робити маленькі ізольовані PR.

Детальність

Рев’ю — це майже завжди відволіктись від своєї роботи та подивитись, що ж там хто написав. На мою думку, одним із показників якісного PR-у та справжньої турботи про колег — це підхід, коли розробник намагається мінімізувати витрати часу рев’юера на пошуки мокапів у Figmа продуктових вимог тощо. Опис PR має включати в себе відповіді на всі питання, які можуть виникнути у рев’юера: навіщо взагалі цей код, як він працює (бізнес-логіка), як це виглядає (приклади зі скріншотами, відео чи гіфками), на що він впливає, які проблеми вирішує тощо. Можна ще додати коментарі в коді, які пояснюють те чи інше рішення, мають лінк на статтю з поясненням підходу.

Моніторинг

Дорослі серйозні люди моніторять стан своєї системи постійно. Для логів ми користуємося Graylog та Coralogix, графіки будуються в Grafana. Моніторинг відбувається постійно, а не тільки після деплою.

У нашого домену існує спеціальний канал у Slack, куди приходять агреговані повідомлення про помилки на проді. Вони поділені на backend та frontend.

Відслідковується навантаження на системи, response time, кількість запитів та помилок по ендпоінтам та сторінкам. Також відслідковуються опосередковані бізнес KPI: кількість замовлень, візитів головних сторінок, створених проєктів тощо. У разі перевищення встановленних трешхолдів або появи неочікуваних змін в показниках приходить повідомлення в Slack.

Моніторинг допомагає нам бути впевненими у стабільності системи будь-коли. Як сказав Юра, мій тімлід, «я без подібного моніторингу з дому більше не вийду».

Mindset того, хто PR дивиться

Пріоритизація

Ми намагаємося переглядати PR-и якомога швидше після їх відкриття. Так людині доведеться менше витрачати час на те, щоб змержитись з main гілкою, знову запустити CircleCi та протестувати все ще раз.

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

Юрій Кабай, тімлідер девелопмент команди grizzlies

Простота

Рев’юер не має робити зауваження коду, який не стосується PR і який можна було б паралельно покращити*. Достатньо залишити коментар, що це було б круто пофіксити, але не критично. Я читала, що в деяких компаніях використовують приставку nit (nitpick) для таких випадків, треба і нам таке запровадити. Зрештою, мета рев’ю — це не довести код до ідеалу, а покращити його до певного рівня.

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

Вигода

Рев’ю — це можливість навчитись чомусь новому. Якщо підходити до цього так, а не скептично, то можна гарно прокачати свої скіли, просто переглядаючи код. Іноді я читаю PR-и команд, які до мене взагалі ніякого стосунку не мають. Але їх написали класні специ і майже завжди в них я можу підгледіти цікаві прийоми в коді.

Ти/ми/він/вона/вони

Базовий рівень, якого очікують від усіх — не дозволяти собі переходити на особистості. Тобто, не «тобі треба переписати», а «нам треба покращити». Формулювання звучить, як банальна ввічливість, але тут дещо глибше. В PR йде обговорення конкретного коду, а не здібностей окремих авторів.

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

Oded Goldglas, техлід інфраструктурної команди

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

🌿💥🌸🐝

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

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

Oded Goldglas, техлід інфраструктурної команди

Підсумки

1. Овнершіп: команди повністю підтримують стабільність свого домену / певної фічі самостійно.

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

3. Уніфікація: зводимо все до одного стилю, щоб знизити поріг входження розробників в нові домени.

4. Пріоритизація: не соромимося витрачати робочий час на якісний код рев’ю. Мінімалізм: мінімум файлів, мінімум рядків, зауваження — тільки по суті.

5. Відповідальність: тестування коду на стейджингу, моніторинг графіків на проді після деплою.

6. Комунікації: збереження атмосфери доброзичливості та взаємоповаги важливіше власних уподобань щодо написання коду.

Заапрувлених вам PR-ів, і «нехай проблеми та незгоди не роблять вам в житті погоди».

👍ПодобаєтьсяСподобалось10
До обраногоВ обраному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
Код-рев’ю не допомагає стовідсотково уникати багів на проді.

Ось це одна з головних речей які треба усвідомлювати. Бо доводилося стикатися зі спробами розділити відповідальність за баги між головним девелопером та рев’ювером. Але у рев’ювера дебагер в голові не встановлений. Також не встановлений інструмент для аналізу покриття тестами.

Якщо це не помилки zero-level

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

Не зовсім зрозуміла контекст, Ви зараз маєте на увазі співбесіду в Fiverr?

Нет я тестовое делал, вроде как сделал, но получил такой ответ:"Я куратор Yalantis Golang School, Элина. Общались с вами ранее по поводу учаcтия в школе. Как я и сообщала ранее, вы успешно выполнили тестовое и далее я предложила вам перейти на следующий этап отбора. К сожалению, у нас с вами не получилось договориться о созвоне и на сегодняшний день мы не можем рассмотреть вашу кандидатуру на участие в школе." Хотелось бы получить от них кодревью, для начинающего разработчика, оно лишними не будет...Как мне правильно написать, что бы они мне выслали кодревью, а не послали сами знаете куда...

На жаль, я не можу Вам допомогти з цим питанням, адже не знаю як налаштовані процеси в Yalantis Golang School та не маю до них відношення. Зазвичай, компанії не роблять цього, адже бажаючих отримати код рев’ю завжди дуже багато, а ресурси компанії на зовнішній код рев’ю дуже обмежені. Проте, гадаю, що Ви можете запитати у куратора чи мають вони наразі можливість зробити код рев’ю для Вас, тому що Ви праві в тому, що код рев’ю дуже корисний для початківців.

Вы меня не так поняли! Если тестовое выполнено, должны ли мне дать коде ревью, или оно дается на этапе собеседования, только кандидатам, которы что то зафейлили в тестовом?

Ніхто нікому нічого не винен. Але ніхто не забороняє вам сказати їм, що вам би хотілося почути якийсь фідбек по тому, що ви написали.

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

LGTM

А мене зацікавив момент з відсутністю QA, якось він не повністю розкритий. Їх же не зовсім немає, просто в цикл розробки-багфіксу на розробника лягає більше тестування?

На разработчика падает ответственность за баги в проде. Количество багов в проде необязательно зависит от количества тестирования

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

І юніт-тести у вас покривають весь код? Чи ви змушуєте розробників тестіти по-повній всі потенційні заафекчені змінами частини продукту?

Скорочу до змісту: Нам приходить з десяток PR-ів кожного дня і у всіх треба пересвідчитись

Дякую! Дуже цікаво було почитати!

Стаття цікава, дякую!

В команді, де я працюю, діє обмеження в 200 рядків на 1 PR.

Але тут не знаю, з одного боку ніби так, але з іншого — якщо рефакторинг, то закидати багато ПРів, де по 2 класи відрефакторили в кожному?

Дякую за відгук!
Взагалі, це залежить від контексту. Мета цього обмеження — уникати ситуацій, коли додається багато бізнес-логіки в одному ПРі, адже такі ПРи важно адекватно рев’ювити. Проте, якщо це рефакторинг (без зміни логіки!) або інфра зміни, які треба залити одним комітом, то немає вимоги розбивати це на маленькі ПРи.

Цікава стаття! Та й оформлена гарно.
Особливо тішить, шо стаття написана державною мовою.
Дякую. Пишіть ще!

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