Как пройти CodeReview?
Ответ
На основе https://toster.ru/q/276441 Понятное дело, многое зависит от проекта, поэтому данный пост стоит адаптировать под ваш случай.
index0h:
* Безопасность:
– Каждый аргумент метода простого типа должен проверяться на тип в случае его проксирования и на граничные значения в случае обработки. Чуть что не так – бросается исключение. Если метод с несколькими аргументами на 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 (за такое надо сжигать).
edli007:
Основное:
1. Наличие критических ошибок и устаревших функций.
2. Использование паттернов, элегантность решений.
3. Читабельность кода, наличие коментариев, наличие доков.
4. Соблюдение парадигм и соглашений ( например, нарушение MVC).
Второстепенно\непринципиально:
1. Быстродействие кода (за исключением хайлоад)
2. Потребление памяти (за исключением бигдаты)
3. Эфективность SQL запросов (за исключением совсем уж несуразных)
4. Избегание в данных момент неважных, но потенциально узких мест (например замедление работы файловой системы при большом количестве картинок в папке аплоада)
5. Новизна примененых технологий.
6. Оправданое\Неоправднанное\Избыточное Велосипедирование.
kalyabus:
- Код не содержит явных и потенциальных ошибок.
- Код работает так, как это описано в документации, техническом задании или сопроводительных комментариях.
- Стиль кодирования соответствует принятым правилам кодирования
- Код имеет сопроводительные комментарии в соответствии с phpDoc
- Вложенность блоков не превышает 4-го уровня.
- Код не генерирует сообщения уровня Strict, Warning, Notice, Deprecated. Если этого невозможно избежать, то непосредственно перед строкой, которая это генерирует необходимо принудительно отключить error_reporting, а непосредственно после строки включить error_reporting в исходное значение (которое было до этого). Такой код должен быть задокументирован специальным образом.
- Закомментированный кусок кода должен быть удален.
- В PHP коде (за исключением phpTemplate) запрещены вставки HTML, JavaScript. Все вставки должны производиться через специальные шаблоны.
- Классы, функции, переменные и константы должны логически именоваться человекопонятным способом на английском языке в соответствии со стандартами кодирования. Не допускается именование транслитом на русском, либо на иных языках
- Область видимости переменных и методов классов всегда должна быть определена (private, protected, public).
- Размер одного метода не должен превышать 40-50 строк.
- Переменная, используемая в цикле, либо в условном блоке должна быть инициализирована заранее.
- Переменная в любой момент времени должна содержать только один тип. Пустая переменная должна содержать null. (не допускается $var = false; $var = ‘test’; . Допускается $var = null; $var = ‘test’;).
- При передаче объектов классов в методы должен использоваться контроль типов.