xoma2
@xoma2
Программист

Попросили проверить код, на что смотреть нужно?

Попросили проверить хороший код или нет. Я не понимаю на самом деле как это сделать.
Есть ли какие то критерии по проверке кода?

Где их можно посмотреть?
Fraemwork Laravel5
  • Вопрос задан
  • 31728 просмотров
Решения вопроса 2
index0h
@index0h
PHP, Golang. https://github.com/index0h
Смотря зачем)). Я когда делаю 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.

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

* Принцип DRY разрешено нарушать в случаях:
- Явного разделения обязанностей
- В тестах (каждый тест должен быть независимым, на сколько это возможно)

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

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

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

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

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

UPD

Формализировал данные критерии по ссылочке: https://github.com/index0h/php-conventions
Ответ написан
edli007
@edli007
full stack, team lead
Основное:
1. Наличие критических ошибок и устаревших функций.
2. Использование паттернов, элегантность решений.
3. Читабельность кода, наличие коментариев, наличие доков.
4. Соблюдение парадигм и соглашений ( например, нарушение MVC).

Второстепенно\непринцыпиально:
1. Быстродействие кода (за исключением хайлоад)
2. Потребление памяти (за исключением бигдаты)
3. Эфективность SQL запросов (за исключением совсем уж несуразных)
4. Избегание в данных момент неважных, но потенциально узких мест (например замедление работы файловой системы при большом количестве картинок в папке аплоада)
5. Новизна примененых технологий.
6. Оправданое\Неоправднанное\Избыточное Велосипедирование.

Мб еще вспомню.
Ответ написан
Пригласить эксперта
Ответы на вопрос 5
apavlyut
@apavlyut
www.pavlyut.ru
Все комментаторы совершили одни и те же ошибки управления потому что, при всем уважении, скорее всего за эти ошибки (в стратегировании) они не платят из своего кармана.

На пальцах отвечаю на ваш вопрос:

1) По структуре - при проверки качества кода / решения / задачи / продукта / настройки сервера и так далее нужно проходить по списку (чеклист) критериев контроля качества - обычно они выглядят как списки определенных параметров которые может замерить третье лицо или сама система - формат проверяемого параметра прямо вот соответсвует / не соответсвует. На сколько процентов пройден чеклист - на столько процентов результат "качественный"
2) Почему ребята ошиблись - потому что стали приводить конкретные списки. Дело в том что у каждого проекта / сиутации / команды / набора компетенций - свои наборы таких чеклистов на разные ситуации. В больших командах сущесвтует основной чеклист который регламентирует CodeReview - и за него отвечает как правило тим лид - он его обновляет, развивает, обосновывает внесенные правила и следит за тем чтобы ПЕРЕД началом разработки все разработчики были ЗАРАНЕЕ ОЗНАКОМЛЕНЫ с этим порятком проверки качества, а все потому что:
3) Количество стайлгайдов и критериев в приципе существует огромное количество - и то как каждому в одной части света / компании удобно делать одно дело - не регламентирует ни разу что именно так же другому человеку в другой ситуации применять эти правила к своему контексту. В виде открытых стайлгайдов они существуют для накопления практик и навыков в первую очередь для их же развития (процесс формулировки наводит порядок в голове) а также дают возможность "на них конкретно" нанизать точечные ответы огромного сообщества людей, и получить те самые разные взгляды на ситуации, и по возможности опять же привести к общему знаменателю. Но это все мелочи жизни, а в вашем случае вы совершите серьезную ошибку если прямо сейчас возьметесь (примите на себя ответственность) проверять чужой код на предмет оценки, потому что:
4) Вас явно используют как внешнего эксперта на которого можно сослаться, от которого можно получить якобы аргументацию для давления на свою позицию при решении какой-то возникшей ситуации во взаимоотношениях клиент-разработчик на проекте куда вас приглашают за экспертизой.
Если вы, не предупредив, о том что "качество кода" начинается с декларации этого качества (в случае если речь идет о проверке этого внутреннего качества в рамках сотрудничества, а не самих задач которые поставлены перед создаваемой системой - фичесов) - любая ваша оценка будет недостоверна контексту ее применения (вы напишете про строки или еще что-то - а у человека будут либо взыскивать деньги / либо недоплатят за работу / или инкапсулируют в договоренности пост фактум за те же деньги работу над соотвествием определенным стилям - это все работа которая должна быть оплачена). Поэтому вот вам вилка ваших дейсвтий:

1) Если у вас просто просят менторства молодые коллеги - дайте им ссылку на гугл и ключевое словосочетание php style guide github
2) Если вас спрашивают (либо вы сами являетесь таким заказчиком который ищет за что зацепиться в коде чтобы продавить свою позицию) - нет критериев качества кода ДО начала работ подписанных на бумаге / пересланных по почте - никакие критерии не могут быть применены к текущим отношениям - только к следующей итерации за следующие деньги.
3) Если вы все же разработчик и вас попросили оценить код - донесите данную ситуацию до стадии корректного закрытия текущего этапа работ - но дальше предложите уже введение стайл гайда если оно того требует. Я полагаю что на самом деле нет. Дав сейчас ответ на вопрос в виде оценки качества кода вы сделаете только одно - абсолюно необоснованно дадите агрумент в явно перекошенном споре, и просто возьмете на себя еще один мешок кармогрязи которую будуете еще сколько-то положенного времени отрабатывать.

Подумайте хорошо на эту тему - придется выбрать свою сторону.
Ответ написан
Комментировать
sivabur
@sivabur
Заблокировали просто так!
Laravel использует PSR-2 !
Ответ написан
@kalyabus
Сначала поймите зачем нужно "проверить" код, потом поймете, "что" проверять. Тем не менее приведу выжимку из критериев, принятых у нас

  1. Код не содержит явных и потенциальных ошибок.
  2. Код работает так, как это описано в документации, техническом задании или сопроводительных комментариях.
  3. Стиль кодирования соответствует принятым правилам кодирования
  4. Код имеет сопроводительные комментарии в соответствии с phpDoc
  5. Вложенность блоков не превышает 4-го уровня.
  6. Код не генерирует сообщения уровня Strict, Warning, Notice, Deprecated. Если этого невозможно избежать, то непосредственно перед строкой, которая это генерирует необходимо принудительно отключить error_reporting, а непосредственно после строки включить error_reporting в исходное значение (которое было до этого). Такой код должен быть задокументирован специальным образом.
  7. Закомментированный кусок кода должен быть удален.
  8. В PHP коде (за исключением phpTemplate) запрещены вставки HTML, JavaScript. Все вставки должны производиться через специальные шаблоны.
  9. Классы, функции, переменные и константы должны логически именоваться человекопонятным способом на английском языке в соответствии со стандартами кодирования. Не допускается именование транслитом на русском, либо на иных языках
  10. Область видимости переменных и методов классов всегда должна быть определена (private, protected, public).
  11. Размер одного метода не должен превышать 40-50 строк.
  12. Переменная, используемая в цикле, либо в условном блоке должна быть инициализирована заранее.
  13. Переменная в любой момент времени должна содержать только один тип. Пустая переменная должна содержать null. (не допускается $var = false; $var = 'test'; . Допускается $var = null; $var = 'test';).
  14. При передаче объектов классов в методы должен использоваться контроль типов.
  15. etc...


Список далеко не полный. Все зависит от проекта, выполняемой задачи, платформы и компетенции команды. Зачастую просят проверить код перед приемкой на сопровождение. Декларируйте какой-то свой свод правил кодирования и отталкивайтесь от него.

PHP, к сожалению (а может и к счастью), не строгий язык...
Ответ написан
Комментировать
На код!)
сори не удержался!

Ну я обычно смотрю:
1. на чистоту функций
2. на перегруженность функции
3. обычно тот кто делает ревью, знаете проект, смотреть на дублирование, возможно это уже есть в проекте, но программист этого не увидел и написал свое решение.
4. на кодестайл, он должен быть единый для всех. название переменных, функций, классов... они должны нести смысл.
5. принты которые забыли выпилить при дебаге, но обычно для этого есть инструменты.
6. еще много зависит от проекта, например если у вас 5к в минуту, вы не можете делать update view + 1, это сильно вас будет тормозить. если не нагнет всю систему.

ну наверное еще многое, всего не вспомнить сразу, но обычно это видно в коде.
Ответ написан
Если вы не знаете, то ничто не поможет.

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

На уровне настройки приложения всё не так просто. Здесь большой опыт нужен.
На уровне разработки - нужно поучаствовать в нескольких проектах с несколькими разработчиками и несколькими ушедшими. Тогда острые углы сразу начнут вырисовываться.

Ну и читать классиков, например на предмет "запаха кода", поддержки чужого кода и хорошего кода.

Если код читаемый - то его легко поправить даже при наличии грубых ошибок. Именно правка кода - основа разработки. Отсутствие знаний и опыта приводит к плохом коду. Отсутствие дисциплины написания кода приводит к бесполезному коду. Плохой код можно улучшить. Бесполезный - выкинуть (переписать).

Единственное замечание - если встречаешься с ужасным кодом, то нужно принимать его как есть. Эмоции не помогут. Нет сил - не берись. Есть силы - степ бай степ пока... )
Ответ написан
Комментировать
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Войти через центр авторизации
Похожие вопросы