На работе предложили прочитать доклад. Вуаля! Далее расскажу:
✓ Что такое кодревью?
✓ Зачем нужен?
✓ Что проверяем?
✓ Типовые проблемы & решения
✓ БОНУС!!! Результаты опроса: «Как вы делаете кодревью?»
Презентация доступна по ссылке.
Что такое кодревью?
Кодревью — это, внезапно, ревью кода. Разработчики пишут код, потом делают MR (merge request), другие его проверяют, устраняются замечания и заливают код в develop. Можно в принципе и сразу заливать в develop, но лучше чтобы ваше творчество кто-то перед этим посмотрел.
https://en.wikipedia.org/wiki/Code_review
https://en.wikipedia.org/wiki/Software_review
Зачем нужен кодревью?
Базовые вещи, которые обычно упоминают разработчики:
улучшить качество кода (стиль, понятность, поддержка)
найти ошибки
передать знания
повысить взаимную ответственность
предложить лучшее решение
проверить на соответствие гайдлайнам
Из доступной статистики нагуглил отчет «The State of Code Review 2017» (ссылка под VPN):
Что проверяем на кодревью?
Есть миллион статей по запросу «code review checklist»: 1 2 3 4.
Основной критерий: код должен быть вам понятным. «Пишите код так, как будто поддерживать его будет склонный к насилию психопат, который знает, где вы живёте».
В первую очередь обращайте внимание: небольшие методы с простым кодом; вынос констант, форматирование (длина строки, наименования), комментарии, освобождение ресурсов (using, unsubscribe), модификаторы доступа, проектные гайдлайны.
Круто если вы знаете код и можете подсказать, как его улучшить. Например, человек написал метод, а вы знаете похожий код в другом классе и лучше вынести общий функционал в базовый класс или утилиту.
Есть чеклисты для разных языков, например, ссылка для Питона.
https://en.wikipedia.org/wiki/Coding_best_practices
https://en.wikipedia.org/wiki/List_of_software_development_philosophies
Типовые проблемы & решения
1. Код с душком |
6. SOLID, DRY, KISS, YAGNI, BDUF, APO, Бритва Оккама |
2. Дизайн с душком |
7. Уязвимости в коде |
3. Рефакторинг |
8. Гниение кода |
4. Антипаттерны |
9. Тёмные паттерны |
5. Паттерны (шаблоны) проектирования |
10. Статический анализ кода |
Далее кратко рассмотрим каждый раздел без подробных комментариев. Детальнее см. ссылки в начале раздела.
Код с душком
https://ru.wikipedia.org/wiki/Код_с_запашком
Дублирование кода |
Ленивый класс |
Длинный метод |
Теоретическая общность |
Большой класс |
Временное поле |
Длинный список параметров |
Цепочка вызовов |
Расходящиеся модификации |
Посредник |
Стрельба дробью |
Неуместная близость |
Завистливые функции |
Альтернативные классы с разными интерфейсами |
Группы данных |
Неполнота библиотечного класса |
Одержимость элементарными типами |
Классы данных |
Операторы типа switch |
Отказ от наследства |
Параллельные иерархии наследования |
Комментарии |
2. Дизайн с душком
https://en.wikipedia.org/wiki/Design_smell
Отсутствие абстракции |
Нарушенная модуляризация |
Многогранная абстракция |
Недостаточная модульность |
Дублирующая абстракция |
Круговая зависимость |
Недостаточная инкапсуляция |
Нефакторизованная иерархия |
Неиспользованная инкапсуляция |
Нарушенная иерархия |
3. Рефакторинг
https://ru.wikipedia.org/wiki/Рефакторинг
Изменение сигнатуры метода |
Введение параметра |
Инкапсуляция поля |
Подъём метода |
Выделение класса |
Спуск метода |
Выделение интерфейса |
Переименование метода |
Выделение локальной переменной |
Перемещение метода |
Выделение метода |
Замена условного оператора полиморфизмом |
Генерализация типа |
Замена наследования делегированием |
Встраивание |
Замена кода типа подклассами |
Введение фабрики |
4. Антипаттерны
https://ru.wikipedia.org/wiki/Антипаттерн
разработки (в ООП, при написании кода, методологические, управления конфигурацией, разные) |
при повторном использовании кода |
архитектурные |
при человеко - компьютерном взаимодействии |
организационные |
сервис-ориентированной архитектуры |
среды |
для интеллектуальной информационной системы |
информационной безопасности |
Мои любимые антипаттерны:
Божественный объект (God object): Концентрация слишком большого количества функций в одной части системы (классе).
Ненужная сложность[en] (Accidental complexity): Внесение ненужной сложности в решение.
Преждевременная оптимизация (Premature optimization): Оптимизация на этапе проектирования сегмента кода, приводящая к его усложнению или искажению.
Грех преждевременной оптимизации. Лодочный якорь[en] (Boat anchor)[13]: Сохранение более не используемой части системы.
Волшебные числа (Magic numbers): Использование числовых констант без объяснения их смысла.
Программирование методом копирования-вставки (Copy and paste programming)[13]: Копирование (и лёгкая модификация) существующего кода вместо создания общих решений.
Изобретение колеса/велосипеда (Reinventing the wheel[13]): Создание с нуля вместо использования хорошего готового решения.
Большой комок грязи (Big ball of mud): Система с нераспознаваемой структурой.
Аналитический паралич (Analysis paralysis)[13]: неоправданно большие затраты на анализ и проектирование. Часто приводит к закрытию проекта до начала его реализации.
Раздутый улучшизм (Creeping featurism): добавление новых улучшений в ущерб суммарному качеству системы.
Синдром варёной лягушки (Boiling Frog Syndrome) — постепенные отрицательные изменения замечают, когда уже поздно.
https://blogs.oracle.com/javamagazine/post/five-code-review-antipatterns
5. Паттерны (шаблоны) проектирования
https://en.wikipedia.org/wiki/Software_design_pattern
https://ru.wikipedia.org/wiki/Шаблон_проектирования
Основные: |
Программирования гибких объектов |
Фундаментальные |
Выполнения задач |
Порождающие * |
Архитектуры системы |
Структурные * |
Enterprise |
Поведенческие * |
Проектирования потоковой обработки |
Частные: |
Проектирования распределенных систем |
Параллельного программирования |
Баз Данных |
Генерации объектов |
Три паттерна со звездочкой — это первые группы дизайн паттернов из книги Банды четырех. А далее уже придумали остальные. Как минимум, полезно их знать, т.к. неосознанно разработчики используют их в коде. Или предложить переделать код для соответствия паттерну. Хорошо бы знать эти шаблоны (для собеседований пригодится): Singleton, Observer, Template method, Prototype, Adapter.
6. Принципы SOLID, DRY, KISS, YAGNI, BDUF, APO, Бритва Оккама
Прекрасная статья на хабре о принципах и на вики о принципе Питера (проект слишком сложный для понимания даже его разработчикам).
YAGNI: You Aren’t Gonna Need It / Вам это не понадобится |
SOLID: |
DRY: Don’t Repeat Yourself / Не повторяйтесь |
S) Single-responsibility principle / Принцип единственной ответственности |
KISS: Keep It Simple, Stupid / Будь проще |
O) Open–closed principle / Принцип открытости-закрытости |
BDUF: Big Design Up Front / Глобальное проектирование прежде всего |
L) Liskov substitution principle / Принцип подстановки Лисков |
APO: Avoid Premature Optimization / Избегайте преждевременной оптимизации |
I) Interface segregation principle / Принцип разделения интерфейсов |
Бритва Оккама: не множить сущее без необходимости |
D) Dependency inversion principle / Принцип инверсии зависимостей |
7. Уязвимости в коде
Не менее прекрасная статья об уязвимостях на хабре и базовая статья на вики.
Обычно секьюрити аудит делают профи уже готового продукта, включая исходный код. Но и при кодревью должно насторожить, если передается значение текстового поля со страницы, из него склеивается SQL и напрямую выполняется запрос в БД, а не используется параметризованный запрос.
SQL-инъекции |
Санитайзинг опасного кода в HTML |
Пароли в открытом виде |
Закладки в код |
Незащищенность данных (html код, куки, API) |
Отсутствие или слабое шифрование |
Авторизация и аутентификация |
Манипуляции с логами |
XSS-внедрение вредоносного кода |
Небезопасная работа с куками |
Утечка информации |
8. Гниение кода: причины
https://en.wikipedia.org/wiki/Software_rot
Изменение окружающей среды |
Редко обновляемый код |
Одноразовость |
Интернет-соединение |
Неиспользованный код |
9. Тёмные паттерны
https://ru.wikipedia.org/wiki/Тёмные_паттерны
https://en.wikipedia.org/wiki/Dark_pattern
Это относится скорее к проектированию UX/UI (нельзя обманывать пользователя и выдавать одно за другое). Но если заметите такое в коде, стоит обратить внимание.
Вопросы с подвохом |
Скрытые платежи |
Подбрасывание товара |
Приманка с подменой |
Ловушка |
Упор на чувство вины |
«Сцукербергивание» личных данных |
Замаскированные баннеры |
Приёмы, не дающие адекватно сравнивать цены |
Принуждение к подписке |
Отвлечение внимания |
Спам друзьям |
10. Статический анализ кода
https://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis
Существует много инструментов для анализа кода: [фронт], а вот для [C#]. Как стандарт: Linter для Javascript/Typescript; SonarQube, ReSharper, Visual Studio для C#.
БОНУС!!! Результаты опроса: «Как вы делаете кодревью?»
Опрос доступен по ссылке.
1. ТОП-5 пользы от кодревью? |
4. Сколько времени тратите на кодревью в-среднем за день? |
2. Что вы проверяете в процессе кодревью? |
5. Как вы проводите кодревью (как узнаете, делаете сразу или откладываете на вечер, как взаимодействуете с коммиттером, ваши лайфхаки и подходы). |
3. Сколько MR оцениваете в-среднем за день? |
6. Ваши предложения по улучшению кодревью? |
Результаты опроса
5 из 7: Как вы проводите кодревью (как узнаете, делаете сразу или откладываете на вечер, как взаимодействуете с коммиттером, ваши лайфхаки и подходы).
Зависит от срочности, обычно стараюсь отложить на конец рабочего дня
1. узнаю в ММ канале merge_requests 2. ASAP делаю ревью 3. как сделал ставлю лайк сообщению в ММ канале 4. если получаю от автора замечания или сообщение что все исправил смотрю и если ок ставлю лайк и сообщаю автору (сам не мониторю статус МР что исправлено) 5. если код очень непонятный или из другого проекта то не погружаюсь в процесс и не сильно придираюсь, проверяю формальные признаки - понятный код, стиль 6. проверяю код в гитлабе, не выгружаю себе ветку и не проверяю что работает
День в день. Иначе код без ревью поедет на прод.
as soon as I see it
По обстоятельствам
Когда нет срочных заданий
Стараюсь проводить как можно раньше. Чаще всего пишу комментарии к коду в корпоративном GitLab, но если в коде какие-то совсем мелкие проблемы, то пишу разработчику напрямую.
Обычно провожу не сразу. Взаимодействую с коммитером через комментарии к коду в GitLab
При получении оповещения по почте
Откладываю, делаю когда прижмет (попросят)
Если задача не критичная, откладываю на время, когда можно спокойно погрузиться в контекст. Отвожу часть рабочего дня именно под ревью, делаю несколько сразу. По процессу – у нас на доске есть отдельная колонка для тикетов в ревью, но туда не смотрим, разработчик сам тегает в чате ревьюеров, когда готов. Всё взаимодействие - в МР-е в гитлабе, там пишем комментарии. Мелкие изменения типа орфографии сразу предлагаем через специальный интерфейс. Очень редко можем созвониться, чтобы что-то прояснить и даже устроить парное программирование. В 80% случаев в первую очередь смотрю и "тыкаю руками" дев-стенд, проверяю соответствие описанию тикета, а также места, которые могли быть затронуты изменениями (по сути, провожу небольшое ручное тестирование). Затем пробегаю глазами весь реквест - за грубые косяки вроде неубранных комментариев глаз цепляется сразу. Затем внимательнее просматриваю места с алгоритмами, неочевидной/сложной логикой, основную бизнес-логику.
Фиксы - сразу, фичи - долго (бывает и неделю может висеть)
Сразу. Серьезные проблемы комментирую в гитлабе, мелкие недочеты могу обсудить в личных сообщениях
Ну все как обычно.
В начале и во второй половине дня
Github notifications, usually within 2 days. In most of the cases, i only leave comments on GitHub without any private communication. Just to keep history
Среди опытных разработчиков из смежных команд договорились о незамедлительном проведении ревью открытых MR-ов, если нет ничего более срочного.
Периодически просматриваю назначенные на меня мры. Для незначительных исправлений оставляю коменты. Для крупных/принципиальных вопросов связываюсь с комиттером для обсуждения, если сразу связаться не удаётся оставляю детальный коммент.
Утром смотрю новые PR в VCS
Сразу, по возможности
В начале дня, если есть задачи, то стараюсь посмотреть все. И когда закрыл свою задачу, то смотрю не появились ли ещё какие-нибудь для повторного ревью или новые. Стараюсь смотреть код всегда в такой последовательности - сначала тот код который не имеет зависимостей, а дальше поднимаюсь выше по уровням. Если замечание/пожелание большое, то оставляю комментарий, даже если коллега сидит рядом, а если что-то несущественное и для меня не сразу понятное, то это обсуждается словами, коммент пишется в зависимости от того к чему пришли.
Когда есть передышка, но стараюсь в тот же день. Сначала просто список замечаний (Feedback ladders), если начинается длинная переписка - прошу автора уведомить меня, когда нужно опять читать.
Если небольшой, то сразу. Большие MR откладываю на вечер. Если замечание требует дискуссии, то обычно ставлю замечание и связываюсь с коммитером. Для больших MR часто локально делаю мердж git merge --squash чтобы видеть изменения и удобнее перемещаться по коду в IDE
Если не попросили сделать ревью прямо сейчас, или до какого то времени. То провожу, когда мне удобно.
Простые смотрю в течение дня, сложные откладываю на утро или вечер
Ревью планируется по времени. Вся переписка ведется только в комментариях\тредах в MR, если было какое-то устное обсуждение, то оно обязательно тезисно фиксируется в комментариях в MR. Если в ходе какого-либо обсуждения(треда) в MR было решено оставить в коде TODO, то TODO обязательно содержит в теле ссылку на этот тред. MR обязательное должен содержать описание. MR желательно должен быть разбит логически на патчи по коммитам. Очень большой MR желательно разбить на несколько MR. Обязательно обращать внимание на code coverage для кода в MR.
Всегда по утрам
выделяю время в течении дня. если большой реквест - устраиваем короткий созвон и обсуждаем реализацию
Выделяю часть рабочего времени каждый день.
На собрании в соответствие с указанными сроками. Результаты отображаются в IDE и соответствующих сопроводительных документах.
6 из 7: Ваши предложения по улучшению кодревью?
Проводить качественный селф тест перед отправкой на МР
1. дать на аутсорс кодревью (Code Review as a Service https://www.pullrequest.com/) 2. балансировщик ревью чтобы в среднем все по объёму делали одинаковую работу 3. назначать по матрице кому что ревьюить - люди по отделам, бэкеры бэк, фронты фронт 4. обязательный ревьюер один из проекта и необязательные ревьюеры из других проектов, просто шаринг знаний и экспертизы 5. написать чеклисты для ревью для кода C#, Angular 6. попросить всех делать ревью МР ASAP
Не делать большие PR. Если ревью занимает часы, то лучше просто парно попрограммировать. Не токсичить Сложные задачи и их решения предварительно оговаривать в виде ADR/RFC/Proposal. Фича на проде сегодня важнее красивого кода завтра.
Маленькие MR.
Самому бы получить пару советов :)
Делайте круговое код ревью. Пара глаз хорошо, а несколько - лучше
Чтоб были тесты доказывающие работоспособность, или видео
Чем больше всякого вроде линтеров, статического анализа, тестов, тем больше на ревью удастся уделять внимание бизнес-логике и архитектуре. Еще удобно договориться об общем "языке" и в целом о процессе: например, префикс nitpick делает комментарий необязательным к исполнению; крупные изменения по стилю/рефакторинг можно выносить в отдельный тикет в техдолг, чтобы не задерживать разработку, и т.д.
Стоит проводить их в комфортной обстановке
Зависит от цели код-ревью в конкретной организации. Так что, скорее всего мои советы будут бесполезны.
Маленькие итеративные MR-ы, чем один большой.
Чтобы джуны хотя бы иногда делали ревью коллег, что выше по опыту
Все норм
Хороший style guide, монорепозиторий (позволяет искать и читать много кода разных команд, изучать лучшие практики), наличие тестов
Автоматизация базовый вещей (форматтеры, линтеры, статические анализаторы, тесты, визуализация тестового покрытия и пр), их интеграция в процесс через CI.
Презентация доступна по ссылке. Спасибо за внимание!
Комментарии (4)
vadimkle
13.06.2023 08:20тулзы для стат анализа - это то, чем надо пользоваться в процесса review? Довольно странно, на мой взгляд.
berlicon Автор
13.06.2023 08:20Если большой MR имеет смысл смотреть его не в гитлабе, а залить себе ветку локально, запустить линтер и выгрузить в результаты ревью список предупреждений линтера, связанных с измененным кодом.
YourgenAP
Проблемы увидел. Решения не увидел. Кто поможет?
berlicon Автор
Чеклисты, гайдлайны, тулзы для стат анализа, бот распределяющий MR по разрабам и все перечисленное в статье и в последних 2х пунктах опроса не зашло? ????