1. Perl / Говнокод #23249

    0

    1. 01
    2. 02
    3. 03
    4. 04
    5. 05
    6. 06
    7. 07
    8. 08
    9. 09
    10. 10
    11. 11
    12. 12
    13. 13
    14. 14
    15. 15
    16. 16
    17. 17
    18. 18
    19. 19
    20. 20
    21. 21
    22. 22
    if ( $MonitorMode eq \">=\" )
    {
      if ( $NbrProcesses < $ProcNumber )
      {
        $Rule->Status(TRUE);
      }
    }
    elsif ( $MonitorMode eq \"<=\" )
    {
      if ( $NbrProcesses > $ProcNumber )
      {
        $Rule->Status(TRUE);
      }
    }
    else
    {
      if ( $NbrProcesses != $ProcNumber )
      {
        $Session->Value(\"PROCESSMODE\", \"\" );
        $Rule->Status(TRUE);
      }
    };

    Кровавый ентерпрайз. Кусок кода мейд бай ХулетПакард

    Запостил: rjhdby, 09 Августа 2017

    Комментарии (37) RSS

    • в оригинале код был в двойных ковычках заквотен?

      нормальный код. просто и понятно. по нему можно учится как из мух слонов не делать. но я догадываюсь что из простого и понятного большой карьеры не сделаешь, почему этот код и говно. тем более, если его каждый индус может понять и - еще хуже - поменять, то карьера закончится еще скорее. быстрее апгрейдим все на энтерпрайз! https://github.com/EnterpriseQualityCoding/FizzBuzzEnterpriseEdition
      Ответить
      • Этот код говно потому, что простой и понятный if->elsif->elsif превратили в четыре if'a, один elsif и один else до кучи увеличив цикломатическую сложность на пустом месте.

        Да, в кавычках - оно часть конфигурационного файла лежащего в БД
        Ответить
        • > что простой и понятный if->elsif->elsif превратили в четыре if'a

          э? ты про что?

          $MonitorMode это очевидно состояние, а как во всех нормальных fsm первая проверка, это проверка состояния.

          > цикломатическую сложность

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

            У исходного кода она, грубо, 16, а при уходе от внутренних if'ов - уже 6.

            Ну и на счет состояния можно с тем же успехом утверждать, что состояние - это $ProcNumber, а $MonitorMode- это модификатор влияющий на способ проверки этого состояния
            Ответить
            • > Я пользуюсь классическим определением про граф потока исполнения :)

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

              с другой стороны, ты тогда должен радоватся что тут не switch/case. иначе бы сложность была 123123123123123, плюс/минус 1.
              Ответить
              • Да ну ладно, эдак можно долго переливать из пустого в порожнее. Вот вам убийственный аргумент - лично для меня
                вот такой вариант на порядок читабельнее и способствует пониманию происходящего при беглом просмотре, без спотыкания на ненужной лестнице :)

                if ( ($MonitorMode eq ">=") && ($NbrProcesses < $ProcNumber) )
                {
                    $Rule->Status(TRUE);
                }
                elsif ( ($MonitorMode eq "<=") && ($NbrProcesses > $ProcNumber)  )
                {
                    $Rule->Status(TRUE);
                }
                elsif($NbrProcesses != $ProcNumber)
                {
                    $Session->Value(\"PROCESSMODE\", \"\" );
                    $Rule->Status(TRUE);
                };

                Ответить
                • Я тут наконец подумал и понял, что этот код неправильный (как минимум, не эквивалентен тому, что в топике).
                  В этом варианте статус выставляется во всех ветках. В топике если мод >=, но условие на число процессов не выполнено, ничего не произойдёт, т.к. альтернативные ветки не будут просматриваться.
                  Ответить
                  • Это почему же?

                    ($MonitorMode eq ">=") && ($NbrProcesses < $ProcNumber) - это одно логическое выражение, атомарное относительно if. Если оно ЦЕЛИКОМ не выполняется, то управление передается следующему elsif
                    Ответить
                    • > Это почему же?
                      > Если оно ЦЕЛИКОМ не выполняется, то управление передается следующему elsif

                      Вот именно поэтому.
                      Давай проверим на конкретных значениях. Допустим, выполнены условия:
                      $MonitorMode = ">="
                      $NbrProcesses > $ProcNumber

                      В коде из топика мы войдём в первую ветку (if ( $MonitorMode eq \">=\" )), проверим условие ( $NbrProcesses < $ProcNumber ), сочтём его ложным и на этом успокоимся. Остальные условия проверяться не будут, потому что самый первый if уже сработал.

                      В твоём коде мы получим false в первой и во второй ветках, в итоге сработает последняя и выставит Status(TRUE).

                      Видишь, код не эквивалентный.
                      Ответить
                      • Позор на мои седины...

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

                        AS учитывая, что там в коде сначала по умолчанию FALSE устанавливается

                        if ( $MonitorMode eq ">=" )
                        {
                            $Rule->Status($NbrProcesses < $ProcNumber);
                        }
                        elsif ($MonitorMode eq "<=" )
                        {
                            $Rule->Status($NbrProcesses > $ProcNumber);
                        }
                        else
                        {
                            $Session->Value(\"PROCESSMODE\", \"\" );
                            $Rule->Status($NbrProcesses != $ProcNumber);
                        };
                        Ответить
                      • Ну вот, кстати, если логику всего скрипта посмотреть, то последний eslif должен так выглядеть:
                        elsif(($MonitorMode eq "==") && ($NbrProcesses != $ProcNumber))

                        Тогда оно и работать корректно будет и определяем условие явно, что тоже плюс
                        Ответить
                        • А можно не трогать нормальный код неумелыми ручонками, и тоже все работать будет.
                          Ответить
                          • Можно, но они зачем то потрогали, потому и работает через жопу и приходится лезть внутрь и разбираться.
                            Ответить
                        • > ($MonitorMode eq "==")
                          А вот это годный фикс. В оригинальном коде он бы тоже не помешал. А то сиди и думай, что там в else обрабатывается - "==" или "!=".
                          Ответить
      • if ( $NbrProcesses != $ProcNumber ) {
          $Rule->Status(TRUE);
          if ( ($MonitorMode eq ">=") and ($NbrProcesses >= $ProcNumber) or
               ($MonitorMode eq "<=") and ($NbrProcesses <= $ProcNumber) ) {
            $Session->Value("PROCESSMODE", "" );
          }
        }
        Ответить
        • говно. ты fsm сломал. в фсм первым ты проверяшь состояние ($MonitorMode), и в зависимости от состояния, происходят дальнейшие действия.

          ЗЫ ты and/or думаю перепутал. ты хотел &&/||. но вроде у тебя тут это к багу не ведет.
          Ответить
          • да, я затупил.
            Ответить
            • если не как я фсмами увлекаешься, то простимо. и может это я затупил: может у них совсем не фсм? - а $MonitorMode это tied переменная которая при каждом обращении, своп в память зачитывает? или это вообще perl6, и if/elsif/else переопределены, и на самом деле этот код посылает сигнал тостеру новый сандвич сделать? хез.
              Ответить
              • да просто небось плагин какой для nagios, который условия для алармов считает. Я просто поторопился, невнимательно код прочитал, этот вариант более адекватный http://govnokod.ru/23249#comment388949
                Ответить
              • В общем, и я, и автор топика сделали одну и ту же ошибку. Мой код выше это тоже самое, что http://govnokod.ru/23249#comment388949
                Ты прав, код сильно проще не сделаешь.
                Ответить
                • "упростить" примитивный код очень сложно ;)

                  оригинальный код подоходит к правилам "явного программирования" (которые никто никогда конкретно не сформулировал). это когда у тебя на одну строчку одно выражение, и нет никаких/минимальные побочные эффекты.
                  Ответить
    • показать все, что скрытоvanished
      Ответить
    • Говно код в том, что

      1. Проверяется один режим, а проверка, по-факту, выполняется другая. Для ясности перевернул второе условие.

      $MonitorMode eq ">=" (больше или равно)
      $ProcNumber > $NbrProcesses (строго больше)


      $MonitorMode eq "<=" (меньше либо равно)
      $ProcNumber < $NbrProcesses (строго меньше)


      2. Совершенно не ясно, являются ли эти случаи попарно равнозначными

      $MonitorMode eq "==" && $ProcNumber == $NbrProcesses
      $MonitorMode eq "!=" && $ProcNumber == $NbrProcesses


      $MonitorMode eq "==" && $ProcNumber != $NbrProcesses
      $MonitorMode eq "!=" && $ProcNumber != $NbrProcesses
      Ответить
    • В предположении, что "написанному верить" и всё так как и задумано можно сделать так:

      my %check_proc_number = (
      	">=" => [ 0, sub { $NbrProcesses < $ProcNumber; } ],
      	"<=" => [ 0, sub { $NbrProcesses > $ProcNumber; } ],
      );
      my @check_proc_number =
      		( 1, sub { $NbrProcesses != $ProcNumber; } );
      
      my ( $set_proc_mode, $check_proc_number ) =
      	$check_proc_number{$MonitorMode} // @check_proc_number;
      
      $set_proc_mode and $Session->Value("PROCESSMODE", "" );
      $check_proc_number->() and $Rule->Status(TRUE);
      Ответить

    Добавить комментарий