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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Підписуйтеся на Telegram-канал «DOU #tech», щоб не пропустити нові технічні статті.

До обраного В обраному 1

Адам Леос, Senior Software Engineer в PlutoTV, рассказывает о сложности алгоритма, нотации Big O, сортировках, самых популярных структурах данных и их использовании для оптимизации проекта. 134

Це продовження попередньої статті про підготовку додатків до локалізації. Тут ми розглянемо наступний етап — коли перший реліз уже позаду, у нас уже є користувацька аудиторія, і ми хотіли б цю аудиторію розширити. 18

Продолжаем знакомство с Go. В этой статье будет показано, как с помощью простых инструментов Go можно создавать высокопроизводительный код под многоядерные системы. 116

первым делом вот такие бессмысленные пхпдоки нужно отрефакторить. Они вносят много шума

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

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

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

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

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

Счего-ли ваш data вдруг mixed? У вас есть право делать его mixed? Вы же сразу его консюмите вашим форматтером, который требует выполнения контракта. А какой контракт, если оно mixed?

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

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

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

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

Нет же. Здесь снова coupling. Теперь вы не можете подставить другой инвариант, а expose-ите то, что на самом деле это csv-file или что-то с делимитером. Нетипизированые аргументы, непонятный список аргументов (как я из одного контракта узнаю что мне надо передвавть в params?) и прочий карго-культ при следовании метрикам только мешает при разработке. Не нужна эта фабрика. Этот файл может сам по себе приходить из DI и DI имеет право знать как такой инстанс собрать. Вам, как потребителю этого инстанса нужно знать только контракт, который вы задаёте. Т.е. минимальный набор действий что с этим файлом может ваш юнит совершить.

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