Как проводить кодревью?

На работе предложили прочитать доклад по теме разработки. Вуаля! Далее расскажу:

Что такое кодревью?
Зачем нужен?
Что проверяем?
Типовые проблемы & решения
БОНУС!!! Результаты опроса: «Как вы делаете кодревью?»

Презентация доступна по ссылке.

Что такое кодревью?

Кодревью — это, внезапно, ревью кода. Разработчики пишут код, потом делают 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-внедрение вредоносного кода (Javascript, cmd, SQL)	Небезопасная работа с куками
Утечка информации

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 из 7: ТОП-5 пользы от кодревью?	5 из 7: Как вы проводите кодревью (как узнаете, делаете сразу или откладываете на вечер, как взаимодействуете с коммиттером, ваши лайфхаки и подходы).
2 из 7: Что вы проверяете в процессе кодревью?	6 из 7: Ваши предложения по улучшению кодревью?
3 из 7: Сколько MR оцениваете в-среднем за день?	7 из 7: Оставьте любую информацию (ваше имя, контакты и др) — СКРЫТО
4 из 7: Сколько времени тратите на кодревью в-среднем за день?