Code review. Які є основні правила перевірки коду

Я — Євгеній Вінійчук, у розробці вже понад 7 років. У компанії Youshido виріс від Trainee до Lead Front-end Developer. Створював сайти та застосунки як для стартапів, так і для бізнес-гігантів — Viasat, Jacobs, Pepsi, Tuborg тощо. Зараз поєдную функції тімліда та Senior Software Engineer. У цьому матеріалі розкажу про власний досвід code review, як ми його організували в Youshido, які правила я засвоїв і на які помилки звертаю увагу.

І спочатку був код...

Додати кнопку на сайт, замовити зворотний дзвінок або розробити новий розділ застосунку — для кожного з цих завдань розробники пишуть код окремо, подібно до гілок, які відростають від стовбура дерева. Щоб бути впевненим у правильності виконаної роботи, розробники перевіряють код одне одного під час code review.

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

Та з часом, коли команда почала розростатися, коли прийшли нові стажери й джуніори, які потребували фідбеку, з’явилась потреба в повноцінному code review. Друга причина: ми були зацікавлені в тому, щоб «виростити» під себе команду. А зростання не може відбуватися без повноцінного діалогу та рецензування роботи.

Тож завдання просто протестувати код розрослося до низки інших:

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

А далі було review

Механізм code review достатньо простий: написали код — перевірили — внесли правки — протестували. Наприклад, з’явилося завдання додати кнопку на сайт. Воно закріплюється за певним розробником. Він створює собі гілку від основного коду, тобто паралельну історію змін. Там пише все, що потрібно для появи кнопки на сайті. Коли робота виконана, розробник створює merge request, де вказує своїх рев’юерів.

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

Ніби нічого особливого. Але, як завжди, вся суть у деталях. І я виписав собі деякі зауваги, на які зважаю в роботі.

Як робити не треба, або «Вийди і зайди нормально»

Не змішувати стиль коду, бізнес-логіку та рефакторинг. У мене бували випадки, коли у файлі, який давно не редагували, розробник разом з виконанням задачі міг відформатувати стиль коду. Наприклад, усі подвійні лапки перетворити в одинарні чи поміняти форматування коду. У результаті мені приходив файл зі змінами у 80% рядків, тоді як треба було просто змінити назву розділу (1 рядок). Це типове змішування задач різного типу, яке змушує вишукувати зміни та не дає сфокусуватись. (Найімовірніше, я одразу поверну такий реквест із проханням розділити зміни.)

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

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

Не забувайте, навіщо ви це робите in the first place. Дуже круто, коли компанія може витрачати на code review стільки часу, скільки потрібно, а розробники можуть дійти до найкращого рішення. Легко впасти в азарт під час пошуку ідеального підходу й забути про те, що основна функція, яку виконує розробник, — це вирішення завдань бізнесу. Якщо часу обмаль, а задача потрібна «на вчора», то треба забезпечити її розв’язання якнайшвидше. В такому випадку я пропоную приходити до good enough рішення, яке може «прожити» певний час, а паралельно з ним створювати задачу на закриття технічного боргу.

Він з’являється тоді, коли ми починаємо «зрізати кути», бо розробнику треба якнайшвидше виконати завдання або проєкт не готовий до нових бізнес-потреб. Але після виконання завдання до цих частин обов’язково треба повернутися, інакше проєкт буде важко розвивати й з’являться помилки в тих місцях, де ніхто не очікував. Щоб не забути, що саме треба виправити, раджу вести перелік таких місць. Найефективнішим засобом для мене виявилось розставляти коментарі @todo, які потім можна знайти й уже все доробити. До того ж IDE надає зручний функціонал з пошуку саме @todo.

Критикуй без фанатизму, бо лишишся без джуніора

Не варто забувати про те, що code review — це оцінювання та критика чужої роботи. І для збереження здорових відносин у колективі та для мотивування колег варто робити це екологічно. В мене є такі основні правила:

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

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

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

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

2. Вести активний діалог. У процесі перевірки коду дивлюся на написане з колегою, і ми разом думаємо, чи можна це рішення покращити. Ви граєте на одному рівні. Для того, щоб це не перетворилося на «роби, як я сказав», можна ставити питання на зразок «Як думаєш, чи можна це переписати краще?» й після відповіді співрозмовника запропонувати власну позицію, яка, найімовірніше, й буде прийнята.

Одного разу ми з колегою зідзвонилися для код-рев’ю, проте він не давав активного зворотного зв’язку. Без фідбеку з «тої сторони» складно підсумувати результат роботи. Бо в кінці ми разом маємо отримати план удосконалення поточного рішення або погодитись, що зміни не потрібні. Тоді я почав ставити питання, які допомогли розкритися, і діалог таки запустився. Виявилося, що у колеги не було досвіду схожих рев’ю, він не знав, як саме це має відбуватись. З часом він почав активно висловлювати свою думку, і робота пішла. Відтак проактивним діалогом вдалося розв’язати кілька завдань.

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

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

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

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

Під час код-рев’ю виявляється чимало помилок, які можна виправити, зменшити кількість циклів back-and-forth при роботі з QA, а значить збільшити швидкість розробки. Окрім того, завдяки перевірці коду підвищується загальний рівень знань про проєкт, що зменшує так званий bus factor.

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

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

👍НравитсяПонравилось18
В избранноеВ избранном12
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

Тут в комментариях писали, и я соглашусь, что для качественного код-ревью, нужно иметь принятый code convention.

Чтобы не спорить о форматировании кода, или каких-либо подходах, принципах, и тд, а ссылаться на code convention. В идеале контролировать его соблюдение автоматически (линтеры, форматтеры).

Ну и добавлять чеклисты при создании МРов. Чтобы автор МРа мог проверить сам себя перед созданием МРа. Например не забыл ли он добавить тесты, протестировать на мобильном устройстве, ну и тд.

Что делать, если например более опытный тимлид советует в комментах изменить стиль кода, а автор отвечает, что не видит в этом смысла? То есть считает что прав и пишет идеальный код.
Эффект Данига-Крюгера во всей красе.
Можно настаивать и блокировать мерж, тем самым ухудшить отношения. Можно закрыть глаза и ухудшить качество кода, пропустив. Есть ли паттерны решения таких ситуаций?

Есть ли паттерны решения таких ситуаций?

youtu.be/vuBWbpTJRqk

Что делать, если например более опытный тимлид советует в комментах изменить стиль кода, а автор отвечает, что не видит в этом смысла? То есть считает что прав и пишет идеальный код.

Это формализовано и задокументировано на проекте? Если нет, то пусть идет лесом.

Я бы сказал, что тимлиду стоит учиться объяснять почему он считает, что его совет действительно делает код лучше. Если объяснить не может, но чувствует, что «так точно лучше» — об этом почти всегда можно сказать, ведь если 95% предыдущих решений тимлида были верными, то вырабатывается кредит доверия. Имхо, вот такой тимлид должен быть (поставил на таймкод, буквально 2 минуты слушать): youtu.be/xHa2FFyK67M?t=1255

Если это вопрос конкретно код-стиля, то он на проекте единый и просто не подлежит обсуждению, unless замечание девелопера реально в тему и все «переступают через кучу мусора» просто потому, что так привыкли. В любом случае, если решение автора как минимум не хуже, всегда можно проявить эмпатию и сказать «смотри, твоё решение тоже подходит, но сейчас на проекте другой стиль и мы не готовы вносить эти изменения повсеместно, по таким-то причинам». Таким образом человек почувствует, что его услышали и конфликт сойдет на нет. А если автор реально предлагает хорошее, то почему не имплементить?

Ну и нельзя полностью избежать ситуации, когда каждый останется при своем мнении, со временем таких ситуаций будет сильно меньше. Есть код-стиль проекта, если автора не смог «продать» свои пожелания, значит едем дальше. Как тимлид учится объяснять, почему его совет имеет место быть, так же и автор кода учится «продавать» свои решения.

Самый простой вариант — настроить автоформат кода при коммите. Уровень cleanup-а тоже настраивается www.jetbrains.com/...​esharper/CleanupCode.html

Дякую за статтю, але я б її перейменував в «Code review для фронт-енду».
І ще такий момент. У вас в статті ревьюер — дуже замотивований співробітник, якого хлібом не годуй, дай якісне code review зробити. Але ж в житті так не буває, і code review стоїть для більшості на останньому місці в списку пріоритетів. Тому хотілося б задати кілька питання:

1) Як ви мотивуєте співробітників робити якісні питання, а не відписки?
2) У вас в проекті code review робить вся команда або призначена людина?
3) Якщо девелопер робить явну помилку в коді, а ревьюер її пропускає, хто з них двох несе відповідальність за production bug?

Не ТС, але спробую відповісти:

Як ви мотивуєте співробітників робити якісні питання, а не відписки?

1. Їм же далі з цим кодом працювати. Тому явну лажу відсікуть. Так, кожен може щось пропустити, але не навмисно.
2. На ревʼю гарантується виділення часу (грубо кажучи, 1/4 кожного дня). На практиці виходить значно менше.
Ну і якщо хтось невідповідальний — це буде видно по іншій роботі. Так, це надто індивідуально, але поки що працювало.

У вас в проекті code review робить вся команда або призначена людина?

Під-відділи по 3-4 людини в яких — всі. Автомат дозволяє submit при наявности хоча б одного +2 і відсутності −2, і коли CI затвердив з боку тестів. Для деяких співробітників правила складніші. Правила постійні і записані в конфигурації Gerrit, але можна добавляти і видаляти. Так, народ по більшости поводиться розумно.

Якщо девелопер робить явну помилку в коді, а ревьюер її пропускає, хто з них двох несе відповідальність за production bug?

Обидва. Хто поставив +2 той приймає свою долю відповідальности. Але автор — більше відповідальний, десь 3:1. Випадків якогось явного покарання я не бачив (може, у сусідів було, хз), але всі знають, що лажу робити не треба :)

1. Їм же далі з цим кодом працювати.

www.linkedin.com/...​-6837296073162182656-wQdn

2. На ревʼю гарантується виділення часу (грубо кажучи, 1/4 кожного дня). На практиці виходить значно менше.

на практиці виходить «пройти ревью як окремий челенж та скіл»

Обидва. Хто поставив +2 той приймає свою долю відповідальности.

тому за звичай «зробити ревью» усі прямо аж горять шо дим коромислом ))

/...​-6837296073162182656-wQdn

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

на практиці виходить «пройти ревью як окремий челенж та скіл»

Не знаю, у нас ніколи принципових проблем не було.

тому за звичай «зробити ревью» усі прямо аж горять шо дим коромислом ))

Ну я поки не бачив тут проблем. Може, гарні команди весь час попадались...

1) Як ви мотивуєте співробітників робити якісні питання, а не відписки?

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

2) У вас в проекті code review робить вся команда або призначена людина?

Призначено декілька людей, али ми підтримуємо постійну ротацію

3) Якщо девелопер робить явну помилку в коді, а ревьюер її пропускає, хто з них двох несе відповідальність за production bug?

Відсутня диференціація як така, тобто, якщо пропустили серйозний баг — робимо ретроспективу щоб зроміти «як так» та які процеси покращити, загалом відповідальність несе вся команда. При цьому важливо зауважити, що і рев’ювер, і розробник обговорюють, як уникнити пропуск такого типу помилок у майбутньому.

У результаті мені приходив файл зі змінами у 80% рядків, тоді як треба було просто змінити назву розділу (1 рядок).

Мы с командой договорились, что принимаем такие ПР-ы на ревью в случае, если логика и код-стайл разбиты по коммитам и кол-во изменений не выходит за рамки разумного.

Соседи по обсуждению потребуют расписанные в мельчайших деталях критерии рамок разумности ;\

Код ревью должен быть основан на документах и на каждое замечание должна быть ссылка на документ.

на документах

На документах каких и для чего?

на каждое замечание должна быть ссылка на документ.

Кому должна?

На документах каких и для чего?

Вот простой пример
Ревьювают код С++, метод класса возвращает const & на член-объект, сам объектик этот имеет еще десятка два других членов, где то pair, где то int, где то float[8] (фиксированный). Выделения динамической памяти нет.

Одна группа людей ставит const & для оптимизации (метод вызывается много раз), вторая — выпиливает и ставит возврат-по-значению под аргументом «вдруг сохраненная ссылка по невнимательности переживет этот объект? как мы в треде будем краш ловить?».

Документ на код ревью утрясает такие проблемы, а без него — будет игра в футбол ссылкой-значением. Т.к. нет договоренности.

Понятно. Но покрыть _все_ такие случаи каким-то документом просто невозможно, ну и это хороший пример того, что разные задачи могут потребовать разных подходов, каждый из которых сам по себе будет разумен. Поэтому всегда остаётся место для такого когда-то знаменитого правила.
Но вслед такому обсуждению можно и часто нужно установить уже какое-то локальное правило (в соотв. документе) «тут мы делаем так-то, решение принято тогда-то в обсуждении», для кода тут достаточно номера тикета в комментариях и более подробного описания в тикете.

Да
Но документ это всего лишь «сборник решений из тикетов». Что упрощает онбоардинг новобранца. Что делать если решения раскиданы по тикетам, а у вас +2 июня и +1 мидл вышли работать за 2 недели?

Что упрощает — бесспорно. Что надо собирать в одно место — тоже.
Но когда говорят

на каждое замечание должна быть ссылка на документ.

это уже перегиб.

это уже перегиб.

Да... Ссылку можно приводить если замечание не было понятно. А так все находится в чеклистах, про которые должны знать разработчики.

А так все находится в чеклистах, про которые должны знать разработчики.

Это не работает. Если много пунктов в чеклистах или много кода, где-то что-то обязательно будет потеряно.
Я бы тут больше возлагался на автоматику, где она возможна.

Без документов будет либо номинальное, либо непроходимое ревью. А напишешь много документов никто читать не будет, имхо тут задача лида стандартизировать привычки и предпочтения, а потом по возможности это задокументировать

Без документов будет либо номинальное, либо непроходимое ревью.

Это преувеличение. Всякие style guides нужны, но не фатальны.

А напишешь много документов никто читать не будет, имхо тут задача лида стандартизировать привычки и предпочтения, а потом по возможности это задокументировать

Да. Но потом всё равно помнить, что никакой документ не покроет все случаи проблемы.

по-нормальному все что не описано не может быть причиной для изменения

Вы описываете мёртвое состояние уже завершённого проекта.

Ты ошибся с темой. Здесь про программирование, а не про юриспруденцию.

Не знаю, як в C ++, але в Java є конвенції, які добре і описані, і задокументовані, так що посилання дати нескладно.

Ніякі конвенції не покриють все, повторюсь. А якщо це тільки питання як назвати — strangeValue, strange_value чи якось інакше — то вони надто банальні, щоб обговорювати. А от наприклад використовувати streams чи явний цикл — тут може бути справжній холівар типу «це ж гарячий шлях, не можна такого!»
В C++ в різних світах різні стилі, але можна визначити якого притримуватись. У нас оформленням коду взагалі тулза займається — вона дурна, але питання стандарту вирішує, і нема такого що «а мені подобається відступ 3 пробіла!»

Не змішувати стиль коду, бізнес-логіку та рефакторинг. У мене бували випадки, коли у файлі, який давно не редагували, розробник разом з виконанням задачі міг відформатувати стиль коду. Наприклад, усі подвійні лапки перетворити в одинарні чи поміняти форматування коду. У результаті мені приходив файл зі змінами у 80% рядків, тоді як треба було просто змінити назву розділу (1 рядок). Це типове змішування задач різного типу, яке змушує вишукувати зміни та не дає сфокусуватись. (Найімовірніше, я одразу поверну такий реквест із проханням розділити зміни.)

А как же принцип «Refactor as you go»? Часто это вообще единственная возможность что-то зарефакторить. Выглядит так, что у вас код для ревью, а не ревью для кода.

Задачу по рефакторингу треба описати в джирі та покласти в беклог (далеко далеко що б до неї в ПО ніколи не дійшли руки).

Я думаю, имелось ввиду разделение на разные ПРы или хотя бы разные коммиты, чтобы не было каши. Реально неудобно, когда нужны минорные изменения, а тебе прилетает 1 ПР из 1 коммита с 30-50 файлами, не относящимися к задаче

«Refactor as you go» це дійсно корисна штука, я тут трошки про інше, коли разом із вирішенням задачі отримуєш такий рефакторинг, на тестування якого потрібна була б окрема задача. Або коли рефакторинг в коміті взагалі не стосується задачі. В результаті це гальмує, а інколи унеможливлює деплоймент основної фічі, заради якої задача була створена з самого початку

Або коли рефакторинг в коміті взагалі не стосується задачі.

А в чем проблема? Читаешь код во время выполнения задачи. Видишь — насрано. Берешь убираешь, раз зашел. Так быстрее всего. А то пока задачу сделаешь, пока объяснишь в тикете, пока другой Вася в нее въедет, пока позадалбывает вопросами, так пройдет больше времени, чем просто сразу сделать. А если еще какой-то человек со скрам-дипломом захочет ее оценить и запланировать в спринт, то изначальное время вообще можно умножить на 3.

А в чем проблема? Читаешь код во время выполнения задачи. Видишь — насрано. Берешь убираешь, раз зашел. Так быстрее всего.

Это хороший идеал, но он может быть несовместим с «вот это надо заделиверить на завтра, хоть тушкой хоть чучелом».
Я лично считаю, что надо резервировать на вычистки-рефакторинги, например, 20% времени. Но как это доказать эффективным менеджерам всех видов и мастей...

И в этом месте мы приходим к тому, с чего я начинал :-)
Именно из-за эффективных менеджеров, любящих играть в «Дженгу наоборот», рефакторинг почти всегда должен идти в обертке фичи. Или его просто не будет.
Качество кода волнует девелопера, фичи — менеджера. Менеджера не волнуют проблемы девелопера. И тут бы, казалось, мы глухом углу. Но, поскольку девелопер умнее, он может спрятать то, что нужно ему (рефакторинг), в обертку того, что нужно менеджеру (фича). ;-) Это как ребенку дают лекарство в шоколадке.

Мои девелоперы с синдромом менеджера срезали такие ревью — «а зачем ты это сделал», «скоуп стори же еще один профиль в адас», «аджайл гласит максимизировать обьем не сделанной работы, давай следовать гайду?», «фреймворк и темплейты на это? не рано ли? а если не понадобится?». Иногда не знал кому давать на ревью что б прошло в мастер, хотя и после пуша прилетали фидбеки устно...

«скоуп стори же еще один профиль в адас»

Эээ... можно расшифровку? Гугл бессилен.

аджайл гласит максимизировать обьем не сделанной работы

Это просто высокопарный вариант «не нужно — не делай»? Тогда не совсем понятно, как оно относится к рефакторингу.
Насколько я понимаю, там достаточно очевидно, в духе, если увидел проблему, которая неминуемо вылазит прямо сейчас — надо решать. Может быть вопрос в количественных оценках (например, шаблонизировать от 2 очень схожих реализаций или хотя бы от 3?), но с какого-то момента вопроса по сути уже нет.

Эээ... можно расшифровку? Гугл бессилен.

ADAS прикладной протокол для передачи по сети машины информации о дороге. Состоит из сообщений «профилей» — profile.

Это просто высокопарный вариант «не нужно — не делай»?

Нет. Вот я впиливаю новую фичу. Вижу что это вставляется в код уже в третий раз как получится, а если поправить архитектуру, то решение становится масштабируемым и в будущем стори поинты для впиливания подобной фичи уменьшатся с 8 до 3 (а таких маленьких фич планируется много).

ADAS прикладной протокол для передачи по сети машины информации о дороге. Состоит из сообщений «профилей» — profile.

А, это уже прикладная специфика. Такой ADAS нагуглился, но в данном контексте он уже не важен, просто «скоуп xxx, а ты делаешь что-то за его пределами».

Вижу что это вставляется в код уже в третий раз как получится, а если поправить архитектуру, то решение становится масштабируемым

Я думаю, с 3 раз уже становится понятно любому, что есть какая-то система. А иначе как? Ждать 6-го раза чтобы наконец до последнего дошло?
Вот это таки имеет смысл формализовать в общих правилах: как только что-то повторяется 2 раза — планировать его объединение, 3 раза — объединять в рамках выполнения этого третьего тикета. И объяснение — два параллельных места ещё можно осилить после копипастинга встречными патчами, на третье уже типовой девелопер не здёбный.

А как же принцип «Refactor as you go»?

У нас действует принцип — не смешивать в одном коммите. А вот чтобы какой-то рефакторинг прошёл в рамках той же задачи — нормально, если в сумме задача не превышает 2-4 дня, иначе выносить.

коммите или мердж-реквесте?

Именно коммите.
Если я правильно понимаю, что такое merge request в вашем процессе, это набор коммитов для одного тикета, и таких наборов в идеале один, но может быть много. Всё равно когда первый коммит — reindentation давно не троганного файла, второй — рефакторинг, третий — фича, это нормально — и хорошо анализируется по отдельности.
Если что — у нас сейчас Gerrit. Как такового merge request у него нет, но есть возможность строить цепочки коммитов, которые потом будут включаться в целевую ветку все сразу. Наверно, это близко.

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

когда дальше идут фолловапы на замечания, то проще посмотреть последующие коммиты, чем все по новой, так что и так и эдак

більшість переглядають діфф merge request’а разом

Еее... уявімо в ланцюжку:
1) Прогон автоформатером
2) Масове перейменування
3) Виправлення багу
4) Рефакторінг — перенесли код з 3 місць в одне з узагальненням
5) Нова функціональність (окремий клас і ще 3 методи в інших)

Що ви побачите в тій каші, що діфф разом???

А так — кожен можна перевірити. Запустити автоформатер у себе, виконати ті ж перейменування в своєму IDE, порівняти результати коду у обох випадках. Інші — перевірити вже очами як звичайно.

Ах так, ще автотести, авточек коду починаючи від форматування, і т.д. — за специфікою місця.

5) Нова функціональність (окремий клас і ще 3 методи в інших)

Що ви побачите в тій каші, що діфф разом???

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

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

плакалЪ (к) (тм)

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

Що треба робити щоб огірки ложкою банка майонезу кит (tm)

Я ніцъ не розпарсив.

плакалЪ (к) (тм)

Це якщо потрібно його перевірити. Звичайно не треба, але я узагальнив той приклад.

Гарна стаття! І тема цікава.
Особливо хочу висловити подяку вам за те, що не полінились написати українською мовою, а не язиком країни-агресора чи окупанта.
Пишіть ще.

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

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

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

Передусім завдання рев’юера — подивитись свіжим поглядом на виконану роботу й знайти кращі рішення.

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

І не кажіть. Бо наш код має працювати (на відміну від всього іншого коду на проекті :)

По більшости згоден, і так і робимо, крім:

> Усе, що було змінене, має бути протестоване.

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

> Після написання коментарів зідзвонитися особисто та все проговорити.

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

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

Я б ще додав до розповіді, які типові технічні засоби використовуються для ревʼю. У нас це Gerrit, але навкруги я рідко бачу його згадування (а жаль).

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

Мова про тестування коду локально. CI починається трохи пізніше.

Мова про тестування коду локально.

Звідки ви знаєте, що хотів сказати автор статті? Разом її писали?

CI починається трохи пізніше.

Чому це? У нас поки CI не пройшло — до візуального ревʼю не приступають.

Звідки ви знаєте, що хотів сказати автор статті? Разом її писали?

Один з пунктів DoR в зрілих командах це тестування коду локально. Ніхто не створює Pull/Merge Request з наступним Code Review для нетестованого коду.

Чому це? У нас поки CI не пройшло — до візуального ревʼю не приступають

Ви запускаєте CI для Feature бренчів? Ну круто. А може ведете розробку прямо в Master? :))))

Ви запускаєте CI для Feature бренчів?

Так. А що вас дивує? Не збігаються з релігією? (а як ще це зрозуміти?)
Як на мене, принцип, що кожний запропонований коміт має пройти перевірку у CI, виглядає досить розумно.

А може ведете розробку прямо в Master? :))))

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

Один з пунктів DoR в зрілих командах це тестування коду локально.

А якщо локальне тестування неможливе (повністю або частково)? Чому я не можу прогнати через CI і отримати подробний звіт про всі недоліки?

Ніхто не створює Pull/Merge Request з наступним Code Review для нетестованого коду.

Ну ось створюєм. Звичайно він находиться в work-in-progress поки не буде схвалений CI, тоді автор переводить його в публічний стан.

Ви хоч щось крім соромоскраму (судячи по згадуванню DoR) чули з процесів розробки?

Чудова пропозиція про технічні засоби та дякую за ваш приклад з практики!

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

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

поєдную функції тімліда та Senior Software Engineer.

Зайчик?)

Думаю, более известные организации не знают про эти принципы:
goomics.net/40
goomics.net/67
(или осознанно их игнорируют)

67-й таки происходит регулярно, невозможно проверить совсем всё. Поэтому часто проверяем только наиболее характерные проблемы каждого из авторов, выучив их заранее.

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