Code smells — обзор на примере PHP
Hola, Amigos! Меня зовут Евгений Шмулевский, я PHP-разработчик в агентстве заказной разработки Amiga. В этой статье хотелось бы рассмотреть вопрос качества кода и что из рекомендаций по нему лично для себя использую. Статья адресована начинающим разработчикам.
Почему решил написать статью о code smell?
Когда я начинал изучение веб-разработки в 2011 году, информации было значительно меньше. К тому моменту у меня уже был опыт программирования на других языках: Basic, Turbo Pascal и немного C#. Обучался я по видео-курсам от Евгения Попова (написал это предложение и даже немного поностальгировал по тем временам :) ). Обучение было простым, но, как показало время, многие аспекты о безопасности и масштабировании кода упускались. По итогу курса, я сделал сайт с развивающими играми для детей (сами игры делал на Flash: action script 2,3). Прошло немного времени и злоумышленники обнулили статистику просмотров по всем играм через SQL-инъекцию. После этого пришлось многое чего пересмотреть в коде проекта да и в вопросах безопасности.
Шло время, я писал код, стараясь использовать модное тогда MVC. В 2013 году перешел на Битрикс от самописных решений и только в году 2018 узнал про такое понятие, как «code smells».
«Код с душком» — запутанный, трудный для понимания код. В таком коде изменения в одном участке могут приводить к неожиданным изменениям в других участках кода. Еще используют понятия: спагетти-код, равиоли-код или пицца-код.
Я начал на практике применять рекомендации и в итоге понял, что с такими вещами нужно знакомиться как можно раньше. Лучше сразу после изучения базового синтаксиса, так как это сильно повышает вашу производительность.
В этой статье я решил сделать обзор того, что есть на данную тематику, а также поделиться некоторыми своими соображениями. В конце статьи собрал ресурсы для более детального ознакомления.
Итак, почему же возникает запутанный код, который сложно поддерживать? Частыми причинами выступают: неопытность разработчика или отсутствие времени. При наличии опыта будут появляться архитектурные проблемы.
Признаки «кода с душком» и что с этим делать
Слишком длинные методы класса (по количеству кода)
Признаки: у ряда авторов встречал, что в идеальном случае код метода должен помещаться на один экран, а это около 30 строк кода (хотя экраны бывают разные). Также в названии метода может присутствовать and/or, таким образом, нарушается SRP. Частный случай — «толстый» Controller.
Чем плохо: сложнее тестировать, отлаживать, затруднена навигация по коду.
Что делать: разбиваем большой метод на более мелкие. А если часть кода — ответственность другого класса, то выносим в зависимости.
Большие классы, имеющие множество обязанностей
Признаки: очень большой (понятие относительное) класс, который выполняет множество задач, которые не относятся к нему напрямую. Нужно проверить действительно ли соотносятся название класса и названия его методов.
Чем плохо: тот класс, который наделен множеством обязанностей, более сложен в тестировании (нужно покрыть тестами весь функционал). Большое число методов затрудняет доработку такого класса.
Еще один минус — если нам потребуется переиспользовать методы такого класса, то в коде могут появляться:
- copy-paste куски кода. Как пример есть такой класс который формирует заказ и отправляет уведомления. Вам понадобились уведомления в другом классе. Возникает соблазн просто скопипастить это дело. Но все-таки правильнее выделить функционал уведомлений (да и любой другой не относящийся к данному классу) в отдельный класс и внедрить как зависимость
- нелогичные вызовы методов, когда пытаются вызвать метод этого класса который на прямую к нему не относится в другом контексте.
Что делать: разбить на более мелкие классы и использовать через внедрение зависимостей. Если привести аналогию, то наши классы — это конструктор из разных элементов, и мы можем их использовать, чтобы создавать нужные нам вещи.
Представим это на примере класса BasketService. Предположим, что помимо управления корзиной пользователя, данный класс САМ делает расчеты по скидкам на позиции. Тогда в случае, если нам потребуются эта же логика в классе оформления заказа OrderService, то нам придется использовать BasketService. Но было бы гораздо удобнее, чтобы эти оба класса использовали для расчета скидок DiscountService. Таким образом, всё, что вне зоны ответственности класса, мы делегируем другому классу.
Непонятные наименования переменных и сущностей
Признаки: из названия переменной, сущности, метода не понятно, за что оно отвечает.
Чем плохо: сложнее дорабатывать код, нужно больше вникать в контекст, чтобы определить, для чего данная сущность (переменная) необходима.
Что делать: сразу оговорюсь, что, на мой взгляд, не существует идеальных правил наименования и некоторые вещи субъективны, но тем не менее можно выделить общие рекомендации:
- давать наименование переменной/сущности достаточное для понимания назначения вне контекста. Например, $subscriptionsCount — более понятное, нежели $sc или $scount.
- не использовать в названии тип переменной. Например, $arResult (привет, Битрикс).
- стараться использовать общепринятые наименования (с годами этот перечень нарабатывается у каждого программиста), переведенные на англ. (count, amount, order, price и т.д.).
- не использовать транслитерацию (например, $zakaz).
Если ваша переменная имеет наименование $c, то совсем не понятно, за что она отвечает, и есть необходимость глубокого погружения в контекст. Если мы назовем ее, например $count, то уже можно предугадать, что это какой-либо счетчик. Лучшим вариантом, на мой взгляд, было бы более подробное наименование $subscriptionsCount. Здесь мы понимаем, что переменная отвечает за количество подписок, даже не погружаясь в окружающий контекст.
Дублирование кода
Признаки: одни и те же куски кода, выполняющие один и тот же функционал, но в разных методах и классах.
Чем плохо: при необходимости изменений в коде, нам придется их вносить во все дубли. Из-за этого усложняется доработка, и возникает риск ошибок.
Что делать: нужно убедиться, что это действительно copy-paste, а не похожий функционал. Иначе выносить всё это дело в отдельный класс/метод будет не лучшим решением. Если это дубляж в чистом виде, то выносим эту логику в отдельный класс/метод. В случае класса внедряем, как зависимость.
Много входящих параметров в методе
Признаки: методы с множеством входящих параметров. Для себя, обычно, выделяю не более трех.
Чем плохо: при вызове метода нужно передать все параметры в таком же соответствии. Также, на мой взгляд, ухудшается читаемость кода.
Что делать: если метод делает слишком много, тогда следует разбить его на более мелкие методы. Если не получается, то можно передавать параметры через объект DTO (data transfer object), основной задачей которого является передача данных.
Использование флага в методах
Признаки: в методе используется булев флаг (например, $isNotify).
Чем плохо: наличие флага говорит о том, что метод делает не только, что от него ожидается, тем самым нарушая принцип единственной ответственности. Также в методе появляются избыточные ветвления, доп. проверки что может вести к ошибкам. Где-то есть флаг, где-то нет, а где-то забыли передать.
Что делать: по возможности, разбить метод на более мелкие методы.
Null return
Признаки: в методе помимо основного типа (array или класса) возвращается тип null.
Чем плохо: если при дальнейшей обработке идет, например, итерация или вызов метода, то null создает определенные проблемы.
Что делать: возвращать пустой объект (NullObject) или пустой массив, если ожидался массив. Например, в Laravel лучше возвращать, если ничего не было найдено, пустую коллекцию нежели null.
Создание объектов в методах
Признаки: в методах используется new(). Исключение тут составляют различного рода фабрики.
Чем плохо: жесткая привязка к какому-либо классу. Недавно, кстати, столкнулся с такой проблемой при доработке админки на Orchid, и нужно было изменить логику работы фильтра (поиск без учета регистра). При построении фильтра использовался объект HttpFilter, и он передавался как параметр метода. Поэтому выполнив наследование от этого класса, я смог переопределить логику этого фильтра. Если бы метод не получал объект в качестве зависимости, то изменить логику фильтра не получилось бы.
Что делать: необходимые классы использовать как зависимости через параметры.
Вложенный if (и в целом использование else)
Признаки: множество вложенных if…else. Бывает встречаешь такие нагромождения, что уже и разобрать изначальную логику трудно. Это очень сильно увеличивает риск допустить ошибку в коде, замедляет отладку, ведь в голове нужно просчитывать возможные варианты ответвлений и их комбинаций. Да и тестировать такие вещи — тоже не самое приятное занятие.
Чем плохо: затрудняет отладку кода, тестирование.
Что делать: в первую очередь, можно отказаться от else. Для этого можно использовать подход early return. Т.е. мы досрочно завершаем работу метода, если условие не выполнилось. Если условие используется в цикле, то можно использовать continue. Также можно подумать о разбиении этого куска кода на более мелкие.
Основная идея — отсекать, как можно раньше. отрицательные случаи. Т.е. проверили, и если случай негативный, то либо return, либо если в цикле continue.
Помимо возврата, можно также бросать исключения, что, конечно, зависит от решаемой задачи. Но основная идея в том, чтобы валидировать негативные случаи вначале метода.
Использование примитивов взамен полноценных объектов (Primitive Obsession)
Признаки: для сложных данных используется скалярные типы. Например, телефон, адрес и т.д.
Чем плохо: критичного здесь ничего нет, но использовав для таких данных свои классы, мы можем упростить поддержку кода.
Что делать: завести отдельный класс для таких данных. Также такой класс будет сам себя валидировать, что упрощает его переиспользование.
Пример полноценного объекта для типа телефон. При создании объекта валидируется и приводится к нужному виду. Если объект сложнее, например, адрес, то он может содержать несколько полей: $country, $region, $city, $street.
Магические числа
Признаки: видим в коде присутствуют какие-либо числа непонятного, на первый взгляд, назначения. Могут дублироваться в разных местах.
Чем плохо: при изменение бизнес-логики необходимо найти в коде всё это дело и изменить, что может повлечь за собой ошибки.
Что делать: вынести в константы, либо конфиги данные числа. Далее при изменении бизнес-логики можно будет менять в одном месте централизованно, чтение также кода улучшится.
Избыточные комментарии
Признаки: комментируется то, что само по себе очевидно.
Чем плохо: ухудшает читабельность кода.
Что делать: комментировать сложные участки кода, называть методы и классы, чтобы они максимально емко отражали сущность производимого действия.
Процедурный стиль в ООП
Признаки: 7 лет назад вел разработку на Битрикс, и у нас в команде был популярен подход, когда делался класс типа Helper и множество статичных методов. Далее это вызывалось в коде по мере необходимости. Из плюсов — быстрота, а минусов здесь гораздо больше: получается супер-класс, который сложно поддается поддержке.
Чем плохо: в некоторых случаях возможно ничего критического нет, но применяя данный подход, мы ограничено используем преимущества ООП.
Что делать: заранее продумать структуру классов, разделить ответственность между ними.
Глобальные переменные (Global Data)
Признаки: использование зарезервированного слова global и обращение к глобальным переменным.
Чем плохо: повышает запутанность кода, затрудняет отладку, т.к. может быть не совсем понятно, где была изменена глобальная переменная.
Что делать: отказаться от использования глобальных переменных. Использовать, например, DTO для передачи данных между слоями приложения.
Dead Code
Признаки: куски закомментированного кода, которые не используются в приложении.
Чем плохо: затрудняют чтение кода.
Что делать: убирать. Если код понадобится в будущем, у нас есть git.
Combinatorial Explosion
Признаки: В дословном переводе «комбинаторный взрыв». Т.е. в коде есть ряд методов, которые имеют разные параметры, но делают почти одни и те же вещи.
Чем плохо: возникают сложности с поддержкой такого кода, т.к. при изменении логики нужно будет вносить изменения в каждый из методов.
Что делать: пересмотреть методы. Выделить, есть ли действительно ключевые отличия, и если нет, то заменяем лишние методы одним методом.
Refused Bequest
Признаки: класс не использует большую часть из унаследованного функционала.
Чем плохо: вносит запутанность в понимание кода.
Что делать: пересмотреть, действительно ли класс должен быть унаследован от данного родителя. Попробовать выделить общий функционал и вынести в отдельный класс, внедрив как зависимость.
В заключении стоит подчеркнуть, что все вышеперечисленное является рекомендацией, и применение может зависеть от ситуации.
В целом, избегание вышеперечисленного помогает в повышении производительности: код проще читать и дорабатывать. Меньше будете на работе расходовать мыслетоплива (термин встретил при чтении книги Максима Дорофеева «Джедайские техники. Как воспитать свою обезьяну, опустошить инбокс и сберечь мыслетопливо»), да и если коллеги не скажут Вам «спасибо», то хотя бы не будут вспоминать вас недобрым словом :)
P.S.: ниже список, что еще почитать по теме:
https://refactoring.guru/ru/refactoring/smells - отличный курс по рефакторингу.
https://github.com/piotrplenik/clean-code-php - отличное описание принципов чистого кода с примерами на PHP.
https://pragmaticways.com/31-code-smells-you-must-know/ - отличная статья по запахам кода в целом.
https://github.com/bmitch/Codor - в описании к пакету можно посмотреть на примеры bad/good практик кода.
https://phptherightway.com/ - здесь описывается правильный подход.