Полезно ли мини-рефакторинг кода в надежде улучшить качество, или это просто «перемещение кода» без особой выгоды?

12

пример

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

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

Оригинальный код:

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

if ($verid)
    $sql1 = "SELECT * FROM product_spec WHERE id = " . clean_input($verid);
else
    $sql1 = "SELECT * FROM product_spec WHERE product_id = " . clean_input($productid) ;
$result1 = query($sql1);
$row1 = fetch_array($result1);
/* html markup follows */

Рефакторинг:

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

//some implementation details omitted 
$this->repository = new SpecRepository($mysql);
if ($verid)
    $row1 = $this->repository->getSpecByVersion($verid);
else 
    $row1 = $this->repository->getSpecByProductId($productid);
/* html markup follows to be refactored or left alone till another time*/

//added new class:
class SpecRepository extends MySqlRepository
{

    function getSpecByVersion(int $verid)
    {
        return $this->getMySql()->paramQuery("
            SELECT * FROM product_spec WHERE id = ?
        ", $verid)->getSingleArray();
    }

    function getSpecByProductId(int $productid)
    {
        return $this->getMySql()->paramQuery("
            SELECT * FROM product_spec WHERE product_id = ?
        ", $productid)->getSingleArray();
    }
}

Должен ли я сделать это?

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

Я предвижу некоторые ответы: «Если вы не знаете причин, по которым вы проводите рефакторинг, не делайте этого», и, возможно, я могу согласиться. Мои причины - со временем улучшить качество кода (и я надеюсь, что я буду делать это, следуя SRP и другим принципам).

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

Почему я рефакторинг в первую очередь?

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

Деннис
источник
ОТ, но нужно учесть, что Select * from ..это можно считать антишаблоном. См. Stackoverflow.com/q/3639861/31326
Питер М
1
@PeterM: Если вы не пишете ORM, в этом случае select *становится «лучшей практикой».
Роберт Харви
@RobertHarvey В этом случае я задаюсь вопросом, почему вы пишете свой собственный ORM: D
Питер М
Я сделал рефакторы для более неактуальных причин. Сокращение технического долга - это хорошо, как только выгоды перевешивают затраты. Похоже, это ваш случай. Вопрос в следующем: есть ли у вас подходящие тесты, чтобы убедиться, что код по-прежнему работает так, как ожидалось?
Laiv
1
Надежность должна быть преобладающим показателем при рефакторинге IMO, а не ремонтопригодностью, которая связана с изменчивостью, поскольку надежность в первую очередь уменьшает причины для изменений. Это повышает стабильность, а код, который является абсолютно стабильным и надежным и выполняет все, что ему нужно для выполнения, требует небольших затрат на обслуживание, поскольку у него очень мало причин когда-либо менять. Старайтесь уменьшить причины изменений, а не пытаться сделать их как можно проще. В поисках первого также будут склонны давать некоторые из последних.

Ответы:

13

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

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


В общем:

Помогает ли рефакторинг другим разработчикам понять ваш код?

Помогает ли рефакторинг улучшить код?

Упрощает ли рефакторинг поддержку кода?

Упрощает ли рефакторинг отладку?

Если ответ на любой из этих вопросов был «да», то это того стоило, и это было больше, чем просто «перемещение кода»?

Если ответом на все это было «нет», то, возможно, его не нужно подвергать рефакторингу.

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

FrustratedWithFormsDesigner
источник
9

Рефакторинг действительно раздувается, если вы разбираете монолит.

Тем не менее, вы уже нашли выгоду.

«В моем случае я добавляю новые функциональные возможности для новой линейки продуктов».

Вы не можете добавить функциональность к монолиту очень легко, не ломая вещи.

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

дьякон
источник
да, но я также могу продолжать добавлять в монолит даже новые функции :), то есть я бы добавил новый if/elseблок и следовал бы оригинальному стилю кода для добавления операторов SQL, а затем я бы поместил новую разметку HTML в тот же файл монолита. И чтобы изменить столбцы, я бы нашел блок if / else, отвечающий за размещение строк SQL, и отредактировал этот SQL, чтобы внести изменения в столбцы, не разбивая этот файл.
Деннис
При рефакторинном подходе я, скорее всего, сначала пошел бы в блок if / else в монолите, чтобы увидеть, что мне нужно перейти в отдельный файл репозитория, чтобы изменить SQL. Или, если бы я работал над этим монолитом недавно, я бы знал, что нужно перейти прямо в файл хранилища, минуя монолит. Или, если бы я искал в своей кодовой базе конкретный SQL, поиск поместил бы меня в новый файл репозитория, также минуя монолит
Деннис
2

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

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

Я ценю рефакторинг по следующим причинам:

  • Я лучше понимаю код
  • Я удаляю дублированный код, поэтому, если у меня есть ошибка в одном месте, мне не нужно охотиться везде, где код был скопирован
  • Я создаю классы, которые имеют свою четко определенную роль. Например, я могу использовать один и тот же репозиторий для увлажнения HTML-страницы, XML-канала или фонового задания. Это было бы невозможно, если бы код доступа к базе данных жил только в классе HTML.
  • Я переименовываю переменные, методы, классы, модули, папки, если они не соответствуют текущей реальности; Я commentкод через имена переменных, имена методов
  • Я решаю сложные ошибки сочетанием юнит-тестов и рефакторинга
  • У меня есть гибкость, чтобы заменить целые модули, пакеты. Если текущий ORM устарел, содержит ошибки или не обслуживается, его легче заменить, если он не распространяется по всему проекту.
  • Я обнаруживаю новые возможности или улучшения, которые приводят к предложениям для клиента.
  • Я создал повторно используемые компоненты. Это было одним из величайших преимуществ, потому что работа, выполненная в этом проекте, могла быть повторно использована для другого клиента / проекта.
  • Я часто начинаю с самого тупого, жестко закодированного, дурацкого решения и реорганизую его позже, когда узнаю больше. Этот более поздний момент может быть сегодня или, может быть, через несколько дней. Я не хочу тратить слишком много времени architectна решение проблемы. Это придет во время написания - переписать код туда и обратно.

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

Я также представил ошибки. Я понимаю, почему некоторые QA спрашивают. are you guys doing refactoring? because everything stopped working!Это риск, который должна охватить команда, и мы всегда должны быть прозрачными в этом.

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

Адриан Ифтоде
источник
Это очень близко к отрицательному мнению - в «Программисте» нет «Я». Под "я" вы меня "я" (голосование вниз) или вы имеете в виду разработчиков, работающих над кодом сейчас и в будущем?
Mattnz
Я тоже это заметил, но я сформулировал это. Также я много лет работал сольным разработчиком, и когда я писал этот ответ, я повторял в основном этот опыт. Я мог бы заменить Iс toхотя.
Адриан
1

Я думаю, что вопрос, который вы должны задать себе, не столько "Есть ли выгода?" но "Кто заплатит за эту работу?"

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

Это слишком просто, когда вы в команде разработчиков где-то видите код и хотите реорганизовать его, чтобы сделать его «лучше». Но определить ценность «лучшего кода», когда продукт уже работает, действительно сложно.

Такие вещи, как «ускоренная разработка новых функций», не сокращают его, поскольку они просто снижают затраты. Вы должны генерировать продажи.

Ewan
источник
1

На мой взгляд, рефакторинг - это хорошая инвестиция.

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

Но чтобы сделать рефакторинг, вы также должны инвестировать в тесты. Вы должны создавать автоматизированные тесты, в идеале вы должны иметь модульные тесты и интеграционные тесты. Это предотвращает нарушение существующего кода.

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

Bernie
источник