Избыточная сложность

Автор оригинала: Владимир Репин
  • Перевод
Слышал такое выражение, что для того чтобы стать программистом, нужно быть ленивым. Но иногда лень в программировании приводит к возникновению ужасного технического долга. В своей заметке об SRP я упоминал, что нарушение этого принципа может привести к увеличению сложности или даже к умножению ее. Один из моих коллег произвел на свет интересный пример, и я решил с его помощью продемонстрировать как это выглядит.



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

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

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

// first place
doSomething(true);

// second place
doSomething(false);

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

doSomething(flag: boolean): void {
  if(flag) {
    // do first thing
  } else {
    // do second thing
  }
}

Эту проверку можно описать как «если меня вызвали из места А, делаем одно, иначе меня вызвали из места Б, делаем другое». Этот флаг, этот «if» и есть то, о чем вся эта заметка. Сложность не исходящая от бизнес-требований. Естественно, я порекомендовал изменить код следующим образом:

// first place
doFirstThing();

// second place
doSecondThing();

//method is split into 2 parts each having their own responsibility
doFirstThing(): void {
  // do first thing
}

doSecondThing(): void {
  // do second thing
}

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

Здесь можно воскликнуть: «Но это же всего один 'if'», или: «Это нарушение очевидно, кто вообще так код пишет?». И тут на сцену выходит второй пример. В нём видно, что увидеть нарушение может быть заметно сложнее, а также что стоимость этого нарушения может составлять больше чем всего один «if». Как и в первом примере, функция используется в двух местах:

// first place
checkValidity(obj);

// second place
checkValidity(arrayOfObjs);

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

checkValidity(parm: MyType | MyType[]): void {
  if(Array.isArray(parm)) {
    parm.forEach(p => checkValidity(p));
  } else {
    // here the object gets checked 
    // and conditional exception is thrown
  }
}

Вот оно. Один «if» становится множеством «if»-ов. Если массив содержит 100 объектов, то этот «if» выполнится 101 раз. А на реальных данных у нас там могло быть 30 тысяч объектов, и это уже внушительный урон производительности.

Очевидно, следуя принципу единственной ответственности, этот метод нужно порефакторить так, чтобы получилось 2 метода:

checkItemsValidity(parms: MyType[]): void {  
  parms.forEach(p => checkItemValidity(p));
}

checkItemValidity(parm: MyType): void {
    // here the object gets checked 
    // and conditional exception is thrown  
}

Соотвественно также надо поправить точки вызова.

Занятно, что примеры, которые я приводил в заметке про SRP, вели к увеличению SLOC, эти же примеры, наоборот, ведут к небольшому его уменьшению, попутно с ожидаемым улучшением качества кода.

На этом всё. Всего лишь пара простых примеров для демонстрации важнейшего из принципов хорошего кода.

Похожие публикации

AdBlock похитил этот баннер, но баннеры не зубы — отрастут

Подробнее
Реклама

Комментарии 19

    +2

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

      0
      Возможно я что-то не понял, но что если в первом примере вызов осуществляется не с явным значением true/false, а с результатом логического выражения. В таком случае есть смысл разносить на 2 функции и выносить это условие в код вызова этих функций или все-таки нет?
        0
        Имеет все равно. Эта «сложность» — условие, по которому мы выполняем первую или вторую функцию — не относится к самой функции. Если вы затолкаете проверку внутрь, то вы усложните функцию без причины.
      0
      С разницей в день вышла другая статья, где была рекомендация, как мне показалось, на похожий случай:
      Не используйте булевы значения в качестве параметров

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

      Повторюсь, не делайте этого.

      Потому что те, кто будут читать ваш код, увидят getUserMessage(userId, true) и будут недоумевать, что это вообще такое?

      ИМХО:
      здесь ИМХО нужен особый подход.
      getUserMessage(userId, true) очевидно плохо.
      Нужны константы с осмысленными именами:
      const
       retrieveFullMessageFl = true;
       retrieveShortMessageFl = false;
      

      И вызов записывать только с этими константами:
      getUserMessage(userId, retrieveFullMessageFl);
      getUserMessage(userId, retrieveShortMessageFl);
      

      ИМХО писать 2 функции вместо одной не всегда удобно. Когда, нпр., выбор между коротким или длинным сообщениями отладка и модификация двух функций м.б. заметно сложнее, чем одной.
        0

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


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


        Кроме того, когда появится требование иногда возвращать только аннотацию краткой сути сообшения (допустим, первые 72 символа), этот код всё равно рефакторить. И в каком-нибудь динамически слабо неявно типизируемом языке с редакторами кода не так здорово, чтобы за три секунды это решить.

          0

          Используйте энамы.

            0

            Именно

          0
          Судя по тому, что вы привели, функция остаётся всё-таки одна, просто она управляется не флагом или напрямую булевыми занчениями, а именнованными булевыми константами, а сама функция при этом вообще не изменяется.
          Так что, речи об отдельной функции для коротких сообщений не идёт.
          0
          В статье про SRP я показывал как это делается. Пока вся реализация находится внутри этого метода, копипастить функцию может быть неудобно.
          Однако если ее декомпозировать, то эти две функции начнут выглядеть примерно так.
          getShortMessage(id) {
          const data = loadMessageData(id);
          return composeShortMessage(data);
          }
          

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

              ЗЫ и да, в той упомянутой статье есть такой пункт — не надо бояться выкинуть свой код в помойку. Конечно, у нас всегда не хватает времени, жмут оценки, давит начальство, «все было против нашей сборной по футболу»(с)
                0
                Видимо я неправильно понял Ваш код. Чтобы получить полное сообщение достаточно вызвать loadMessageData, а composeShortMessage выбирает из data только то, что нужно для краткого сообщения.
                  0
                  Это псевдокод, чтобы обозначить то, как примерно должны выглядеть методы.
                  Мне трудно это все доносить, люди, которые пишут код примерно так как здесь,
                  1
                  image
                  с трудом могут представить как это не делать код с флагом, а то и с двумя-тремя

                  Я же предполагаю, что код надо писать примерно так и хардкодные параметры, ака флаги, из такого кода испаряются сами собой.
                  2
                  image


                  Это примеры кода из реального проекта. их не предполагается внимательно читать, просто взгядом оценить, чтобы понять о чем идет речь.
                  Второй пример кода лучше не только потому, что он больше декомпозирован, но и еще потому, что получает список сущностей про списку ключей, а не как оригинальный, который получает всегда по одному ключу, что есть проблема N+1. Речь не о ней, конечно же, но стоило упомянуть, что код который лучше не обязан работать медленнее, он может и быстрее работать, чем «то что было раньше».

                  PS не получилось картинки из г-драйва подключить, работают если в отдельном окне открыть по правой кнопке
          0
          deleted
            0
              0
              del
                0

                То есть вы предлагаете вместо


                element.classList.toggle( 'visible' , isVisible )


                Везде писать


                if( isVisible ) element.classList.add( 'visible' )
                else element.classList.remove( 'visible' )

                Я правильно понял вашу мысль?

                  0
                  element.classList[ isVisible ? 'add' : 'remove' ]( 'visible' )
                  </joke>

                  Если я правильно понял посыл автора, то система должна содержать две специализированные функции (add и remove) и, опционально, собранную с параметром (toggle). В первом и особенно во втором примерах обе ветки if-а заинлайнены, а не выделены отдельными именованными функциями.

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

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