Устаревший код – сторонний код

https://robertbasic.com/blog/legacy-code-is-3rd-party-code/
  • Перевод
image

В TDD-сообществе существует совет, который говорит о том, что мы не должны использовать mock-объекты для типов, которыми не владеем. Я считаю, что это хороший совет, и стараюсь следовать ему. Конечно, есть люди, которые говорят, что мы вообще не должны использовать mock-объекты. Независимо от того, какого мнения вы придерживаетесь, совет «не имитировать то, что не ваше» – содержит в себе еще и скрытый смысл. Люди часто пропускают его мимо ушей, видя слово «mock» и впадая в ярость.

Этот скрытый смысл заключается в том, что следует создавать интерфейсы, клиенты, мосты, адаптеры между нашим приложением и сторонним кодом, которым мы пользуемся. Будем ли мы создавать mock-объекты этих интерфейсов в наших тестах даже не так важно. Важно то, что мы создаем и используем интерфейсы, которые лучше отделяют наш код от стороннего. Классическим примером этого в мире PHP будет создание и использование HTTP клиента в нашем приложении, который использует Guzzle HTTP client, вместо использования Guzzle напрямую.

Почему? Хорошо, для начала, Guzzle имеет гораздо более мощный API, чем тот, который вашему приложению (в большинстве случаев) нужен. Создание своего HTTP клиента, который предоставляет только необходимый набор из API Guzzle, ограничит разработчиков приложения в том, что они смогут с этим клиентом сделать. Если API Guzzle изменится в будущем, нам надо будет необходимо внести изменения в одном месте, вместо того, чтобы исправлять его вызовы во всем приложении в надежде, что ничего не сломается. Две очень хорошие причины, и я даже не упомянул mock-объекты!

Я не думаю, что этого трудно достичь. Сторонний код обычно лежит в отдельной папке нашего приложения, часто это vendor/ или library/. Он также располагается в другом пространстве имен и имеет другое соглашение об именовании, чем то, что используется в нашем приложении. Сторонний код довольно легко определить и, с небольшой долей дисциплины, мы можем сделать код нашего приложения менее зависимым от сторонних частей.

Что, если мы применим те же правила к устаревшему коду?


Что если мы будем смотреть на наш легаси-код, так же, как и на сторонний? Это может быть трудно сделать, или даже контрпродуктивно, если устаревший код используется исключительно в режиме поддержки, когда мы только правим баги и немного подстраиваем небольшие его части. Но если мы пишем новый код, который (пере)использует устаревший, я считаю, что стоит рассматривать его так же, как и сторонний код. По крайней мере с точки зрения нового кода.

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

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

Другая, может даже более серьезная проблема с устаревшим кодом в том, что мы готовы изменять его, исправлять, взламывать его потому, что не рассматриваем его как сторонний код. Что мы делаем, когда видим баг или хотим добавить новую возможность в сторонний код? Мы описываем проблему и/или создаем pull request. То, что мы не делаем – это не идем в папку vendor/ и не правим код там. Почему мы так делаем с устаревшим кодом? А потом скрещиваем пальцы и надеемся, что ничего не сломалось.

Вместо того, чтобы слепо использовать устаревший код в новом коде, давайте попробуем написать интерфейсы, которые будут включать только требуемое подмножество API старого «божественного» объекта. Скажем, у нас есть объект User в устаревшем коде, который знает все обо всем. Он знает как изменять email и пароль, как повышать пользователей форума до модераторов, как обновлять публичные профили пользователей, устанавливает настройки уведомлений, сохраняет сам себя в базе и многое другое.

src/Legacy/User.php
<?php
namespace Legacy;
class User
{
    public $email;
    public $password;
    public $role;
    public $name;

    public function promote($newRole)
    {
        $this->role = $newRole;
    }

    public function save()
    {
        db_layer::save($this);
    }
}

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

Давайте ограничим сами себя и запретим обращаться к этим публичным свойствам и попробуем угадать, как устаревшая система работает при повышении прав пользователя:

src/LegacyBridge/Promoter.php
<?php
namespace LegacyBridge;
interface Promoter
{
    public function promoteTo(Role $role);
}

src/LegacyBridge/LegacyUserPromoter.php
<?php
namespace LegacyBridge;
class LegacyUserPromoter implements Promoter
{
    private $legacyUser;
    public function __construct(Legacy\User $user)
    {
        $this->legacyUser = $user;
    }

    public function promoteTo(Role $newRole)
    {
        $newRole = (string) $newRole;
        // Ты думал, что $role в устаревшей системе это строка? Угадай теперь!
        $legacyRoles = [
            Role::MODERATOR => 1,
            Role::MEMBER => 2,
        ];
        $newLegacyRole = $legacyRoles[$newRole];
        $this->legacyUser->promote($newLegacyRole);
        $this->legacyUser->save();
    }
}

Теперь, когда мы хотим повысить права User в новом коде мы используем интерфейс LegacyBridge\Promoter, который имеет дело со всеми тонкостями повышения пользователя в устаревшей системе.

Изменение языка наследия


Интерфейс для устаревшего кода дает нам возможность улучшить дизайн системы и может избавить нас от возможных ошибок в именовании, которые были сделаны давно. Процесс изменения роли пользователя с модератора на участника это не «promotion» (повышение), а скорее «demotion» (понижение). Никто не мешает нам создать два интерфейса для этих разных вещей, даже если устаревший код будет выглядеть так же.

src/LegacyBridge/Promoter.php
<?php
namespace LegacyBridge;
interface Promoter
{
    public function promoteTo(Role $role);
}

src/LegacyBridge/LegacyUserPromoter.php
<?php
namespace LegacyBridge;
class LegacyUserPromoter implements Promoter
{
    private $legacyUser;
    public function __construct(Legacy\User $user)
    {
        $this->legacyUser = $user;
    }

    public function promoteTo(Role $newRole)
    {
        if ($newRole->isMember()) {
            throw new \Exception("Can't promote to a member.");
        }
        $legacyMemberRole = 2;
        $this->legacyUser->promote($legacyMemberRole);
        $this->legacyUser->save();
    }
}

src/LegacyBridge/Demoter.php
<?php
namespace LegacyBridge;
interface Demoter
{
    public function demoteTo(Role $role);
}

src/LegacyBridge/LegacyUserDemoter.php
<?php
namespace LegacyBridge;
class LegacyUserDemoter implements Demoter
{
    private $legacyUser;
    public function __construct(Legacy\User $user)
    {
        $this->legacyUser = $user;
    }

    public function demoteTo(Role $newRole)
    {
        if ($newRole->isModerator()) {
            throw new \Exception("Can't demote to a moderator.");
        }
        $legacyModeratorRole = 1;
        $this->legacyUser->promote($legacyModeratorRole);
        $this->legacyUser->save();
    }
}


Не такое уж большое изменение, но цель кода стала гораздо яснее.

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

Ну. И что?
Реклама
Комментарии 35
    +5

    КМК, наглядный пример старого изречения: "можно решить любую проблему добавлением еще одного уровня абстракции, кроме проблемы слишком большого количества абстракций"
    На практике я бы все-таки учитывал насколько легаси действительно легаси, и сколько нам его надо.
    Я занимался сам изоляцией легаси, и это может быть очень круто — сам видел.
    Но заниматься этим при каждом удобном случае… сомневаюсь. А уж особенно сравнивать это со сторонним кодом — очень странно. У легаси есть одно положительное свойство — оно, как правило, не меняется. Так что с точки зрения тестов чуть ли не первый кандидат на мок (хотя проблемы, которые описаны, безусловно имеют место быть)

      +2
      > У легаси есть одно положительное свойство — оно, как правило, не меняется.

      Сильно зависит от определения легаси. Есть и такое (недословно): «легаси — код, при изменениях которого ты не можешь сказать, будут ли нежелательные сторонние эффекты».
        0

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

          0
          Кандидаты на изменение места, где реализована изменившаяся в требованиях логика. Грубо два реальных варианта: лезть менять код, в котором ты не уверен, и обернуть этот код и менять его обёртку.
            0

            Да, все так есть. И бывают случаи, когда надо править легаси, матюгаясь и скрестив пальцы, чтобы ничего не сломалось. И бывают случаи, когда лучше обернуть легаси и жертвовать чем-то ради того, чтобы не ковыряться в этом гуано динозавров.
            Но посыл статьи ведь не в этом, а в сравнении легаси со сторонним кодом с точки зрения TDD. А это неочевидно — если и так необходимо оборачивать легаси, тесты тут уже побоку, а если дело только в тестах, то сравнение со сторонним кодом сомнительно хотя бы по вероятности изменения

              0
              Легаси можно понять и принять, и даже улучшать и рефакторить.

              А можно отгородиться от него обёрткой, которая с таким подходом через пару лет и для автора станет легаси, и он начнёт писать следующую обёртку. И, вместо того, чтобы исправить 1 проверку где-то в глубине первоначального кода, будет править 100500 обёрток, написанные под разные сценарии.
                0
                значит вы не поняли смысла этой обертки.
                  0
                  Тут главный вопрос: можно ли править старый код (за обёрткой), или надо это всеми силами избегать и все правки делать только в новом коде.
                    +1
                    обертка нужна для того, что бы иметь возможность заменить реализацию. «заменить» не всегда означает «переписать с нуля», чаще это мы берем старый код, копируем его, пишем тесты что бы убедиться, что код ведет себя так, как нам надо, и правим его если это не так.

                    Ключевое тут — вы можете очень планомерно переводить проект на новую реализацию, покрывать все тестами, рефакторить и т.д.

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

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

                      То есть, вместо переписывания кода предлагалось переписывать обёртку.
                        0
                        «переписывать обертку» глупо, обертка она обертка. Она должна позволять вам переписать то что под капотом, а не сам капот.
                          0
                          Надо было написать это не мне, а под комментарием VolСh, которым началась эта ветка ))
                      0
                      Лучше не надо править старый код пока есть его использование без обёрток.

                      Но нужно понимать, конечно, что нет абсолютных правил или догм. Главный критерий — увереннность в понимании того, чо старый код делает и на что повлияют потенциальные изменения.
                        0
                        Лучше не надо править старый код пока есть его использование без обёрток.
                        Риски от правки старого кода никак не меняются, написана вокруг него обёртка, или нет.
                          0
                          меняются. Эффекты от изменений изолированы на уровне обертки. Как пример — branch by abstraction как раз таки пример таких оберток для рефакторинга планомерного.

                          А так да, придется тесты писать и думать что делаете, магии не бывает.
                            +1
                            Риски от правки старого кода меняются не от того, что написана вокруг него обёртка, а от уменьшения количества мест в коде, где старый код используется без обёртки. Они не исчезают полностью, но значительно уменьшаются.

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

            0
            Идея здравая, но как определить легаси код?

            В каждый момент времени можно найти код с разной степенью устаревания, думаю, нельзя выделить конкретный момент, когда уже нужно делать обёртку над ним. Вндрение подобных абстракций, скорее, должно быть архитектруным решением, которое принимается на очередном цикле стратегического планирования.
              –1
              Есть определение, сомнительное, но мне нравится «легаси-код — код без тестов» :)
                0
                Интересное определение.

                Но если новый код, который использует легаси, покрывается тестами, то легаси код тоже (в некотором смысле) автоматически покрывается тестами :-)

                А если новый код тестами не покрывается, то пользы от абстракции нет.
                  0
                  Не покрывается. Если старый код будет замокан, то вы протестите только, что моки работают как вы определили
                    0
                    а, да, про моки забыл.
                      0
                      Можно написать абстрактные тесты на интерфейс, принимающие какую-то реализацию. И передать легаси-реализацию.
                0
                Устаревший код это на сколько в глубину лет? Какой «аптайм» частей кода? 10-20-30 лет? Вот это интересно тоже.
                  0
                  Legacy code != устаревший код. Это вопрос к переводчику.

                  Большинство пишут код так что уже через пару дней его можно считать легаси.
                    0
                    legacy переводится как наследство, наследие. Legacy code — унаследованный код буквально. В целом — это код, в котором ты не разбираешься досконально, но от тебя требуется его хотя бы поддерживать, а часто и развивать.
                    +1
                    Тоже задаюсь вопросом что подразумевать под легаси-кодом…
                    Одно точно ясно, что этим кодом мы полностью управляем…
                    Тогда скорее остаются вопросы: на сколько мы его знаем? на сколько он покрыт тестами?
                    На сколько он поддерживаемый?

                    Несколько забавных правил рефакторинга… это правило 3, прежде чем прежде чем рефакторить надо понять зону ответственности кода, надо код покрыть тестами…

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

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

                    Я не вижу пока причин полноценно относиться к легаси-коду как внешней зависимости.

                    первые разработчики сделали код.
                    вторые сделали уровень абстракции над предыдущим кодом.
                    третьи сделали уровень абстракции над предыдущим кодом.
                    четвертые…
                      +1
                      Скорее это код, которым по задумке начальства мы должны управлять как своим, но по факту он для нас практически сторонний.
                        0
                        Тут вопрос, прикладывает ли разработчик усилия, чтобы понять и принять тот код, или будет повторять: NIH, это не моё, я тут ничего не понимаю, могу написать заново, если точно скажете, как.
                          0
                          вы тут уже скатываетесь в другую проблему. Обманчивое «это проще выкинуть и переписать с нуля». Почти всегда когда разработчик произносит эту фразу, это скорее эмоции и отчаянье, нежели здравое решение.

                          Если у вас есть кусок системы которые непонятно что делает, то вам в любом случае придется разобраться с тем, что оно делает. Вот только в случае «перепишу ка я с нуля», велика вероятность что на выходе получится другой непонятный кусок кода который будет непонятен уже третьему разработчику, ведь легаси он на то и легаси что объективно оценить задачу у вас не выйдет. А это значит что в какой-то момент вы пойдете на компромис «сделать задачу хорошо или сделать задачу что бы работало но с не такими жесткими факапами по срокам»
                            0
                            я с Вами согласен, а также добавлю что есть еще одна причина, как правило легаси код рабочий, разработчики уже словили кучи проблем и поправили все что связано с бизнес логикой, то есть при переписывании можно не учесть многих нюансов, от сюда и как мне кажется и появилось правило трех. Так что переписывать все с нуля это плохо. Лучше на мой взгляд проводить частичный рефакторинг, что позволит придерживаться бизнес логики и изменять код который будет соответствовать требованиям. Как-то так…
                              0
                              Именно так, полностью согласен. Если переписывать с нуля, не факт, что лучше выйдет.
                              +1
                              Тут нюанс: в подавляющем большинстве случаев некому точно сказать как должно быть. А значит, понимать и разбираться придётся по любому.
                          +3
                          ИМХО легаси-код — это код, который отвечает хотя бы 2 критериям из 3:
                          1) ЧТО это? Это мы, что ли, пять лет назад писали? Срочно дайте мне меня-пять-лет-назад!
                          2) «принцип дома программиста» — если на этот код чихнуть — все развалится. Это нормально. Легаси — это если оно разваливается, но ты не понимаешь даже того, почему.
                          3) Приоритетное. Взято с реального проекта(обработчик скриптового языка на Lua, аргументы частично удалены для сокращения обьема). Если при беглом пролистывании наметанный глаз видит такое — то это срочно нужно как-то куда-то деть и обращаться бережно, как с хрустальной вазой. В отличие от 2 пункта — касается комментов, а не самого кода.
                          код
                          -- пофикшу позже, юникод неработает. Почему? ХЗ. КОИ-8 навсегда. Armat.
                          function tmp (...)
                          ...
                          end
                          
                          -- я ХЗ на кой тут это надо, но без этой ереси все падает к del . А логи можно и почистить руками. David.
                          callerr (...)
                          
                          WTF = GetScriptPosX(ScriptWay, ObrStroka, FlagZalypyByka)
                          -- Да, без флага никак. То есть совсем. И что он делает мы не знаем, возвращает nil. Но без него nil идет уже в основу. И как эта магия вызова сОтоны работает мы тоже ХЗ. Я не нашел даже где он выставляется - то ли в недрах функции, то ли вообще из двигла выдергивается, там, del, МАГИЯ происходит. David.
                          ...
                          
                          -- Я без понятия почему, но в двигле этих гомоdel значения XYZ мыши инвертированы. Кроме того, граничные значения опять таки инвертируются, так что далее - такой вот трэш. BK4Ever. И да, del v0.15.2 - ну просто полный del, баг на баге.
                          WTF4 = WTF4 * -1
                          if WTF4 == 128 then
                          WTF4 = 127
                          elseif WTF4 == -128 then
                          WTF4 = -127
                          elseif WTF4 > 128 or WTF4 < -128
                          callerr(...)
                          WTF4 = 0
                          end
                          SetScriptPos(X, WTF4, dec)
                          -- Почему везде DEC? Потому, что на дробных в одних местах надо точку, в других - запятую. В падлу, проще округлить. David. 
                          

                            +1

                            Предложенный способ не очень поможет, если в оборачиваемых legacy-методах совсем лапша с побочными эффектами.
                            Т. е. толку, что вызов User::promote обернут, если он внутри дергает кучу статических методов и отправляет email пользователю впридачу?

                              0
                              Выделить из него метод, который только промоутит и обертку сделать для него.

                            Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                            Самое читаемое