DSE Fest — технично и понятно про data science для разработчиков. Первые доклады уже на сайте >>
×Закрыть

Рефакторинг: основные принципы и правила

Привет, DOU. Меня зовут Андрей Данильченко, я PHP-разработчик в Wikr Group. В этой статье расскажу о рефакторинге.

Как бы не хотелось, не всегда удается сразу писать код хорошего качества. Причинами могут быть нехватка знаний программиста или недостаток времени. К тому же иногда при выполнении задачи изменяются требования — и это тоже не лучшим образом отражается на качестве кода. Поэтому рефакторинг становится неотъемлемой частью процесса разработки. Мы выделяем на него, как правило, одну неделю раз в полтора месяца.

Когда нужен рефакторинг

Согласно «Википедии», рефакторинг — это процесс изменения внутренней структуры программы, не затрагивающий её внешнего поведения. Его цель — упростить понимание работы программы.

Итак, что значит «упростить понимание работы программы»? Конкретные цели рефакторинга могут быть такими:

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

Также рефакторинг помогает быстрее реализовать программные продукты. Повышается качество — и, соответственно, скорость разработки. Рефакторинг точно необходим, если к вам в команду приходит новый человек, и код в таком виде, в котором он существует, ему не понятен. Это говорит о том, что качество кода неудовлетворительно.

Для рефакторинга, во-первых, напишите хорошие тесты: unit, функциональные или интеграционные. Во-вторых, изменяйте код небольшими итерациями. На каждом шаге прогоняйте тесты. Для качественного рефакторинга полезно знать шаблоны проектирования. Без них будет сложнее проектировать и масштабировать большие проекты.

Что именно рефакторить

Рассмотрим, какие элементы кода затрудняют его восприятие, ухудшают качество и, соответственно, требуют рефакторинга.

Повторы

Допустим, у нас есть такой фрагмент:

$dto
    ->setId($data['id'])
    ->setTitle($data['title'])
    …
    ->setCreatedAt($data['createdAt']);

Решение — реализовать гидратор:

$hydrator->hydrate($data, $dto);

Метод гидратора:

public function hydrate(array $data, $object) : void
{
   foreach($data as $property => $value) {
       $setterName = 'set' . $property;
 
       if (method_exists($object, $setterName)) {
           $object->{$setterName}($value);
       }
   }
}

Комментарии

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

Пример:

$data = $this->getData($cursor);

// put data to csv
foreach ($data as $row) {
	fputcsv($file, $row);
}

Решение — переписать код, заменив комментарии вынесением кода в методы. Даже несколько строк кода лучше вынести в метод, чтобы не использовать комментарий:

$data = $this->getData($cursor);
$this->putDataToCsv($file, $data);

Условные выражения

Условные выражения запутывают. Конечно, по своей сути if, else, elseif, switch не плохи. Они становятся плохими, когда делают проект менее гибким. Чтобы избежать нагроможденности, стоит заменять условные выражения стратегией и/или спецификациями.

Пример:

class ErrorResponsePart implements ResponsePartInterface
{
   /**
    * Error response part data.
    *
    * @var ResponseDtoInterface $partData
    */
   private $partData;
 
   /**
    * {@inheritdoc}
    */
   public function addData(ResponseDtoInterface $data) : void
   {
       if ($data instanceof ErrorDtoInterface) {
           $this->partData = $this->format($data);
       }
   }
 
   /**
    * Prepares response data.
    *
    * @param ResponseDtoInterface $data
    */
   public function format(ResponseDtoInterface $data) : void
   {
        if ($data instanceof ListDataDtoInterface) {
            $formatted = $data->getListData();
        } elseif (/* some expressions */) {
            // some logic
        } elseif (/* some expressions */) {
            // some logic
        }
 
       return $formatted ?? $data;
   }
}

Решение — вынести if из метода addData в спецификацию. А метод format — в отдельный класс, применив стратегию:

class ErrorResponsePart implements ResponsePartInterface
{
   /**
    * Specification.
    *
    * @var SpecificationInterface $specification
    */
   private $specification;
 
   /**
    * Response part data.
    *
    * @var ResponseDtoInterface $partData
    */
   private $partData;
 
   /**
    * Formatter context.
    *
    * @var FormatterContext $formatterContext
    */
   private $formatterContext;
 
   /**
    * ErrorResponsePart constructor.
    *
    * @param SpecificationInterface $specification
    * @param FormatterContext $formatterContext
    */
   public function __construct(SpecificationInterface $specification, FormatterContext $formatterContext)
   {
       $this->specification = $specification;
       $this->formatterContext = $formatterContext;
   }
 
   /**
    * {@inheritdoc}
    */
   public function addData(ResponseDtoInterface $data) : void
   {
       if ($this->specification->isSatisfiedBy($data)) {
           $this->partData = $this->formatterContext->process($data);
       }
   }
}

Спецификация c бизнес-правилами:

class ErrorSpecification implements SpecificationInterface
{
   /**
    * {@inheritdoc}
    */
   public function isSatisfiedBy(ResponseDtoInterface $object): bool
   {
       return $object instanceof ErrorDtoInterface;
   }
}

С помощью контекст-стратегии можно передать несколько стратегий и тем самым уйти от множества if-ов:

class FormatterContext
{
   /**
    * Formatters.
    *
    * @var FormatterInterface[]
    */
   private $formatters;
 
   /**
    * FormatterContext constructor.
    *
    * @param FormatterInterface[] $formatters
    */
   public function __construct(array $formatters = [])
   {
       $this->formatters = $formatters;
   }
 
   /**
    * Format response data part.
    *
    * @param ResponseDtoInterface $data
    * @return mixed
    */
   public function process(ResponseDtoInterface $data)
   {
       foreach ($this->formatters as $formatter) {
           $data = $formatter->format($data);
       }
 
       return $data;
   }
}

Конкретная стратегия:

class ListDataFormatter implements FormatterInterface
{
   /**
    * Checks object for needed requirements.
    *
    * @var FormatterSpecificationInterface $specification
    */
   private $specification;
 
   /**
    * ListDataFormatter constructor.
    *
    * @param FormatterSpecificationInterface $specification
    */
   public function __construct(FormatterSpecificationInterface $specification)
   {
       $this->specification = $specification;
   }
 
  /**
   * {@inheritdoc}
   */
  public function format(ResponseDtoInterface $data)
   {
       if ($this->specification->isSatisfiedBy($data)) {
           $data = $data->getData();
       }
 
       return $data;
   }
}

Спецификация для форматера:

class ListDataSpecification implements FormatterSpecificationInterface
{
   /**
    * {@inheritdoc}
    */
   public function isSatisfiedBy(ResponseDtoInterface $object) : bool
   {
       return $object instanceof ListDataDtoInterface;
   }
}

Непонятные имена

Важно использовать такие имена переменных, методов, классов, которые будут ясно сообщать о том, что именно делает код.

Пример:

$fl = count($data) >= ExportBag::LIMIT_PER_FILE;

Заменяем на:

$isExceededLimitPerFile = count($data) >= ExportBag::LIMIT_PER_FILE;

Большие методы, классы

Слишком объемные структуры смотрятся громоздко и затрудняют понимание. Лучше выносить код в небольшие методы или классы. У себя мы приняли, что оптимальные для прочтения методы — это такие, которые имеют длину не более 10 строк.

Длинный список параметров

Избегайте большого списка аргументов в методах, конструкторах. Мы стараемся использовать до 5 аргументов в конструкторе.

Наследование

Предпочтительнее использовать композицию вместо наследования. К примеру, 2 дочерних класса наследуют от родительского все его методы. Если мы добавим в родительский класс метод, который нужен только для одного из дочерних классов, он автоматически будет применим и ко второму. Если же использовать инжект, дочерние классы будут независимы и не будут содержать лишнего. Конечно, все зависит от ситуации — иногда без наследования не обойтись.

Цепочки сообщений: несоблюдение закона Деметры

Закон Деметры говорит, что любой метод любого объекта может вызывать методы только из:

  • своего объекта;
  • переданных ему параметров;
  • любого созданного им объекта;
  • автономных объектов, к которым у него есть прямой доступ.

Программный модуль должен взаимодействовать только с известными ему модулями-«друзьями» и не взаимодействовать с «незнакомцами». При этом мы получаем меньшую связность кода и не знаем о структуре «незнакомцев».

Пример:

$postService->getTemplateResolver()->getName();

Заменяем на:

$postService->getTemplateName();

Статика

Использование статики ведет к непредсказуемости кода. Статические переменные несут глобальное состояние, данные не инкапсулированы в объекты. Изменяя эти переменные из разных мест приложения, мы не можем гарантировать корректность их состояний.

Статика приводит к процедурному программированию, тогда как в объектно-ориентированной парадигме мы инстанцируем объекты и позволяем им управлять данными как и когда это нужно. При использовании статики невозможно проектировать на основе контрактов.

Внешние зависимости, оператор new

Все внешние зависимости передаются в конструктор через DI. Если нам нужны объекты с состоянием, которые не можем инжектить через DI (Newable, Transient objects), то используем фабрики. Фабрики инкапсулируют все сложные операции сборки объекта. При этом фабрику инжектим в класс через DI.

Пример:

$delimiter = '|';
new CsvFile(delimiter);

Заменяем на:

class CsvFileFactory implements FileFactoryInterface
{
   /**
    * {@inheritdoc}
    */
   public function createFromArray(array $params = []) : FileInterface
   {
        $delimiter = $params['delimiter'] ?? CsvFile::DEFAULT_DELIMITER;
 
        return new CsvFile(delimiter);
   }
}
 
$fileFactory->createFromArray(['delimiter' => '|']);

Универсальные объекты

Существуют такие универсальные объекты, которые имеют доступ к другим общим объектам. Например, они используются в таких шаблонах проектирования, как сервис локатор и реестр. Оба нарушают закон Деметры: они знают про всю группу объектов, тем самым провоцируя высокую связность системы.

Вместо этого инжектим только нужные нам зависимости.

Используя универсальные объекты, мы имеем чистый конструктор. Отказавшись от такого подхода, нам, возможно, придется передавать много зависимостей в конструктор. Это может указывать на то, что у класса не единая ответственность и необходимо пересмотреть дизайн системы.

Пример:

public function updateAction() : Response
{
    return $this->get('app.user_handler.service')->handle();
}

Заменяем на:

public function updateAction(UserHandler $userHandler) : Response
{
    return $userHandler->handle();
}

Вместо выводов

После проведения нескольких сессий рефакторинга мы поняли, что они не только постоянно улучшают кодовую базу наших проектов. Они еще влияют и на мотивацию разработчиков, которые могут приводить в код в соответствие с уровнем своей экспертизы. При правильном развитии программиста он постоянно повышается.

Я не претендую на истину и понимаю, что не все согласятся с вышеизложенными подходами. В этой статье я хотел рассказать о тех решениях, которые мы используем в компании. Если ваши подходы и принципы отличаются, приглашаю рассказать о них в комментариях.

LinkedIn

27 комментариев

Подписаться на комментарииОтписаться от комментариев Комментарии могут оставлять только пользователи с подтвержденными аккаунтами.

Статья неплохая, но для новичков. И таки-да, поиск ошибок не является целью рефакторинга. Не знаю, как в PHP, но в Java рефакторить можно только код, покрытый тестами. Иначе как вы узнаете, что не внесли новых ошибок?

найти ошибки

Слишком обще. Какие ошибки? Функциональные? Рефакторинг не для этого. Проводя рефакторинг ни в коем случае нельзя изменять функциональность программы. Если в процессе рефакторинга находится функциональная ошибка — весь код шелвится, ошибка исправляется и только после этого процесс рефакторинга продолжается уже на исправленой системе. А не-функциональные ошибки типа ошибок проектирования мы как раз исправляем так что «найти» их уже поздно.

public function hydrate(array $data, $object) : void
{
   foreach($data as $property => $value) {
       $setterName = 'set' . $property;
 
       if (method_exists($object, $setterName)) {
           $object->{$setterName}($value);
       }
   }
}

я понимаю что это — пхп, но не надо делать так только потому что вы можете. Проблем здесь как минимум две: У вас кодзавязан на формат в котором лежат данные. Т.е. вы expose-ите принцип хранения чего-то на всю часьт аппа что знает о этом $object/$dto unit-е. Если когда-либо принцип хранения изменится — изменится должна будет вся та часть аппы, а это, подозреваю, дикие количества строк ктода. Далее: method_exists и конкатенация. Вы бы ещё eval добавили, право дело! Тут то скрытые баги и полезли. Молодец, ничего не скажешь. Вместо того чтоб работать по контракту и наглядно показывать что и откуда мы берём и что и куда и в каких обстоятельствах перекладываем мы полагаемся на «авось» когда структура данных сменится мы не забудем в DTO методов накидать. Нельзя так. Гораздо лучше — fail fast чтоб ещё при первой сборке/тесте это упало если где-то кто-то что-то завмыкал. Код то пишут люди и люди завмыкивают. Более того в таком магическом коде оптимайзер не сможет заинлайнить метод-коллы + магия в пыхе адски дорога. Такой код мог написать только человек который не знает ни как стэк пыха работает ни структуры стэк-фрейма никогда в глаза не видел ни что такое опкод и почему он важен. Возможно даже структуру zVal’а не знает и не слышал. Наоборот, рефакторинг надо проводить для того чтоб избавится от такого кода. До этого рефакторинга (а это вне сомнения он и есть только не во благо, а как раз наоборот) было лучше. Единственное что — этот флюент код с цепочкой сеттеров можно было поместить в вашу hydrate функцию.

Чтобы избежать нагроможденности, стоит заменять условные выражения стратегией и/или спецификациями

нет же! Стратегия не для того используется. Используя стратегию как вы — вы нарушаете LSP. Вы не можете альтерить поведение на инвариантах. Именно потому стратегия ещё называется «алгоритм». Strategyиспользуется для того чтоб в одной и той же точке программы выбирать способ которым добудется результат, а не выбирать то, каким результат будет. Для этого существует полиморфизм в ООП, хотя адептам -er, -or и прочей анемичности этот термин на практике не известен, но тогда так и скажите что вы работаете в процедурном стиле. Здесь нет ничего плохого, много годного кода написано в процедурном стиле, здесь нечего стыдится. Стыдится и избегать нужно как раз того что вы делаете — выдавания одного стиля за другой. В данном случае выдавания процедурной анемичности за ооп.

Решение — вынести if из метода addData в спецификацию. А метод format — в отдельный класс, применив стратегию:

тем самым introduce-ив implicit content coupling. Как раз то, с чем обычно борятся. Отлично. Просто великолепно. И тим/тех лид пропустил такой код на ревью?

/**
    * Format response data part.
    *
    * @param ResponseDtoInterface $data
    * @return mixed
    */
   public function process(ResponseDtoInterface $data)
   {
       foreach ($this->formatters as $formatter) {
           $data = $formatter->format($data);
       }
 
       return $data;
   }
Счего-ли ваш data вдруг mixed? У вас есть право делать его mixed? Вы же сразу его консюмите вашим форматтером, который требует выполнения контракта. А какой контракт, если оно mixed?
Заменяем на: $isExceededLimitPerFile

только в зависимости от конвенции. Если вас форсят флаги начинать на is- то да. Если более разумные люди работают — имя limitPerFileExceeded тоже будет вполне ок и даже более естественно звучать.

оптимальные для прочтения методы — это такие, которые имеют длину не более 10 строк

когда-то тоже так думал, но в последствии есть места где пожалел о таком решении. Говорить о том что юнит большой или нет просто по количеству строк кода — безсмысленно и беспощадно. Да, это самая лёгкая метрика, но не самая правильная. Да, «большие» юниты — всё ещё плохо, но разделять их нужно при достижении какого-то лимита концептов встречаемых в этом юните, но никак не просто по строкам кода. Излишне большое количество мелких юнитов ни чем не лучше для понимания (а то и хуже) чем большой кусок кода. Ещё раз обращаю внимание на слово «излишнне».

Длинный список параметров

применимо только если вы полностью используете ООП с инкапсуляцией и полиморфизмом. Т.е. если не страдаете анемией кода. Иначе такие метрики и попытки в них вкладываться выглядят как «у бедых людей самолёты тоже из соломы, просто они лучше притворяются». Это касается передачей в метод нетипизированой хеш-мапы и бравое репортование о том что это «один аргумент».
Нет, это не так. Это не один аргумент. Вы просто в попытке вложиться в метрику убили систему хинтования и проверки контракта.

несоблюдение закона Деметры

то же самое. Для соблюдения этого закона нужно чтоб данные находились там же где и операции. Т.е. чтоб юниты несли операции, а не были анемичными структурами с геттерами/сеттерами и/или отдельными процедурами по обработке/мутированию данных.

class CsvFileFactory implements FileFactoryInterface
{
   /**
    * {@inheritdoc}
    */
   public function createFromArray(array $params = []) : FileInterface
   {
        $delimiter = $params['delimiter'] ?? CsvFile::DEFAULT_DELIMITER;
 
        return new CsvFile(delimiter);
   }
}
 
$fileFactory->createFromArray(['delimiter' => '|']);
Нет же. Здесь снова coupling. Теперь вы не можете подставить другой инвариант, а expose-ите то, что на самом деле это csv-file или что-то с делимитером. Нетипизированые аргументы, непонятный список аргументов (как я из одного контракта узнаю что мне надо передвавть в params?) и прочий карго-культ при следовании метрикам только мешает при разработке. Не нужна эта фабрика. Этот файл может сам по себе приходить из DI и DI имеет право знать как такой инстанс собрать. Вам, как потребителю этого инстанса нужно знать только контракт, который вы задаёте. Т.е. минимальный набор действий что с этим файлом может ваш юнит совершить.
Универсальные объекты

Иногда они просто нужны. Обычно DI-frameworks описывают легитимные юз-кейсы таких юнитов. Очень характерный пример — разрешение circular reference. Попробуйте добавить constructor-dependency на Doctrine репозиторий в конструктор Doctrine event handler’а например. Но тут тоже лучше пользовать не на прямую локатор/контейнер, а бридж через контейнер к юниту. Типа такого:

interface SomeUnit {
    public importantBusinessAction(): void
}
class ConcreteUnit implements SomeUnit {
    public importantBusinessAction(): void {
        // todo
    }
}
class SomeUnitContainerBridge implements SomeUnit {
    private $container; // from constructor
    public importantBusinessAction(): void {
        // Demeter violation as a tradeof
        $this->container->get('namespace.service_alias')->importantBusinessAction();
    }
}

стаття не дуже якісна, після

public function hydrate

вже все стало остаточно зрозуміло

Коментар не дуже якісний. Після відсутності аргументованого пояснення що саме не так, вже стало остаточно зрозуміло, що коментатор любить кидатися лайном.

я прокоментував комент який мені сподобався ), а там досить доступно @Alexandr Sova пояснив саме цей приклад. «Кидатись лайном» тут не є метою, просто з одної сторони треба максимально підтримувати людей які проявляють ініціативу і пишуть такі статті але з іншої сторони коли контент не дуже то критика повинна сприйматись нормально

Так що не так із

public function hydrate

?
Давайте своїми словами.

Достаточно ёмко и содержательно. Годная статья.
Хоть большую часть и так знал.
Единственное, не очень понятно про проблемы наследования и предпочтение композиции (мне-то понятно, но для новичков, полагаю, что нет).
Открыл для себя спецификации. Хотя ради одной строчки кода я возможно и не стал бы создавать целый класс (очень сомнительное, на самом деле, заявление). К тому же появляется лишняя зависимость в конструкторе и куча «лишних» тестов. Иногда возможно проще нарушить какие-то принципы, если понимание кода при этом будет проще. Это очень субъективно и на истину не претендую. Возможно, просто ещё мало опыта.
И фабрики через DI обычно не передаю. На самом деле в тех контекстах, в котором их использую, и не нужно. Юнит-тестированию мне это не мешало, по крайней мере. Хотя понимаю, что скорее всего что-то делаю не так.
По длине методов, тоже стараюсь обычно в районе 5-15 строк, хотя бывают случаи, когда получаются более длинные, но правило одного экрана (20-30 строк) всё-таки святое.
А в целом, всеми руками ЗА указанные принципы.
От себя добавил бы — по возможности использовать минимальное количество открытых методов, поскольку: а) придётся поддерживать обратную совместимость; б) слишком открытый интерфейс класса даёт слишком много возможностей использовать его не так как задумано.

пхп так суров что у него даже чисто свои чисто внутреннего потребления паттерны есть #notbad

Статья хорошая. В добавок можно почитать «Чистый код» Роберта Мартина www.amazon.com/...​aftsmanship/dp/0132350882.

Приятно узнавать автора :)

Спасибо за статью! Очень полезно и актуально.Для меня так точно.

Спасибо! Очень рад, что статья оказалась полезной для Вас.

Две цитаты:

«Programs must be written for people to read, and only incidentally for machines to execute.»
― Harold Abelson

«Any fool can write code that a computer can understand. Good programmers write code that humans can understand.»
― Martin Fowler

И в заключении можно еще раз заглянуть в википедию и перечитать внимательнее: «... цель — упростить понимание работы программы.» Понимаемо, что , пардон, понимание работы программы человеком, программером, коотрый придет на Ваше место позжее. Соответственно, и цели рефакторинга начинаются с «сделать код более понятным для других...», а «найти ошибки» — это никак не цель рефакторинга, а возможный приятный бонус в его результате :) Найти ошибки — это цель отладки, смотрим туда же, в вики: «Отла́дка — этап разработки компьютерной программы, на котором обнаруживают, локализуют и устраняют ошибки».

Да, Вы правы. Спасибо за комментарий!

Пересказ Анкла Боба на пхп. Две книги клин кода автору.

Спасибо. Как я писал в статье, основной целью является поделиться нашими подходами и рассказать, что важно проводить рефакторинг. Если говорить о литературе, то помимо Роберта Мартина, также были рассмотрены работы Фаулера, Кериевски, Вернона.

Не могу не заметить: ссылаться на не профильный ресурс, а именно Википедию, в профильной статье для определения базовых понятий не профессионально. Складывается впечатление, что Вы не читали указанных авторов и пытаетесь изобрести велосипед, который уже изобрели лет 18 назад. Обратите внимания, что в комментариях уже дважды предложили ознакомится с трудами авторитетов.

А теперь начинается тестирование...

Цікава стаття. Також кому цікаво дана тема буде дуже корисно почитати книжку Мартіна Фаулера про техніки рефакторинга martinfowler.com/books/refactoring.html

Спасибо! Согласен, книга полезная.

Статья хорошая, но все-таки тему «рефакторинг» не всунуть в одну статью, очень много различных приемов есть, это нужно скореее линейку статей делать.

Согласен с Вами. Практически на каждый пример, можно написать отдельную статью. Спасибо за комментарий!

Спасибо за статью! Вставлю свои пять копеек о рефакторинге: refactoring.guru/ru/refactoring

Рад, что Вам понравилось! Спасибо!

Подписаться на комментарии