×

Код-ревʼю з двох кінців: як підготувати свій код та провести ревʼю чужого

Усі статті, обговорення, новини для початківців — в одному місці. Підписуйтеся на телеграм-канал!

Усім привіт! Мене звуть Данило Толмачов, я Full-Stack розробник та Team Lead в Techstack. За понад 10 років професійного досвіду я провів тисячі код ревʼю (CR) для різних стеків та мов програмування, а також, звичайно, багато разів готував свій код до ревʼю. Спираючись на це, я виявив загальні правила код-ревʼю та сформував декілька підказок, що допоможуть уникнути помилок.

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

Навіщо потрібен CR

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

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

  1. Уникати дрібних помилок, у тому числі і через неуважність. Кожному потрібні очі, які можуть оцінити ваш код, якщо ви щось пропустили або недогледіли. Описка, не переданий параметр у функцію чи метод — все це може помітити інша людина, коли ви вже не можете бути достатньо уважним.
  2. Отримувати погляд від людей з досвідом, відмінним від вашого. У кожного з нас у багажі є унікальний досвід, якого немає в інших. Якщо на ваш код подивиться людина з іншим бекграундом, вона зможе привнести нові ідеї та рішення. Корисним може бути віддати код на ревʼю тому, хто працював на проєкті до вас і може розказати, які базові принципи були закладені при його створенні. Також гарною ідеєю буде показати код людині, що має досвід в іншій індустрії (наприклад, якщо ви працюєте над healthcare-продуктом, а ваш ревʼюєр — з фінансовим). Така людина може підказати вам рішення, що не є розповсюдженими в індустрії вашого продукту, але добре підходять для вирішення його проблем.
  1. Обмінюватися досвідом і бути на одній хвилі. Добре, коли люди в команді можуть підтримувати та підміняти одне одного. Знаючи код, що пишуть ваші колеги, ви зможете набагато легше почати працювати в тій самій зоні, якщо доведеться. Так само й вони зможуть швидко та безболісно перейти в зону вашої роботи за потреби.

Тож не ігноруйте CR та не бійтесь його. Витрачена година часу зараз на рев’ю збереже нерви й заощадить години на підтримку в майбутньому.

Як підготувати ваш код до ревʼю

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

  • Автоматизуйте все, що можете. Спрощуйте все: використовуйте статичні аналізатори, які допомагатимуть виявляти code smells і security vulnerabilities, раннери, що запускатимуть усі тести (unit, integration, E2E, etc.), і style guides, що позбавлять вас потреби думати про розстановку крапок і ком. Автоматизація підвищить якість вашої роботи, заощадить час і допоможе змістити фокус рев’юера з перевірки самого коду на його відповідність бізнес-проблемам, які код має вирішувати.
  • Уникайте «by the way changes». Подавайте на ревʼю лише те, що прописано у тасці, та не путайте вашего ревʼюєра. Це не значить, що ви не повинні покращувати той енвайронмент, в якому ви працюєте. Це значить лише те, що все повинно бути добре систематизовано та пов’язано. Якщо ви бачите баг, що не стосується ані зони, де ви працюєте, ані коду, якого ви торкаєтесь, заведіть окремий таск або тех дебт, і запускайте його у процес окремо.
  • Документуйте свій код. Код є найважливішим поясненням того, як працює та чи інша фіча. Він вже сам по собі може багато вам розповісти. Тому для того, щоб він максимально ефективно пояснював себе сам, не забувайте коментувати та оновлювати його документацію, приділяйте увагу його структурі та назвам, які використовуєте. Коли ви пишете код — ви використовуєте мову. Тому ваш код повинен розповідати історію того, що виконується та як.
  • Зменшуйте всі пул-реквести. Декомпозуйте свій код та тримайте пул-реквести у розмірі до 500 рядків бізнес-коду. За даними дослідження Smart Bear & CISCO, перевірити навіть понад 300-400 рядків коду без відволікань та отримання додаткової інформації якісно не вдається.

    З мого досвіду, 500 рядків бізнес-коду (без коментарів, нью-лайнс та форматування) — це максимальний розмір, який можна якісно обробити на ревʼю. Тому рекомендую з повагою ставитись до ревʼюєра та не пропонувати йому оглянути більше цього обсягу.

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

Як виконувати CR

Тепер поставимо себе на сторону ревʼюєра та спробуємо розібратися, якими принципами керуватися, коли вам треба дати зворотний зв’язок на чужий код.

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

    Нехай propose instead of blame стане вашим керівним принципом у цьому процесі. Тому, якщо критикуєте, завжди пояснюйте, за що та чому, а головне — пропонуйте натомість можливі варіанти покращення.

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

Correctness. Іноді розробники взагалі не розуміють, навіщо вони пишуть код і яку проблему він вирішує. Щоб уникнути такої ситуації, можна запитати себе: Чи робить цей код те, що повинен? Чи справляється він з едж-кейсами? Його можна адекватно протестувати? Наскільки правильно і повно його було протестовано? Чи добре він справляється з конкретним юз-кейсом?

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

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

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

Тому код має бути написаний зрозуміло, а це означає, що він:

  • відображає те, що написано в бізнес-реквайрементах;
  • коректно використовує назву змінних, функцій і класів;
  • спирається на загальноприйняті конвенції.

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

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

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

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

Життєвий цикл CR

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

  1. Перше (авторське) ревью. Як я вже згадував, перш ніж передавати код далі, перевірте його самі. Це допоможе позбавитись поверхневих помилок.
  2. CR демо (опціонально). Не завжди ваш код буде зрозумілим сам по собі. Бувають важливі фічі, які мають велике значення для бізнесу, або буває так, що ви працюєте з якоюсь новою технологією, в якій ви вже розібралися, а інші розробники — ще ні. Для таких випадків CR демо допоможе краще зрозуміти контекст і, відповідно, ваш код.
  3. 1-on-1 час ревʼюєра з вашим кодом. Це стандартний процес і основа CR. На цьому етапі ревʼюер, керуючись принципами, які ми щойно розглянули, перевіряє авторський код та надає зворотній звʼязок.
  4. Відмова або успішне проходження ревʼю. Після ревʼю ваш код може бути або успішно прийнятим, або такимм, що потребує корективів. У разі, якщо ваш код не пройшов рев’ю, тікет переводиться в колонку not started. Через це може зрости час на зворотний зв’язок, оскільки тікету доведеться знову пройти CR. Тому автору потрібно стежити за тікетами, за які він відповідає, і самостійно нагадувати рев’юеру про їхній статус. У разі успіху тікет переміщується далі по борду та наближається до релізу.
  5. Conflict resolutions. Це конфлікти вашого тікету з іншими на момент інтеграції тощо. Уникнути конфліктних ситуацій допоможе колаборація з іншими розробниками, які теж працюють у цій зоні. Домовтеся, що спочатку зробите пересічні тікети, а потім — усі інші.

Що маємо в кінці

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

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

Усім якомога більше апрувів та менше реджектів!

👍ПодобаєтьсяСподобалось27
До обраногоВ обраному14
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

Статті не завадило б рев’ю :)
Є описки, як-то imput, такимм тощо.

Уникайте такої великої кількості сленгових та транслітерованих слів:
«едж-кейсами» — граничними випадками,
«енвайронмент» — оточення,
«реквайрементах» — вимоги
і багато іншого. Деякі не зрозуміло, як правильно писати, наприклад скільки «п» у «апрувів».

Також є зайві коми, наприклад:

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

Хоча тут можна послилатись на авторські знаки.

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

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

За понад 10 років професійного досвіду я провів тисячі код ревʼю (CR)

невеличка математика:
не зрозуміло скільки конкретно під фразою

тисячі код ревʼю

.
але якщо спробувати вгадати, то це більше однієї тисячі, але менш як 10 тисяч, тому що тоді було б сотні код рев’ю і десятки тисяч код рев’ю
візьмемо середня 5к код рев’ю за 10 років.
5000/10 = 500 рев’ю що року
якщо в році 254 робочих дні, то виходить 2 код рев’ю кожного дня упродовж 10 років...

щось не віриться)

> Уникайте «by the way changes».

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

Refactoring. Tech debts. Окрема таска зі своїми definition of ready/estimates etc. Бажання фіксити все навколо у ревьювера одразу відпадає, коли все заганяється під єдиний процес розробки. Якщо важливіша нова фіча а тім лід час на технічний долг не може виділити — то все, приїхали. Не треба брати на себе відповідальність за те, що можеш зробити але не маєш. Бо таска сама по собі вказує на рамки.

Хочу додати, що це насправді працює тільки в аутсорсі або досить забюрократизованому продукті. В стартапі понять «definition of ready/estimates» майже ніколи не існує (судячи з мого досвіду в 10+ стартапах), як і «єдиний процес розробки», тож такі «пропонування» від рев’ювера це норм практика, бо інакше воно ніколи не пофікситься. Головне імплементувати їх окремо від фічі, наприклад як PR based on PR, щоб завжди можна було замерджити саму фічу і рефакторинг не став блокером.

З приводу 500 рядків у PR — це далеко не проблема, якщо автор коду зробить нормальну історію коммітів. Я зазвичай великі таски разбиваю на підпункти, і кожен пункт комічу окремо. Тож дуже часто (так, не завжди), мої великі PR можна порев’ювити окремо по коммітам. Якщо у вас 10 коммітів над один підпункт, їх можна просто об’єднати в один.

Ну і навіть якщо людина просто викатила величезний PR без нормальної історії коммітів (тобто комміт #10 може переписати коміт #1, тож може здатися що не має сенсу рев’ювити коміт #1), все одно можна відкрити комміт і фінальний diff поряд один з одним і без проблем це опрацювати.

Є стародавній жарт про те, що ревьювер отримавши пул реквест на 2 строки знайде там 3 помилки, а на 2000 строк скаже, що все ідеально)

А є сурова правда про те, що якщо всім начхати, то і в двух рядках критичну помилку не знайдуть, а якщо не начхати, а фіча велика, то є способи нормально порев’ювити, а не розповідати про «є дослідження, у мене лапки». Звичайно в ідеалі робити ПРи малими, але світ не ідеальний)

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

Звичайно, я цього й не спростовував. Але ідеальний світ закінчується там де є великі гроші, а справжня реальність починається там, де за тиждень треба викатити модуль, а не фічу на 8 годин чи багфікс. Це самий звичайний стартап або ж малий/середній продукт. І таких проектів переважна більшість. Я не спростовую ваших тейків про «як робити ідеально». Напевно мені просто хотілося б мати інформацію про те, як зробити болючий процес менш болючим, тому і додав цей комент. Бо будучи умовним джуном дана статя для мене виглядала б як «ось десь там існують райські процеси, почитаю про це, і повернусь до свого болота».

Можливо, базуючись на своєму досвіді, ви б могли створити допис про те, як вирішувати конкретні болячки з конкретними прикладами? Бо як каже одна досвідчена людина яка творить курси по фронтенду: «спочатку треба показати як робити погано, а потім показати як з цього зробити краще». Ось тоді value допису виросте кратно імхо.

Дуже «просто» ревювати такий ПР якщо ці коміти в одному файлі, ще й памятати що саме поревював в попередньомк коміті. А коментарі як писати?

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

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

Та і взагалі, а зараз ви типу просто не рев’ювите якщо ПР великий чи що? Те що я описав це лише логічний варіант того, як покращити якість такого рев’ю. Не всюди він спрацює, бо не всюди є на це час й професіоналізм команди (комміти треба ще гарно створити). Є й інші способи.

Та і взагалі, а зараз ви типу просто не рев’ювите якщо ПР великий чи що?

Є ж різниця ревювавати ПР як одне ціле, і ревювати по комітно? Особливо якщо там коміти типу «fix previous commit» і інша срань.
По хорошому коли ПР сквошнутий, нащо мені бачити ті проміжні коміти

Ну так і я ж про це, що всю срань можна сквошнути до одного комміту який «додає авторизацію через сервіс #1», а сквош наступної срані «додає авторизацію через сервіс #2». І якщо в сумі це величезний ПР, то по-нормальному створені і сквошнуті коміти дозволять порев’ювити покомітно, якщо важко рев’ювити все одразу. Може я не розумію вашого посилу, але ж наче про одне і те ж розмовляємо, ні?)

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

От реально не бачу ніякої зручності у тому.

А якщо коміт #1 вносить зміни у тій же стрічці що коміт #2, який з них правильний?

Слухайте, ви наче не читали що я писав.

Тож дуже часто (так, не завжди), мої великі PR можна порев’ювити окремо по коммітам.

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

Мати для кожного ПР свій тул сет і різні способи для проведення ревʼю, на мій погляд, значно ускладнює процесс. Та, з великою вирогідністтю, звужує коло ревʼюверів, тому що кожен хоче отримати якісне ревʼю, але не кожен може його дати.

Так я ж і не пропоную

для кожного ПР свій тул сет і різні способи для проведення ревʼю

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

В своїх командах я завжди намагаюся будувати якісні прроцеси та вводити правила, які сможуть спростити життя чи вберегти від помилок. Проведення код ревʼю не повино бути викликом.
Бувають і великі ПР, коли зводеться велика фіча, чи збирається великий реліз, але точно це не повинні бути кожне 1-2 ПР. І навіть всі ті великі ПР були колсь маленькими. 300-500 рядків бізнес-коду — це золоте правило на яке орієнтуюся я і яке вже допомагає моїм товаришам по команді

Дякую за статтю.
Додам до піраміди пункт Performance. Між Correctness та Security.
Критично важливо бути певним, що після деплою швидкість роботи системи буде відповідати нефункціональним вимогам.

По темі і без води. Дуже структуровано. Дякую. Пишіть ще.

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