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

Я сейчас не говорю про «Административный кодекс», куда я как раз и отношу неправильное применение шаблонов, неиспользование тестов, неоптимизированный код, даже харкодинг каких-нибудь настроек и «магические числа» (хотя уже на грани). В этих случаях разная правоприменительная практика. Например оптимизированный код часто сложнее для понимания, чем неоптимизированный. Неоптимальный алгоритм зачастую легче воспринимается при чтении кода, а ведь разработчик 95% времени читает свой или чужой код и только 5% пишет. Или если вы пишите скрипт для друга забесплатно, побыстрее и заходкодили пару настроек, вы скорее всего правильно поступили. Решив, что интеграция туда логики извлечения настроек (и ее тестирования) из отдельных конфигов потребует намного большего времени, чем хардкод.

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

Статья 1. Закомментированный код


Как часто в проектах любого уровня это встречается, человек как бы говорит вам: я немного не уверен, может быть это еще понадобится, а может и нет я — хз, просто оставлю это пока тут. Типичный кодомусорщик. Есть даже целые закомменченные модули. Это опять же очень сильно влияет на читаемость кода, даже несмотря на то, что он идет другим цветом. Решение есть — просто удалить, и все. Но если вы такой хламушник, можете сделать в git спец ветку с названием например warehouse-of-old-trash и ваша душа будет спокойна.

Статья 2. Мертвый код. Мертвые зависимости


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

Сюда же можно добавить бестолковые заглушки. Например вчера увидел такое:

if (true){
  someFunction();
}

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

// нужно проверить на то-то, 
someFunction();

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

Статья 3. Копипаст


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

Статья 4. Гигантские методы


О да, этим страдают даже более-менее приличные проекты. По моему убеждению, метод должен быть не больше экрана компьютера (± усредненного конечно). Иногда на том же github сидишь разбираешься в чьем-то коде и понимаешь, что ты уже 3й экран пролистал, а идет та же самая функция, и мозг уже отказывается что-то понимать. Ну разбей ты по некоторым признакам код на функции, ну понятней в сто раз будет да еще и возможно переиспользовать какие-нибудь можно будет. Даже экран это много, в такой размер как исключения могут попадать функции с имплементацией математических выражений, где разбиение на мелкие части понимания может не добавить, иногда лучше чтобы все математическое выражение было в одной функции, но опять же не больше экрана.

Отягчающие обстоятельства


А. Множественность. Один раз, как говорится, можно. Ну например срочно нужно было отдать в релиз и быстро чего-нибудь там закомментили, отключили и тд. Но когда, это по всему проекту и тянется не один год, то это — вдвойне криминал.

Б. По предварительному сговору группой лиц. Если вы пишете один, или свой изолированный модуль это одно, но когда несколько человек отвечают за код содержащий выше описанное это — втройне криминал.

P.S. Чтобы не быть ханжой скажу, что у самого иногда в проектах проскакивают признаки состава преступлений из выше приведенных статей, но я никогда не назову такой свой код хорошим. Просто последнее время часто по работе вижу людей, «надувающих щеки» и говорящих, что вот у них код идеальный и 100% профессиональный. А при первом взгляде, когда еще не видно ошибок высокого уровня ( в паттернах там и тд ) вылезает вот это вот все.
Поделиться с друзьями
-->

Комментарии (192)


  1. maxru
    16.03.2017 16:55
    +15

    // нужно проверить на то-то,
    someFunction();

    Минутка рекламы — JetBrains IDE умеют распознавать // TODO:… и составлять из них списки (и напоминать при каждом удобном случае, например, останавливая коммит).
    Подозреваю, что похожим трюкам можно научить и более прочие IDE.


    1. SergeyVoyteshonok
      16.03.2017 17:15
      +1

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


    1. Akon32
      16.03.2017 18:52
      +2

      По крайней мере NetBeans это тоже мог (лет 5-6 назад).
      На базовом уровне достаточно и grep'а.


    1. Tishka17
      16.03.2017 19:05
      +6

      Vim подсвечивает ярким цветом TODO и FIXME в комментариях


      1. potan
        17.03.2017 11:20

        Еще и XXX подсвечивался. Он в FreeBSD с такой целью использовалсяю


    1. PahanMenski
      16.03.2017 19:59
      +5

      И Visual Studio так умеет.
      Хотя в C# еще удобнее использовать директиву #warning, чтобы предупреждения мозолили глаза


      1. GLeBaTi
        17.03.2017 12:38

        https://msdn.microsoft.com/ru-ru/library/txtwdysk.aspx


    1. olegbukatchuk
      16.03.2017 19:59

      Спасибо не знал, пойду поковыряю настройки.


      1. zuwr2t
        17.03.2017 10:13

        В идее ничего не надо настраивать. Если есть коммент со слово «todo» в любом регистре оно подсвечивается, строка добавляется в список туду и при коммите будет показан варнинг.


    1. kafeman
      16.03.2017 22:09
      +3

      Eclipse так на дефолтных настройках делает.


    1. handicraftsman
      16.03.2017 23:52

      YARD позволяет составить список всех @todo тегов.


    1. phoenixweiss
      17.03.2017 00:09

      Даже для Atom есть расширение для подсветки всех TODO / FIXME


    1. S0mbre
      17.03.2017 10:13

      RAD Studio также делает списки из TODO.


      1. Danik-ik
        18.03.2017 18:24

        Ещё бы она не могла, это ещё с древних дельф унаследовано. В седьмых точно есть, раньше — не помню...


  1. Leoon
    16.03.2017 17:20
    -2

    Да, спасибо автору. Очень полезно!


    1. username666
      17.03.2017 17:19

      А чего заминусовали-то??


      1. grossws
        17.03.2017 17:25
        +2

        Бесполезные комментарии часто минусуют


      1. mayorovp
        17.03.2017 17:25
        +1

        Рискну предположить, что потому что "спасибо" принято выражать плюсом.


        1. poxvuibr
          17.03.2017 17:34
          +1

          А как человек плюс поставит, если у него карма отрицательная?


          1. Leoon
            17.03.2017 17:39
            -2

            Да, именно. Пришел человек на сайт. Читает полезные статьи, реально полезные. А оценивать их может только тот, кто здесь очень много времени или по приглашению. Дак им нахер не нужны такие статьи. Существует много англоязычных сайтов, откуда сюда такие же люди переводят статьи, для таких как я. Я и с положительной кармой не мог не чего делать.
            Авторы! Поэтому, если кто то пришел почитать статью вашу, и она реально интересная для развивающегося человека. Он вам плюс не где не поставит. А поставят только те, кто и без вас это знают!
            Минусуют второй раз за благодарность. Мне пох, я буду благодарить. Положил я на эту сраную Карму!


            1. sumanai
              17.03.2017 17:45
              +1

              А оценивать их может только тот, кто здесь очень много времени или по приглашению.

              Достаточно одной не самой лучшей статьи. Вот как у меня.


              1. frnch
                18.03.2017 11:49

                Согласитесь, так себе выход. К функционалу «поблагодарить» напрямую таки не приводит.


          1. mayorovp
            17.03.2017 19:26
            +2

            Отрицательная карма — это наказание для пользователя, а не индульгенция на мусорные комментарии.


      1. Leoon
        17.03.2017 17:34
        +1

        А об этом написали уже кучу статей. Что здесь минусуют просто так. Сайт умирает в плане качества людей. И это заминусуют, потому что это правда. И на минус нельзя ответить. Администации фиолетово на это. Они заняты получением дохода, и не могут допустить, чтоб можно было ответить на то, что тебя неоправданно заминусовали. Это не приоритетная задача.
        Рейтинг, когда то придуманный для полезности человека стал — фиговым листком, которым можно мерятся как длиной органа.
        Благодарность автору в словах считается ненужным комментарием. А обсужение того, что у него там херня написана — плюсуется десятками! Всем спасибо, всем пока!
        Автору еще раз спасибо за полезную информацию, не могу не чего, кроме комента оставить автору.


        1. VMichael
          17.03.2017 18:16
          -3

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


        1. mayorovp
          17.03.2017 19:41
          +10

          Комментарии отнимают у читателя дефицитный ресурс — время. Поэтому комментарии должны содержать информацию.


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


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


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


  1. LoiIver
    16.03.2017 17:20
    +2

    Resharper тоже отлично распознает тудушки :)


  1. sspat
    16.03.2017 17:24
    +3

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


  1. alexeykuzmin0
    16.03.2017 18:01
    +6

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


    1. Ivan22
      17.03.2017 16:22

      идешь к тим лиду и убеждаешь его. Если убедил — он волюнтаристски распространит новые правила на всех. Если не убедил — смирись и жди пока сам станешь ТимЛидом и будешь устанавливать правила для кода.


      1. alexeykuzmin0
        17.03.2017 16:30
        +3

        Я в свое время выбрал третий вариант — поскорее свалить из этого ада. ИМХО, самый эффективный способ решения проблемы.


      1. nckma
        17.03.2017 17:08
        +2

        Самое смешное, что обычно бывает вот так: тим лид заставит всех писать единообразно, например, иерархия наследования, венгерская нотация, правильное число отступов на табуляции, и т.д. и т.п. Потом проходит время мода меняется. Теперь ОН ЖЕ заставляет google code rules, а венгерская нотация запрещена. Наследование c++ — не глубже 2, приветствуются только шаблоны. В код глянуть — это тихий ужас и кошмар: смесь старого и нового кода — это выглядит интересно.
        И ведь известно, что благими намерениями выстлана дорога в ад.


        1. Ivan22
          23.03.2017 11:21

          Безобразно, но однообразно. Армейский принцип, со своими плюсами.
          В команде из 20+ человек вообще неединообразно писать вообще не реально!


  1. Gorniv
    16.03.2017 18:47
    +4

    Чем больше пишу код и изучаю — тем больше ощущение сколько всего еще надо выучить… А на код написанный больше полугода всегда какой-то «нетакой» уже)


  1. tandzan
    16.03.2017 18:47
    +5

    Лживая документация это вообще международный терроризм (привет OpenSSL!). Или давно устаревшая (привет Botan!). В которой примеры написаны с синтаксическими ошибками (опять ты, OpenSSL).
    (Полез в код OpenSSL, выяснить, почему размер IV не совпадает с заявленным, увидел 'return -2', решил не использовать больше ее никогда)


    1. nckma
      17.03.2017 17:12

      Что вы используете вместо OpenSSL?


      1. powerman
        17.03.2017 18:10

        Я когда-то использовал MatrixSSL. Потому что OpenSSL это, таки да, тихий ужас.


      1. tandzan
        17.03.2017 18:53

        Для моей задачи хватило libsodium, остальное реализовываю сам.


  1. zenkz
    16.03.2017 18:48
    +7

    По поводу этих 2х примеров:

    if (true){
    someFunction();
    }

    // нужно проверить на то-то,
    someFunction();

    Первый пример не так бестолков как какжется, потому что вместо someFunction() может быть несколько строк
    кода и проверка нужна на весь блок, поэтому лучше если бы код выглядел так:

    //TODO: Need to check all parameters for null
    if (true){
    someFunction();
    }

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


    1. SergeyVoyteshonok
      16.03.2017 18:53
      +2

      Насчет комментариев это да. Изучая VueJs доставляли комментарии на китайском =)


    1. areht
      16.03.2017 18:56
      +2

      > кто знает что случится с вашим проектом в будущем

      На этот случай есть YAGNI


      1. NickKolok
        17.03.2017 00:23

        PlayOnLinux, разбирали на вузовском OpenSource-кружке, комментарии и имена переменных на французском иногда бывают.


        1. areht
          17.03.2017 07:01
          +1

          Смысл YAGNI не в том, что результат вот совсем никогда не понадобится, а в том, что в 99.97% случаев усилия будут потрачены впустую.

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


    1. jok40
      17.03.2017 12:29

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


      1. acmnu
        17.03.2017 15:14
        +5

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


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


        1. mayorovp
          17.03.2017 15:23
          +1

          Запросто. Чтение на английском и письмо на нем же — это разные навыки.


          1. sumanai
            17.03.2017 16:34

            Вот я не знаю английского. При просмотре фильмов и робких попытках чтения художественной литературы я понимаю, что мои знания не дотягивают даже до знаний современных пятиклассников. Но при этом знаю кучу технических слов и минимум связующих, что позволяет мне написать какое нибудь «Small optimization, remove unused code» в комментарии к коммиту. Да, коряво, но полностью понятно.


            1. mayorovp
              17.03.2017 16:38
              +2

              Одно дело — обобщающий комментарий к коммиту. Другое дело — объясняющий комментарий в коде.


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


              1. sumanai
                17.03.2017 17:44
                +1

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

                Конечно, комментарий в стиле «Constructor» перед методом с именем "__construct" не нужен.
                А вот комментарий «For compatibility with third-party extensions» перед куском кода, который нужен только сторонним дополнениям, которых вообще нет в дереве исходных кодов проекта- вещь полезная.


                1. jok40
                  17.03.2017 18:33
                  +1

                  Вот другой пример:

                  // Стартуем задачу и устанавливаем APN, User ID и password:
                  // формируем строку в три этапа, так как запрос параметра
                  // возвращает результат всегда в одном и том-же буфере, что
                  // не позволяет воспользоваться одной командой sprintf.

                  Или вот:

                  // Таймаут не считаем ошибкой, так как модуль не отвечает
                  // "SEND OK", пока реально не отправит данные на
                  // сервер. Из-за этого таймаут возникает слишком часто -
                  // а в результате мы выполняем совершенно не нужную
                  // переустановку соединения с сервером.

                  Можно было бы напрячься и написать всё это по-английски. А потом, спустя пару лет, можно было бы напрячься, прочитать это и понять. Только вот вопрос — зачем так напрягаться, если этот код кроме меня и моих коллег никто не видел и не увидит?


                  1. ctacka
                    17.03.2017 19:50
                    +2

                    Ну вот мне приходилось читать комментарии на польском и на венгерском. И спасибо google translate, что он сам понял, что это венгерский!


                  1. n01d
                    18.03.2017 21:03
                    -1

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

                    Ещё вариант: чтобы лишний раз не переключать раскладку. И не нужно мне говорить про Punto Switcher — он только мешает, когда пишешь код.


                    1. mayorovp
                      20.03.2017 08:44
                      +1

                      Если человеку проще выразить свои мысли на английском чем переключить раскладку — вопросов нет. Но тут обсуждалась несколько иная ситуация!


                      PS а в портфолио закрытый код демонстрировать не получится в любом случае


        1. jok40
          17.03.2017 16:44

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

          Например, во время стартового тестирования работоспособности DSP-процессора в лог могла вывестись вот такая строка:

          TEST DTMF64 pass... reseting 020202

          Нет, это не то, что Вы подумали. Это вовсе не сообщение о том, что тест пройден. Как раз наоборот — это сообщение о сбое проверки. После его вывода АТС уходила в перезагруз. Обратите, кстати говоря, внимание на слово 'reseting'.

          Спрашивается — зачем было напрягаться и писать по-английски? Эту АТС покупали иностранцы? Вряд-ли.

          Так что всё должно быть в меру. Не нужно впадать в крайности.


          1. acmnu
            17.03.2017 17:03

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


    1. saluev
      17.03.2017 15:03
      +3

      Но можно ведь без if:

      /* TODO: need to check this and that and also that */ { 
          someFunction();
      }
      


  1. sbnur
    16.03.2017 19:57
    +1

    А является ли болезнь криминалом


    1. White_Scorpion
      17.03.2017 16:42

      Если записана в уголовный кодекс — да. История знает примеры. Вплоть до летальных.


      1. sbnur
        17.03.2017 17:50

        а пример привести


        1. White_Scorpion
          17.03.2017 18:48

          Ну как пример относительно недавних событий — гомосексуализм, считавшийся болезнью, в Германии в 30-40 годах прошлого века.


          1. sbnur
            17.03.2017 19:48

            Гоомсексуализм считался в нацистской Германии развратом, а не болезнью


            1. White_Scorpion
              17.03.2017 20:49

              И болезнью тоже.


              1. sbnur
                17.03.2017 21:18

                Ссылку можно на статью —
                Гомосексуализм считался болезнью в Англии и принудительно лечилось (например, лечили Тьюринга), но уголовного преследования не было
                Болезнь не считается преступлением по уголовным кодексам — никто не виноват угловно, что он заболел
                Все-таки уголовное преступление есть посягание на чужую собственность и чужую личность, а болезнь дело личное, но вредные для общества болезни подлежат различным мерам ограничения, но не уголовного характера


                1. White_Scorpion
                  19.03.2017 04:54
                  -1

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


                  1. sbnur
                    19.03.2017 06:42
                    +1

                    думаю вы видите разницу между административным решанием и уголовным — есть адиминстративыные кодексы, есть уголовные — это две большие разницы


                    1. White_Scorpion
                      19.03.2017 18:33

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


                      Повторюсь — не юрист.


                      1. sbnur
                        19.03.2017 18:52

                        просто повторю исходную позицию — чтобы не обсуждать юридические тонкости — болезнь не является преступлением
                        Поэтому заголовок статьи надуман


  1. Gric_Art
    16.03.2017 19:59
    +1

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


  1. mikefrost
    16.03.2017 20:01
    +5

    Вот еще одна статья — разный стиль написания кода в одном проекте.
    А стиль, типа

    function foo()               {
    function bar()               {
    echo "hello"                 ;
                                 }
                                 }
    

    вообще должен влечь за собой максимальную меру


    1. AnROm
      17.03.2017 14:39
      +3

      Я подобный стиль видела только на картинках с приколами, а кто-то в реальных проектах с этим встречался?


      1. Germanets
        17.03.2017 16:36

        Не совсем такие, но подобные запутывающие конструкции, увы, встречать приходилось, к примеру:

        if (a == b)
           doFirstBigFunctionWithMultiplyParameters(a, b, c, d, e, f, g); else
           doSecondBigFunctionWithMultiplyParameters(a, b, c, d, e, f, g); 
        


        if ((a == b) && isParametersChecked(a, b, c, d)) doFirstFunctionWithParameters(a, b, c, d, e); else
           doSecondFunctionWithParameters(a, b, c, d, e, f, g); 
        


  1. ZurgInq
    16.03.2017 20:08
    +2

    4 пункт — вообще лишний, в данной формулировке. Сто раз обсасывалось.
    3 пункт — из крайности в крайность. Стремление сократить копипасту и переиспользовать какой-то однострочник отовсюду — вот лютое зло, ибо плодит зависимости на ровном месте.
    1 и 2 пункт — тут всё верно, хотя и банально.


    1. SergeyVoyteshonok
      16.03.2017 20:36

      По 4-му, вам приятно читать функцию по 3 страницы? Что понятнее прочитать:

      function (){
      //1-ая страница кода
      //2-ая страница кода
      //3-я страница кода
      }
      

      или
      function (){
      делаемРаз();
      делаемДва();
      делаемТри();
      }
      


      по 3-му, я же сказал — не однострочник, а последовательность действий больше 2х операторов (хотя и два тоже иногда надо убрать). Не обязательно добавлять зависимость на другой модуль, если функция используется только тут можно выделить этот код в приватную функцию. Если данный функционал используется в других модулях, то да — добавлять зависимость на другой объект, но это улучшит тестируемость вашего кода.


      1. ZurgInq
        16.03.2017 20:57
        +6

        У вас в статье почему то фигурирует:

        любой повторяющийся код содержащий более двух операторов должен быть убран в функцию

        А тут у нас уже появилось несколько «если». То есть не любой код. И вот про эти «если» многие в порывах чувств забывают.

        Для методов «делаем раз, делаем два» — уже то же написали несколько «если» другие комментаторы. Я могу добавить ещё. Дробление сложного метода на много коротких повышает понимание, ЧТО делает метод. Но к сожалению, часто уменьшается понимание КАК он это делает. В особенности, если итоговая вложенность методов сильно больше двух.

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


        1. SergeyVoyteshonok
          16.03.2017 21:06
          +1

          Для методов «делаем раз, делаем два» — уже то же написали несколько «если» другие комментаторы. Я могу добавить ещё. Дробление сложного метода на много коротких повышает понимание, ЧТО делает метод. Но к сожалению, часто уменьшается понимание КАК он это делает. В особенности, если итоговая вложенность методов сильно больше двух.


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


          1. acmnu
            17.03.2017 15:21
            -1

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


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


      1. Izaron
        16.03.2017 22:09

        У меня есть тулза, где используется простыня из switch, преобразовать ее в нечто красивое не получилось после обдумывания с товарищем. Есть переменная, к которой подсоединяются байты и которую нужно проверить, равняется ли она одной из куче дефайнов (кодов 1-4 байта) и в зависимости от этого что-то сделать. Сделать что-то адекватное для уменьшения простыни не представилось возможным.
        Старик С99 приговаривается к уголовщине из-за отсутствия синтаксического сахара в свитчах. Примеров еще несколько, думаю.
        https://github.com/Izaron/matroska-subtitles/blob/master/matroska.c#L495 (тот пример)


        1. SergeyVoyteshonok
          16.03.2017 23:13
          +2

          Да с С конечно тяжело, но тоже есть варианты.
          У вас там в switch часто повторяется последовательность действий

          read_vint_block_skip(file);
          MATROSKA_SWITCH_BREAK(code, code_len);
          


          и еще несколько

          их можно в функции,
          а коды в массив

          double someCodes1[] = {MATROSKA_SEGMENT_TRACK_FLAG_LACING, MATROSKA_SEGMENT_TRACK_FLAG_FORCED, ...};

          + функция isCodeInArray

          получится что-то типа

          if (isCodeInArray(code, someCodes1))
          yourFunc

          if (isCodeInArray(code, someCodes2))
          yourFunc2

          я думаю размер функций уменьшится в несколько раз
          P.S. в С не силен


      1. Wesha
        17.03.2017 02:24

        Области видимости, ёктель!

        Что приятнее читать:

        function (параметр){
        //1-ая страница кода
        //2-ая страница кода
        //3-я страница кода
        }


        или

        function (параметр_один){
        ...полстраницы кода готовит параметры...
        делаемРаз(параметр_один, параметр_два, параметр_три, параметр_четыре, параметр_пять);
        ...десять строк кода...
        }

        function делаемРаз(параметр_один, параметр_два, параметр_три, параметр_четыре, параметр_пять) {
        ...полстраницы кода готовит параметры...
        делаемДва(параметр_один, параметр_два, параметр_три, параметр_четыре, параметр_пять, параметр_шесть, параметр_семь, параметр_восемь, параметр_девять);
        ...десять строк кода...
        }

        делаемДва(параметр_один, параметр_два, параметр_три, параметр_четыре, параметр_пять, параметр_шесть, параметр_семь, параметр_восемь, параметр_девять, параметр десять) {
        ...десять строк кода...
        }


        1. DnV
          17.03.2017 11:06
          +2

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


        1. a4k
          17.03.2017 12:39
          +2

          Передавать объект или, если параметры однотипны, использовать какой-нибудь список.


        1. Zenitchik
          17.03.2017 13:17
          +3

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


        1. vasiliysenin
          24.03.2017 10:49

          function (параметр){
          //1-ая страница кода
          //2-ая страница кода
          //3-я страница кода
          }

          В таком виде это тоже плохо.
          наверное имелось в виду
          function (параметр){
          //делаемРаз
          //делаемДва
          //делаемТри
          }

          Но лучше
          //Эта функция выполняется в три этапа
          //Раз,Два и Три
          function (параметр){
          //делаемРаз
          страница кода
          //делаемДва
          ещё страница кода
          //делаемТри
          ещё много кода
          }
          

          И ещё очень важный нюанс, комментарий должен выражать намерение программиста (то что код должен делать). А комментарии, объясняющие что код делает, полезны только в учебниках по программированию.


  1. Dreyk
    16.03.2017 20:11
    +3

    if (true) конечно неприятно, но гораздо неприятней if (false) — это эволюционировавший криминал из первого пункта про комментарии. еще доставляет exit() или return посреди скрипта/инклюда (речь о пхп) в начале, или лучше где-то посредине файла


    1. RomanArzumanyan
      17.03.2017 14:47
      +2

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


  1. zenkz
    16.03.2017 20:42

    Самые страшные из этих грехов — огромные методы и множественность.
    Множественность — это самый страшный грех, потому что он делает программу непредсказуемой, а изменения болезненными. Представьте себе простую форму с кнопками, но доступность кнопок меняется в 20 разных методах и в дополнение ко всему ещё и извне модуля. Если программист делая изменения исправит что-то и не заметит хотя бы 1, то мы получаем трудноуловимый баг и кучу головной боли.
    К тому же исправить в одном месте и в 20 — это разные трудозатраты (притом даже не в 20 раз, а больше)

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


    1. Danik-ik
      18.03.2017 19:55

      Не надо представлять, это страшно! У меня такого (именно такого) тридцать две тысячи строк и полсотни форм. На Дельфи 7. Жесть? Ешё не вся жесть, слушайте дальше.
      … С десятками повторяющихся query.sql.add(format(...)[...]); query.sql.add…
      … Комментарии? Не знаем таких
      … Контроль версий? Некогда… Что, можно совместно работать над одним кодом, над одним файлом? Не верим, не получится. Не сольётся, нет…
      … Тесты… Ну да, не забудь прощёлкать форму перед релизом. Табордер проверю!


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


      Но заявление про функции на один экран без уточнения про руководство здравым смыслом и без учёта инструмента и задачи считаю религиозным экстремизмом. Большинство — да. Но сходить с ума за идею я не буду. Не надо путать цель (читаемость) и инструмент (размер). Инструмент надо выбирать с умом и по месту, не так ли?


      1. SergeyVoyteshonok
        18.03.2017 21:16

        Но заявление про функции на один экран без уточнения про руководство здравым смыслом и без учёта инструмента и задачи считаю религиозным экстремизмом.


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


        1. Danik-ik
          19.03.2017 10:51

          «Математическую» — ложное предположение, заведомо ложное. Прикладное программирование отнюдь не ограничивается математикой. И между «можно» и «нужно», насколько мне известно, пропасть. Бывает и можно, и лучше станет, но не настолько же лучше, насколько дороже. И цель (читаемость) !== средству (выносу мозга… Нет, части мозга… Или мозга по частям в отдельные черепа, простите за такое сравнение). А утверждение, что некое правило поведения априори не должно иметь исключений, пусть так делают все, и давайте не будем про здравый смысл — это и есть религиозный экстремизм. Я не утверждаю, что есть методы, которые нельзя безвредно с точки зрения понятности разбить (хотя про sql запросы таки да, утверждаю), и примеров не приведу. Но я утверждаю, что это не всегда оправдано в несферических обстоятельствах, начиная от дефицита ресурсов (время, квалификация и т.п.) и заканчивая тем самым «цена/качество».


  1. Idot
    16.03.2017 20:45
    -2

    Закомментированный код

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

    В принципе неверное решение!
    Комментирование кода используется по причине отсутствия хорошо работающего удобного контроля кода. И удалять такой код — это лечить головную боль топором. В случае необходимости откатиться на одну из предыдущих версий кода, или просто необходимости его посмотреть, будет много заслуженного мата в адрес того кто удалил этот код.
    => правильное решение, это организовать удобный хорошо работающий контроль версий кода. И тогда отпадёт необходимость хранить неактуальный код внутри актуального.


  1. maslyaev
    16.03.2017 20:56
    +8

    Заминусуйте меня до смерти, но я, бывает, предпочитаю не длинные, а очень длинные методы.

    Банальный пример. Допустим, вытаскиваем мы что-то из какой-то внешней системы. Обычная ETL-задачка. Для начала нужно проверить параметры, потом установить соединение, сформировать запрос, получить ответ, распарсить ответ, сопоставить сущности, выполнить кучу разных трансформов, записать данные, запротоколировать. Если вытянуть всё в одну колбасу, получается, например, 5 тыс. кода. Выносим повторяющиеся куски, получаем 2 тыс. строк основного метода плюс 1 тысяча строк вспомогательных. Есть ли смысл в погоне за снижением цикломатической сложности нарезать оставшиеся 2 тыс. на мелкие кусочки, если заведомо известно, что они в качестве самостоятельных единиц точно больше нигде и ни для чего не понадобятся? От слова «никогда»?

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

    Когда мне надо написать «роман в коде», я просто в его начало вписываю описание сюжета, а потом размечаю повествование. Как-то так:

    // 1. Делаем раз
    // 2. Делаем два
    // 3.…

    // (1) Делаем раз


    // (2) Делаем два
    ...
    И нормально. Никто не жаловался.

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


    1. SergeyVoyteshonok
      16.03.2017 21:04
      +2

      Есть ли смысл в погоне за снижением цикломатической сложности нарезать оставшиеся 2 тыс. на мелкие кусочки, если заведомо известно, что они в качестве самостоятельных единиц точно больше нигде и ни для чего не понадобятся? От слова «никогда»?


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


      1. maslyaev
        16.03.2017 21:25
        +6

        Монолит — один предмет тестирования, а сотня его ошмётков — соответственно сотня предметов. Не забываем о рефакторинге. В какой-то момент может оказаться, что ключевую логику (допустим, это 50% объёма кода) можно ускорить на порядок, если сделать не вдоль, а поперёк. Что будем с тестами делать?

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

        Да, понять метод в 2000 строк не просто. Но понять 100 методов по 20 строк — вообще катастрофа. Может быть, отчасти из-за этого и получается, что "разработчик 95% времени читает свой или чужой код и только 5% пишет"?


        1. sspat
          16.03.2017 21:55

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


          1. maslyaev
            16.03.2017 22:21
            +2

            Во многих случаях достаточно покрыть публичные методы.
            А публичный-то метод у нас один. Собственно, сам монолит.

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


            1. sspat
              16.03.2017 22:39
              +4

              Вот с этим обычно вообще беда. Варианты в стиле «Посушить_клиенту_голову_после_стрижки_ежиком_если_он_пришел_по_рекомендации_васи» — форменный кошмар.


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

              Подсушить голову(
                  Проверить Стрижку(
                      Проверить Рекомендацию(
                          Клиент,
                          Вася
                      ),
                      Стрижка Ежиком
                  )
              )
              
              Подсушить голову (Клиент) :void;
              Проверить стрижку (Клиент, Стрижка) :Клиент;
              Проверить рекомендацию(Клиент, Клиент) :Клиент;
              


              1. maslyaev
                17.03.2017 12:04

                Метод внутри метода в случае его заведомо однократного использования не сокращает длину кода внешнего метода. Даже немножко увеличивает. То есть количество страниц останется примерно тем же.
                Но как способ сделать так, чтобы враги не догадались, как оно работает, это определённо можно рекомендовать :))


                1. sspat
                  17.03.2017 12:33
                  +2

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


                  1. maslyaev
                    17.03.2017 20:25

                    Аналогия про оглавление не совсем правильная. В оглавление сводится вся структура, а здесь мы имеем в основном оглавлении только список глав, а список разделов глав — в самих главах, подразделов — в разделах и дальше вглубь. Получается что-то вроде такого:

                    def Книга:
                      Жили были дед да баба
                      ПолучениеЯичка()
                      ЯичкоРазбито()
                      Горе()
                      Утешение()
                    
                    def Обещание:
                      Снесу я вам яичко
                      Не золотое, а простое
                    
                    def УдачнаяПопыткаПолученияРезультата:
                      Мышка бежала
                      Хвостиком махнула
                      Яичко упало и разбилось
                    
                    def ЯичкоРазбито:
                      ПерваяПопыткаПолученияРезультата()
                      УдачнаяПопыткаПолученияРезультата()
                    
                    def Утешение:
                      Не плачь дед, не плачь баба
                      Обещание()
                    
                    def ПолучениеЯичка:
                      Была у них курочка Ряба
                      Снесла она яичко не простое, а золотое
                    
                    def Горе:
                      Плачет дед
                      Плачет баба
                    
                    def ПерваяПопыткаПолученияРезультата:
                      Дед бил, бил, не разбил
                      Баба била, била, не разбила
                    

                    Как понятность? Нормально? Это у нас ещё без циклов и ветвлений. И по модулям не разбросано. И ни одного прекрасного паттерна ООП не применено.
                    Так и живём :(

                    В дополнение к этому нужно понимать, что кроме control flow есть ещё data flow. Курочка инициализируется в 1-й строке функции «ПолучениеЯичка», там же юзается, а потом ещё юзается в функциях «Утешение» и «Обещание». С объектом «Яичко» тоже не всё просто.


                    1. sspat
                      17.03.2017 20:45
                      +1

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

                      Для начала нужно проверить параметры, потом установить соединение, сформировать запрос, получить ответ, распарсить ответ, сопоставить сущности, выполнить кучу разных трансформов, записать данные, запротоколировать. Если вытянуть всё в одну колбасу, получается, например, 5 тыс. кода. Выносим повторяющиеся куски, получаем 2 тыс. строк основного метода...


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


                      1. maslyaev
                        17.03.2017 22:09

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

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

                        Естественно, разбираться в 2000 строчках кода сложнее, чем в 20. Кто бы спорил? Но если 20 умножить на 100, то 2000 не получится. Добавится необходимость помнить, как эта беда складывается в дерево вызовов. То есть помнить, что «Мышка бежала» получает управление через функцию «ЯичкоРазбито», которая стартует после «Снесла она яичко не простое, а золотое» в функции «ПолучениеЯичка» и перед «Не золотое, а простое» в функции «Обещание».


                    1. areht
                      18.03.2017 01:42

                      def Книга:
                        Жили были дед да баба
                        ПолучениеЯичка()
                        ЯичкоРазбито()
                        Горе()
                        Утешение()
                      
                      def ПолучениеЯичка:
                        Была у них курочка Ряба
                        Снесла она яичко не простое, а золотое
                      
                      def ЯичкоРазбито:
                        ПерваяПопыткаПолученияРезультата()
                        УдачнаяПопыткаПолученияРезультата()
                      
                      def ПерваяПопыткаПолученияРезультата:
                        Дед бил, бил, не разбил
                        Баба била, била, не разбила
                      
                      def УдачнаяПопыткаПолученияРезультата:
                        Мышка бежала
                        Хвостиком махнула
                        Яичко упало и разбилось
                      
                      def Горе:
                        Плачет дед
                        Плачет баба
                      
                      def Утешение:
                        Не плачь дед, не плачь баба
                        Обещание()
                      
                      def Обещание:
                        Снесу я вам яичко
                        Не золотое, а простое
                      


                      > Как понятность? Нормально?

                      Да, вполне. Если ещё и методы не называть ПерваяПопыткаПолученияРезультата — ещё лучше будет.

                      > Так и живём :(

                      Преднамеренно запутывая код? )


                      1. Wesha
                        18.03.2017 01:55
                        +1

                        А параметры (Дед, Баба, Мышка, Курочка, Яичко и проч.) в функции кто передавать будет — Пушкин? Или на глобальных переменных всё делать будете?


                        1. areht
                          18.03.2017 10:09

                          Есть варианты. Дед, Баба, Мышка, Курочка — скорее всего, поля класса. Яичко можно передавать.


                      1. VMichael
                        18.03.2017 10:06
                        +1

                        Когда работают с SQL процедурами (и вообще с массивами данных реляционных БД), там портянки в подавляющем большинстве случаев понимать легче, чем разбитие на кучу подпроцедур и функций. Там портянки в 2000 срок — рядовое в общем то явление.
                        Ваш оппонент не зря приводит неоднократно ETL.
                        Что бы осознать это нужно поработать плотно с базами данных в энтерпрайзе.
                        Словом как везде, действовать в русле здравого смысла (хоть он и разный бывает у разных людей.


                        1. areht
                          18.03.2017 11:11
                          +1

                          Лично я, работая плотно с базами данных в энтерпрайзе, таких портянок не видел. Мат начинался строк с 300.

                          А вот чисто на SQL портянки видел, и мне вообще не понравилось. Но на SQL это скорее в силу скудности инструментария.

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


                          1. VMichael
                            18.03.2017 11:15

                            Ну, значит такие были у вас задачи.
                            Если вы чего то не видели, не значит, что этого нет, и что ваш подход единственно правильный в этом мире.
                            Конечно я сюда выкладывать портянки не буду.
                            P/S: Мат в разработке не приветствую, считаю тут нужна холодная голова. А мат это концентрированное выражение мысли и чувства в горячей ситуации.


                            1. areht
                              18.03.2017 14:03
                              +1

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


                              1. VMichael
                                18.03.2017 18:18

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


                      1. maslyaev
                        20.03.2017 18:40

                        Преднамеренно код никто не запутывает. Он сам прекрасно запутывается. А делать то, что вы сделали, то есть выстраивать в порядке исполнения — помогает мало.

                        Мы все на уровне автоматизма знаем, что после того, как отрабатывается последняя строчка функции, управление уходит в вызывающую функцию, а не перескакивает на первую строчку следующей по порядку в тексте. То есть после выполнения «Плачет баба» у нас следующей строчкой будет… что? Смотрим, ищем… ага, после вызова «Горе» у нас вызов функции «Утешение», а там у нас первым делом «Не плачь дед, не плачь баба». То, что эта строчка в тексте модуля сразу после «Плачет баба» — это обязано нам ничего не говорить о порядке выполнения.


                        1. areht
                          20.03.2017 19:12
                          +3

                          > помогает мало

                          Зачем же Вы тогда перемешали, если разницы нет? Сначала хотели запутать, а теперь говорите, что помогает мало?

                          Что ещё мало помогает? Нормально методы называть? Выдерживать стандарты кодирования? Не делать однобуквенных переменных?

                          Если вы не поддерживаете элементарного порядка в коде — конечно у вас всё не понятно.

                          Вы решили, что «обязано нам ничего не говорить» и макароны в коде, а у меня конвенция, и её соблюдение мне «говорит» и делает код предсказуемым.


                    1. SergeyVoyteshonok
                      18.03.2017 12:27
                      +2

                      Господа, что-то мы уехали в какие-то литературные примеры. Дайте вариант реального кода, где реально 4 страницы кода нельзя сделать более читаемыми (а возможно даже меньше по размеру), разбив на функции. Вот мой пример, который я приводил ниже. Я утверждаю, что сейчас там почти ничего не понятно, сделав функцию меньше, например выделив результаты промисов ( код который идет внутри then ) в отдельные функции, читаемость повысится в разы. Приведите реальный пример реальной простыни, которую нельзя разбить.


        1. zenkz
          16.03.2017 23:07
          +3

          С другой стороны найти и отладить ошибку в 20 строках кода легче, чем в 2000 строках кода. Да и не нужно понимать 100 методов, т.к. что они делают должно понятно из названия / комментариев, а как они это делают — это уже проблема самих методов и туда нужно закапываться только если вы поняли что проблема в нём. А вот для 2000 строк кода — нужно держать в голове всю ветку условий и набор параметров.


          1. maslyaev
            17.03.2017 12:22

            Чаще всего, ошибка не в 20 строчках и не в 2000, а в одной. Иногда её проще отладить в длинной портянке, иногда целесообразно вытащить в отдельно отлаживаемый кусок. Если второе, то вынести. Почему бы и нет? Главное — подходить с умом к процессу и не делать фетиш ни из мелкокусочечной нарезки, ни из монолитостроения.

            А вот для 2000 строк кода — нужно держать в голове всю ветку условий и набор параметров.
            А их всё равно держать придётся. Кусок в 20 строк всё равно обретает смысл только в контексте всей задачи. Если задача сама по себе сложна, то нет никакого способа уменьшить сложность её реализации. Да, каждый отдельный маленький метод прост, но выполнение задачи даёт не каждый отдельный метод, а их полный комплекс. Нарезка монолита на 100 кусочков очень не способствует пониманию единого целого во всей полноте.


            1. areht
              17.03.2017 14:58
              +1

              > Нарезка монолита на 100 кусочков очень не способствует пониманию единого целого во всей полноте.

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

              Для «понимания во всей полноте» надо не только свои функции инлайнить, но и, как минимум, функции фреймворка и ОС.


              1. maslyaev
                17.03.2017 22:23

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


                1. areht
                  18.03.2017 02:47

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

                  Выделять абстракции — это навык.


                  1. Danik-ik
                    18.03.2017 20:32

                    А выделять абстракции там, где их нет, ради веры в то, что читаемость === один экран — это уже религиозный экстремизм :).


                    Разные задачи, разные инструменты, разные критерии понятности, да даже экраны у всех разные. А метод всё равно что-то "должен". А я считаю, что раз расписки нет, то и не должен ничего :))


                    1. areht
                      18.03.2017 21:29
                      +3

                      1) А я разве где-то утверждал, что должен?
                      2) Опять же, разве я где-то писал, что «читаемость === один экран»? При прочих равных, один экран прочитать проще, чем два (надеюсь это не надо доказывать?). Дальше вопрос только в том, сможете ли Вы написать в один экран, хватит ли Вам квалификации под конкретную задачу с заданными инструментами. Квалификацию и инструменты мы, теоретически, можем улучшать. Может быть даже можно переформулировать задачу. А можно написать спагетти и сказать «мне так понятнее».
                      3) Может быть Вы покажете как же выглядит понятная простыня без абстракций? А то двое уже слились. И на моей практике такие пространные рассуждения про критерии понятности выливаются в банальный говнокод.

                      Бывает, что написать лучше не получается. Бывает, что простыню на 2 экрана стираешь, заменив на вызов пары методов.
                      Но вот что бы на 2000 строк не нашлось абстракций — не видел.


      1. RomanArzumanyan
        17.03.2017 14:53
        -1

        Не всегда.

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

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


    1. netpilgrim
      16.03.2017 21:58

      Если IDE поддерживает Inline Method и Extract Method рефакторинг не пугает.


      1. maslyaev
        16.03.2017 22:31
        +2

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


        1. areht
          17.03.2017 08:37
          +1

          Зачем монолит переколбашивать 3 раза? Три раза «может оказаться, что ключевую логику можно ускорить на порядок, если сделать не вдоль, а поперёк»?

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

          А уж комментарии в таком монолите безвозвратно устаревают после первого же переписывания


      1. radium
        16.03.2017 22:41
        +3

        Это смотря как пользоваться. На прошлой работе был коллега, который писал длинный метод, а затем бездумно выделял куски кода и делал Extract Method. Получал методы с кучей параметров, из которых половина — out и ref (речь о C#). Поддерживать Это было невозможно.


        1. radium
          16.03.2017 23:12
          +3

          Для наглядности приведу один из реальных кусков такой работы.

                  private void MakeWorkflow(WorkAndRestInputArgumentsBase arguments,
                      IList<DriverActivityFullRecord> records,
                      IList<DriverSpecificConditionRecord> driverSpecificConditionRecords)
                  {
                      List<DriverActivityFullRecord> filteredDublicateDAFR;
                      List<DriverActivityInterval> driverActivityIntervals;
                      IList<TimeRange> ferryOrTrainCrossingIntervals;
                      MakeWorkflow_Part1(arguments as WorkAndRestInputArgumentsEuropean, records, driverSpecificConditionRecords,
                          out filteredDublicateDAFR, out driverActivityIntervals, out ferryOrTrainCrossingIntervals);
          
                      if (driverActivityIntervals.Count <= 0 || filteredDublicateDAFR.Count <= 0)
                          return;
          
                      bool isINSPTransportation;
                      List<TimeRange> weeks;
                      List<TimeRangeWithDriverActivityIntervals> weeksWithActivityIntervals;
                      List<RestTimeInfo> recreations;
                      List<WorkWeekRests> workWeekRests;
                      GenericRange<DateTime> dataRange;
                      MakeWorkflow_Part2(arguments as WorkAndRestInputArgumentsEuropean, driverActivityIntervals,
                                                 filteredDublicateDAFR, ferryOrTrainCrossingIntervals,
                                                 out isINSPTransportation,
                                                 out weeks,
                                                 out weeksWithActivityIntervals,
                                                 out recreations,
                                                 out workWeekRests,
                                                 out dataRange);
          
                  }
          


    1. zenkz
      16.03.2017 22:48
      +2

      Мне кажется, что после того как вы выделили «главы» в своём «романе» — целесообразно вынести каждую «главу» в отдельный метод. Т.е. если даже код не будет вызываться повторно (и будет выполняться строго линейно), то всё-равно удобнее работать с небольшим куском кода, который имеет чёткое предназначение, чем копаться в портянке на 5000 строк кода и мешанине условий и переменных.
      А вообще в большинстве случаев портянка кода говорит о непродуманной архитектуре модуля (возможно нужен какой-нибудь паттерн вроде Visitor-а)


      1. sspat
        17.03.2017 00:10
        +1

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


    1. Bronx
      17.03.2017 08:03
      +3

      Некоторые задачи имеют чётко выраженный характер длинного последовательного нарратива.… Есть ли смысл в погоне за снижением цикломатической сложности нарезать оставшиеся 2 тыс. на мелкие кусочки

      Цикломатическая сложность последовательного (т.е. без циклов и ветвлений) кода минимальна и равна единице, уменьшить её можно только удалив код полностью :)


      1. maslyaev
        17.03.2017 12:29

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


        1. Bronx
          17.03.2017 12:38

          Что вы подразумеваете под «кусочками»? 100 идущих подряд арифметических операций — это 100 кусочков, или один? А 100 последовательных вызовов разных функций? Если каждая функция имеет сложность 1, то 100 последовательных вызовов даст сложность 100, по-вашему?


          1. maslyaev
            17.03.2017 13:13

            Монолит:

            function big_func() { // CC = 5
              for ... {
                for ... {
                  ...
                }
              }
              if ... {
                for ... {
                  ...
                }
              } else {
                ...
              }
            }
            

            Порезали:
            function big_func() { // CC = 1
              small_func1();
              small_func2();
            }
            
            function small_func1() { // CC = 3
              for ... {
                for ... {
                  ...
                }
              }
            }
            
            function small_func2() { // CC = 3
              if ... {
                for ... {
                  ...
                }
              } else {
                ...
              }
            }
            

            Максимальная цикломатическая сложность была 5, а стала 3. Типа профит.


            1. Bronx
              17.03.2017 21:57
              +1

              Во-первых, суммирования тут нет — если бы было суммирование, то получилось бы 3+3=6.

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


    1. Espleth
      17.03.2017 09:09
      +3

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

      Если разбивка такой системы на методы по 20 строк кода делает ваш код менее читабельным, то стоит задуматься, а умеете ли вы писать нормально методы?


      1. maslyaev
        17.03.2017 12:57
        -3

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

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

        В таком случае если тормоза адские, то можно считать, что повезло, потому что сразу заметили и сразу исправили. Но часто бывает, что из-за такой же или подобной пакости снижение скорострельности получается маленькое и на глаз не заметное. Процентов на 10. Потом в схожей ситуации добавится ещё 10, потом ещё и ещё. В результате имеем, что программка, которая раньше летала на Pentium-3 теперь тормозит на Core i7. Хотя функциональности добавлено с гулькин нос. Начинаем смотреть что плохо, и понимаем, что перепахивать нужно всё. А на это уже ни ресурсов, ни желания :(


        1. sspat
          17.03.2017 13:53
          +3

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


          1. maslyaev
            17.03.2017 15:06
            -2

            По-правильному (предположим, изначально так и было сделано) все исходные данные для проверок извлекаются из базы заранее, вне циклов, и передаются вызываемым методам по цепочке вызовов. В конечном счёте выглядит всё вполне невинно: например, в метод прилетают параметры типа ItemID, Quantity, Summ, QExist. Метод сравнивает Quantity c QExist, и если нет превышения, то пускает выполнение по одной ветке, а если есть, то по другой. Предположим, в базу добавили флажок «Запретить отгрузку». Соответственно, если он для ItemID выставлен, то выполнение пускается по «обломной» ветке. Для добычи значения флажка уже есть метод GetStoppedFlagForItemID(), и вызов этого метода мы, не подумав, вписываем в условие. Здравствуй, тормоз.

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

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


            1. mayorovp
              17.03.2017 15:26
              +2

              Само существование GetStoppedFlagForItemID, который лезет в базу за одним флагом — это уже ошибка. Которую очень просто недопустить!


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


              1. maslyaev
                17.03.2017 17:03
                -1

                Само существование GetStoppedFlagForItemID, который лезет в базу за одним флагом — это уже ошибка. Которую очень просто недопустить!
                Не поверите, но бывает так, что такие фокусы бывают частью базового фреймворка. Добавляем в таблицу Item флажок Stopped, и сразу в коде можем писать if (MyItem.Stopped).... Обалденно удобно, если не злоупортеблять.

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


                1. mayorovp
                  17.03.2017 17:06

                  Вот именно. Проверяем заранее загруженный флажок if (MyItem.Stopped) — и никаких лишних запросов к базе! Им просто неоткуда взяться.


                  1. VMichael
                    17.03.2017 18:10

                    Быть может, как вариант, флажок не всегда нужен или редко нужен.
                    Зачем его всегда заранее грузить?


                    1. mayorovp
                      17.03.2017 19:23

                      Затем, что в ситуации "лень разбираться" лучше загрузить по умолчанию все данные сразу, чем делать два запроса.


                      1. maslyaev
                        17.03.2017 20:41
                        -1

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

                        В режиме «fuck cyclomatic complexity delusion» пункты (а) и (б) сильно проще, пункт (в) без разницы, а пункт (г) вообще не нужен.


                        1. mayorovp
                          18.03.2017 10:56
                          +1

                          В режиме «fuck cyclomatic complexity delusion» пункты (а) и (б) сильно проще, пункт (в) без разницы, а пункт (г) вообще не нужен.

                          Не вижу обоснования.


            1. sspat
              17.03.2017 15:31

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

              Про флажок в базе данных не совсем понял.

              Предположим, в базу добавили флажок «Запретить отгрузку». Соответственно, если он для ItemID выставлен, то выполнение пускается по «обломной» ветке. Для добычи значения флажка уже есть метод GetStoppedFlagForItemID(), и вызов этого метода мы, не подумав, вписываем в условие. Здравствуй, тормоз.

              С чего должны возникнуть тормоза? Метод GetStoppedFlagForItemID() лезет в базу чтобы считать одно единственное свойство? Почему оно не считывается и не помещается в обьект Item к которому имеет отношение, когда он создается? А потом просто Item->isStopped()?


              1. maslyaev
                17.03.2017 17:28
                -1

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


                1. areht
                  17.03.2017 20:00
                  +1

                  А как вы в портянке видите, что GetStoppedFlagForItemID вызывать не надо?


                  1. maslyaev
                    17.03.2017 20:50

                    Я не совсем лох, и в момент написания кода мне известно, что этот вызов тянет за собой SQL-запрос. Простой, быстрый, но всё же тянет. И ещё я вижу хотя бы чисто по отступам, что оно внутри трёх вложенных циклов. Ситуация «запрос в цикле» опознаётся автоматически.


                    1. areht
                      17.03.2017 21:17
                      +1

                      > И ещё я вижу хотя бы чисто по отступам, что оно внутри трёх вложенных циклов.

                      Кроме синтаксиса есть семантика. Хотя бы по названию метода GetStoppedFlagForItemID видно, что он, в 99 случаях из 100, будет вызван из цикла.

                      А там уже 2 шага до удаления подобных методов с айтемов под лозунгом aggregate roots. Нет метода — нет проблемы.


                      1. kolu4iy
                        19.03.2017 00:01

                        На sql у меня не будет такого метода. Зато будет запрос по всем item (все равно нужный для работы), к которому joinом прицепится колонка lIsStopped. И упаси меня боже от скалярной функции: это будет именно join или просто колонка из таблицы. Потому — "мальчики, не ссорьтесь" (ц) Мальвина.


                        1. areht
                          19.03.2017 00:55
                          -1

                          Такой метод появляется не на пустом месте, а потому, что флаг лежит, например, в другой БД, и join просто не работает.


                          1. kolu4iy
                            20.03.2017 09:28

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


                            1. areht
                              20.03.2017 12:32
                              -1

                              Флаг в другой БД — это объективное требование, от того, что вы побьете кого-то, join не заработает.


                              1. kolu4iy
                                20.03.2017 14:52
                                +2

                                Отдельная ХП в другой БД, отдающая (оптимизированно!) нужный набор данных. Джойн уже во временную таблицу, в которую сделали insert… exec.
                                Даже если вас съели, у вас есть минимум два выхода.


                                1. areht
                                  20.03.2017 15:43
                                  -1

                                  Если у вас есть доступ в другую БД, можно и так.


                      1. maslyaev
                        20.03.2017 09:55
                        -1

                        На всякий случай: ситуация с ItemID и Stopped — целиком выдуманная. Бывает так, что в функцию, вызываемую в цикле, вклячивается длинный запрос, цепляющий через join десяток таблиц. Так что на Stopped, пожалуйста, не фиксируйтесь.


                        1. areht
                          20.03.2017 13:32
                          +1

                          Вы kolu4iy хотели ответить? Я не сомневался, что бывает. Я сомневался, что с «автоматическим опознаванием» такой ситуации есть сложности.


                        1. kolu4iy
                          20.03.2017 14:55

                          С подобными ситуациями обычно помогает бороться документ, который можно озаглавить, например, «Концепция разработки», с коэффициентами премирования. Обычно людям обидно регулярно получать на 10-15% меньше, чем их коллега, просто потому что они допустили подобный вариант развития событий. Ну и время на рефакторинг, в случае выявления подобных ситуаций, обойдётся дешевле, чем запустить подобною многовложку в продакшн и смотреть как плачет бизнес.


          1. Wesha
            17.03.2017 23:12

            Почему метод проверки сам лезет в базу? Он должен получить все нужные ему для проверки данные на входе

            А что, если лезть в базу за данными для доп. проверки надо только в 0.01% случаев, а запрос к базе занимает 10 секунд? Всё равно делать запрос каждый раз и давать на вход?


            1. sspat
              18.03.2017 10:09
              +1

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


    1. acmnu
      17.03.2017 15:24
      +2

      Думаю проблема при таком подходе не столько в длинне функции, сколько во внутренней связанности. Если в десятом по ходу действии используются переменные объявленные в действиях 1 и 2, становится сложновато дебажить.


      1. mayorovp
        17.03.2017 15:29

        Да ладно дебажить. При таком подходе невозможно без скрола увидеть тип переменной!


        … а некоторые еще любят перед простыней кода на десяток страниц объявить все переменные в начале функции.


        1. alexeykuzmin0
          17.03.2017 16:20

          Ну, невозможность увидеть тип переменной — это не такая уж и проблема, многие считают, что предпочтительно писать type agnostic код.


        1. maslyaev
          17.03.2017 17:56

          Люблю венгерскую нотацию нежно :))


      1. maslyaev
        17.03.2017 17:43
        +3

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

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


        1. acmnu
          17.03.2017 19:34
          +3

          Обычно такой контекст закапывают внутрь класса или в структуру, которую явно передают от блока к блоку.


          1. maslyaev
            17.03.2017 20:57
            -1

            Вовнутрь класса — отстой. Все эти промежуточные данные не должны оседать в состояние объекта.

            Сделать структуру «Контекст» и таскать её с собой повсюду — годное решение. Сам сто раз так делал. Позволяет не плодить методы со списком параметров на три экрана. Жаль только, что в вопросе про происхождение кривого значения это ничем не помогает. Вместо «где же так криво вычислился вот этот параметр» имеем «где же так криво вычислилось вот это свойство контекста».


        1. nporaMep
          18.03.2017 04:52

          «где же так криво вычислился вот этот параметр?»
          это сложно если методы мутируют контекст.

          можно сделать большой метод вот таким:
          var context = new Context();
          context.Prop1 = GetProp1Method(context);
          context.Prop2 = GetProp2Method(context);
          ActOnFullContext(context);

          GetProp1Method(IGetProp1Abstraction input);
          GetProp2Method(IGetProp2Abstraction input);
          public class Context: IGetProp1Abstraction, IGetProp2Abstraction {
          public int Prop1 {get;set;}
          public int Prop2 {get;set;}
          }
          public interface IGetProp1Abstraction {
          object SomethingElse {get;}
          }
          public interface IGetProp2Abstraction {
          int Prop1 {get;}
          }
          public interface IBigContext {
          int Prop1 {get;}
          int Prop2 {get;}
          object SomethingElse {get;}
          }
          }

          Так будет только одно место где делается set Prop1 или set Prop2, а весь инпут гет-онли.


  1. VMichael
    16.03.2017 22:33
    +12

    Любое догматичечкое применение даже правильных вещей — есть зло.
    Жизнь (и программирование в частности) многогранна и не укладывается в набор догм.
    Везде в мега правильных правилах нужно добавлять слова «В большинстве случаев, но бываю и исключения, которых много».


    1. SergeyVoyteshonok
      17.03.2017 14:03

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


  1. CodeName33
    16.03.2017 23:39
    +3

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

    Было так:

    СТАРЫЙ КОД

    Так не надо потому что…


    1. NLO
      17.03.2017 04:31

      НЛО прилетело и опубликовало эту надпись здесь


      1. Gorthauer87
        17.03.2017 10:19

        Git blame, нормальные ide его умеют


        1. NLO
          17.03.2017 11:39

          НЛО прилетело и опубликовало эту надпись здесь


          1. Zenitchik
            19.03.2017 23:05

            Загрузить её из гита? Я, например, в Идее — именно так и поступаю.


            1. NLO
              20.03.2017 03:54

              НЛО прилетело и опубликовало эту надпись здесь


              1. sumanai
                20.03.2017 19:56
                +4

                И 95% времени он там только мешает.


                1. NLO
                  23.03.2017 08:55

                  НЛО прилетело и опубликовало эту надпись здесь


      1. Bringoff
        17.03.2017 13:16

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

        Intellij Idea умеет Git history for selection с контекстного меню, шорткат забиндить, скорее всего, тоже можно.


        1. NLO
          23.03.2017 09:01

          НЛО прилетело и опубликовало эту надпись здесь


      1. mayorovp
        17.03.2017 14:06
        +1

        Visual Studio умеет делать такое на уровне методов. Плюс одна причина держать методы маленькими.


    1. DarkEld3r
      17.03.2017 12:07
      +3

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

      Хм… после/перед исправлением не помешало бы тест написать, так что в будущем вернуться к ошибочному варианту не получится.


      1. NLO
        20.03.2017 03:59

        НЛО прилетело и опубликовало эту надпись здесь


        1. mayorovp
          20.03.2017 08:50
          +3

          Вот именно, "6 — возвращаем код обратно в состояние 1"


          Это и означает — "вернуть ошибочный код не получилось"


          1. NLO
            23.03.2017 08:51

            НЛО прилетело и опубликовало эту надпись здесь


            1. DarkEld3r
              23.03.2017 11:59
              +1

              После пары таких «залётов» разработчик плюнет и просто оставит в комментариях вариант неправильного кода

              У меня в практике именной такой ситуации не случалось, так что может чего-то не понимаю, но разве не может получится, что таких неправильных версий несколько накопится? И что их всех закомментированными держать? Мне всё-таки подход с кратким описанием в духе "делаем так-то потому то и вот эдак делать нельзя иначе..." кажется более удобным и правильным. Тем более, что он лучше масштабируется: если описание разрастается, то его всегда можно перенести куда-то, а в коде оставить ссылку или айди в какой-нибудь джире.


              1. NLO
                23.03.2017 12:27

                НЛО прилетело и опубликовало эту надпись здесь


                1. DarkEld3r
                  23.03.2017 12:44

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

                  Или нет. Потому что не знают о таких инструментах или привыкли делать как раньше и не хотят ничего менять.


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


  1. Roman_Kh
    17.03.2017 07:18
    +2

    git log --pretty=short -u -L 128,135:your-file как раз покажет историю изменения строк 128-135 в файле your-file


  1. ncix
    17.03.2017 10:00
    +9

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


    1. SergeyVoyteshonok
      17.03.2017 14:24
      +1

      Вот-вот. Буквально на днях сидел разбирался в коде passport-saml, функция SAML.prototype.validatePostResponse меня просто убила


  1. nporaMep
    17.03.2017 12:40

    пункты 3 и 4 спорные.

    По пункту 3 согласен насчет копи паста бизнес-логики, типа (если статус = 2 или 3 или статус2 = 5) и такая копи-паста. Поддерживать такое нереально.
    Еще имеет смысл оборачивать внешние зависимости, аля код типа JsonConvert.Serialize/Deserialize («ну наш же проект всю жизнь будет получать на вход json и только json»)
    Но всякую банальную математику или if/else по 3-5 строчек в каждом зачастую только вредит.

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

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

    По пункту 4 — согласен если разбивка на методы делается изначально, аля ты пишешь алгоритм сверху вниз:
    типа
    object DoRequirement1Process(input){
    DoStep1()
    DoStep2()
    DoStep3()
    }
    void DoStep1() { //TODO: implementation }
    void DoStep2() { //TODO: implementation }
    void DoStep3() { //TODO: implementation }

    и потом каждый раз когда какой-нибудь DoStepX не совсем получается имплементировать как планировалось ты сильно задумываешься и переосмысливаешь всю структуру, а не просто вставляешь костыль и модифицируешь сначала шаги алгоритма, а не вытаскиваешь невлазящий кусок имплементации «чтоб работало».
    Придерживаюсь мнения что методы должны быть двух типов: метод-оркестратор (всмысле метод который только вызывает методы других классов), или метод-имплементация (метод который что-то делает и что-то возвращает, но не вызывает ничего сам). Миксовать нельзя, даже если между шагами хочется вставить что-то казалось бы безобидное типа _logger.log() или примитивный foreach внутри которого вызывается 1 метод.

    з.ы. работаю сейчас на старом VB.net/VB6 web service с главным классом в районе 80 000 строк и нескольких сторонних классах по 20-40к строк. Приятного мало. Примерно неделю читал его прежде чем представить как же это поменять, спустя 3 месяца уже могу прикинуть в уме примерно где надо что-то поменять, что не есть гуд.
    С другой стороны в гринфилд сервисе который пишем над этим веб сервисом один товарищ работал по схеме «напишу ка чтоб работало, потом раскидаю по методам и сервисам». В итоге получаются методы и объекты завязанные под конкретную реализацию, нулевая абстракция и какие-то жесткие утилитарные методы оборачивающие примитивные операции типа арифметики и присвоений через рефлексию или линк экспрешны. Смысл непонятен, поменять реализацию невозможно, вся иерархия этих объектов завязана конкретно на json+rabbitmq+webapi и удачи поменять хоть что-нибудь.


  1. debose
    19.03.2017 03:21
    +1

    Длинные легаси методы без тестов. У меня есть аргументы против разбиения ради разбиения:
    1) засорение истории изменений. Пока метод такой какой он есть, через blame можно сразу увидеть зачем писалась та или иная строка. Если же всё было зарефачено ради компактности, то всё что видно в истории — это последний рефакторинг. Когда кто и зачем написал конкретный блок кода — становится совершенно непонятно. Как вариант можно сначала разобраться и перенести это знание в тесты, но объём усилий уходящий на рефакторинг возрастёт на порядок.
    2) Отсутствие юнит тестов. Без них рефакторинг, даже такой безобидный как extract method, превратится в ходьбу по минному полю.


    Но это о том, что касается легаси кода. Продолжать так писать, даже при сопровождении этого кода, это преступление.