Слишком длинные методы класса (по количеству кода)
Признаки: у ряда авторов встречал, что в идеальном случае код метода должен помещаться на один экран, а это около 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.