Code Review Rules

CodeReview.md

Я когда делаю Code Review критерии следующие:

• Безопасность:

  • Каждый аргумент метода простого типа должен проверяться на тип в случае его проксирования и на граничные значения в случае обработки. Чуть что не так - бросается исключение. Если метод с кучкой аргументов на 80% состоит из поверки из аргументов - это вполне норм))
  • Никаких trigger_error, только исключения.
  • Исключения ДОЛЖНЫ быть человеко-понятны, всякие "Something went wrong" можно отдавать пользователю, но в лог должно попасть исключение со стектрейсом и человеко-понятным описанием, что же там пошло не так.
  • Каждый аргумент (объект) метода должен быть с тайпхинтингом на этот его класс, или интерфейс.
  • За eval как правило шлю на **й.
  • @ допускается только в безвыходных ситуациях, например проверка json_last_error.
  • Перед работой с БД - обязательная проверка данных.
  • Никаких == и !=. Со swtich - единственное исключение, по ситуации.
  • Если метод возвращает не только bool, а еще что-то - жесткая проверка с ===, или !== обязательна.
  • Никаких условий с присваиваниями внутри. while($row = ...) - тоже идет лесом.
  • Магические геттеры/сеттеры разрешаются только в безвыходных ситуациях, в остальном - запрещены.
  • Конкатенации в sql - только в безвыходных ситуациях.
  • Параметры в sql - ТОЛЬКО через плейсхолдеры.
  • Никаких глобальных переменных.
  • Даты в виде строки разрешаются только в шаблонах и в БД, в пхп коде сразу преобразуется в \DateTimeImmutable (в безвыходных ситуациях разрешено \DateTime)
  • Конечно зависит от проекта, но как приавло должно быть всего две точки входа: index.php для web и console(или как-то по другому назваться) - для консоли.

• Кодстайл PSR-2 + PSR-5 как минимум, + еще куча более жестких требований (для начала все то что в PSR помечено как SHOULD - становится MUST)

  • В PhpStorm ни одна строчка не должна подсвечиваться (исключением является typo ошибки, например словарик не знает какой-то из аббревиатур, принятых в вашем проекте). При этом разрешается использовать /** @noinspection *** */ для безвыходных ситуаций.
  • Если кто-то говорит, что пишет в другом редакторе и у него не подсвечивается, на эти отговорки кладется ВОТ ТАКЕЕЕНЫЙ мужской половой **й и отправляется на доработку)).

• Организация кода:

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

• Тестируемость (в смысле простота тестирования) кода должна быть высокая.

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

• Принципы MVC:

  • Никаких обработок пользовательского ввода в моделях, от слова совсем.
  • Никаких ***ть запросов в БД из шаблонов.
  • Никаких верстки/js/css/sql-ин в контроллерах.
  • В моделях НИКАКОЙ МАГИИ, только приватные свойства + геттеры с сеттерами.
  • В моделях разрешено использовать метод save(при наличии такого разумеется) только в исключительных ситуациях. Во всех остальных - либо insert, либо update.

• Принципы SOLID:

  • Никаких божественных объектов умеющих во все.
  • Если метод для внутреннего пользования - private, никаких public.
  • Статические методы разрешаются только в случае безвыходности.

• Принцип DRY разрешено нарушать в случаях:

  • Явного разделения обязанностей
  • В тестах (каждый тест должен быть независимым, на сколько это возможно)

• Работа с БД:

  • Запрос в цикле должен быть РЕАЛЬНО обоснован.
  • За ORDER BY RAND() - шлю на***й.
  • Поиск не по ключам (конечно если таблица НЕ на 5 строк) запрещен.
  • Поиск без LIMIT (опять же если таблица НЕ на 5 строк) запрещен.
  • SELECT * - запрещен.
  • Денормализация БД должна быть обоснована.
  • MyISAM не используется (так уж)) )
  • Множественные операции обязательно в транзакции, с откатом если чо пошло не так.
  • БД не должна содержать бизнес логики, только данные в целостном виде.
  • Не должно быть нецелесообразного дерганья БД там, где без этого можно обойтись.

• Кэш должен очищаться по двум условиям (не по одному из, а именно по двум):

  • Время.
  • Протухание по бизнес логике. Разрешается по только времени в безвыходных ситуациях, но тогда время - короткий период.
  • При расчете ключей кэша должна использоваться переменная из конфигурации приложения (на случай обновлений кэш сбрасывается кодом, а не флашем кэш-сервера). В случае использования множества серверов - это очень удобный и гибкий инструмент при диплое.

• О людях:

  • "Я привык писать так и буду дальше" - не вопрос, ревью пройдешь только когда поменяешь свое мнение.
  • "Я пишу в vim-е и мне так удобно" - здорово, код консолью я тоже в нем пишу)) но есть требования к коду, если в них не сможешь - не пройдешь ревью.
  • "Я скопировал этот страшный метод и поменял 2 строчки" - это конечно замечательно, но по блейму автор всего этого метода ты, так что давай без говняшек, хорошо?
  • "Оно же работает!" - вот эта фраза переводится примерно так: "да, я понимаю, что пишу полную хрень, но не могу писать нормально потому, что руки из жо", я правильно тебя понял?))
  • "У меня все работает!" - рад за тебя, а как на счет продакшна?
  • "Там все просто" - не используй слово "просто", от слова "совсем". Вот тебе кусок кода (первого попавшегося с сложной бизнес логикой), где там ошибка (не важно есть она, или нет)? Ты смотришь его уже 2 минуты, в чем проблема, там же все "просто"))

• Всякое:

  • ActiveRecord (это я вам как в прошлом фанат Yii говорю) - полное говно, примите за исходную. По факту у вас бесконтрольно по проекту гуляют модельки с подключением к БД. Не раз натыкался на то, что в тех же шаблонах вызывают save, или update (за такое надо сжигать).
  • То, что используется Laravel - это печально((. Что бы выполнить требования приведенные выше, приходится "воевать" с фреймворком.

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

(c) index0h