Тестовое задание

    Обнаружил очень необычное тестовое задание, вот его текст:

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

    Внимательно ознакомьтесь с кодом функции.

    Какие потенциальные недочёты и неудобства вы в ней обнаружили?

    Каким образом целесообразнее всего произвести рефакторинг или спроектировать код заново? (если это нужно, на ваш взгляд, сделайте это).»


    <?
    function choose_period($field_name='')
    {
        global 
    $TEXT$PHP_SELF$PERIOD_TO$PERIOD_FROM;

        if (!isset(
    $PERIOD_TO)) $PERIOD_TO = date("d.m.Y",time());
        if (
    $PERIOD_TO) {
         ereg("(.+)\.(.+)\.(.+)", $PERIOD_TO, $r);
         
    $PERIOD_TO=mktime(23,59, 59, $r[2],$r[1],$r[3]);
        }
        if (!isset(
    $PERIOD_FROM)) 
         $PERIOD_FROM = date("d.m.Y"strtotime("-1 month +1day"$PERIOD_TO));
        if (
    $PERIOD_FROM) {
        ereg("(.+)\.(.+)\.(.+)",$PERIOD_FROM,$r); $PERIOD_FROM=mktime(0,0,0,$r[2],$r[1],$r[3]);
        }
        
    $TEXT.="<form action='$PHP_SELF'>".$GLOBALS['FORM']." &nbsp; &nbsp;
        c <input type=text name=PERIOD_FROM value='"
    .($PERIOD_FROM ? date("d.m.Y",$PERIOD_FROM) : "")."' size=12 title='".date("d.m.Y H:i:s",$PERIOD_FROM)."'>
        по <input type=text name=PERIOD_TO value='"
    .($PERIOD_TO ? date("d.m.Y",$PERIOD_TO):"")."' size=12 title='".date("d.m.Y H:i:s",$PERIOD_TO)."'>
        <input class=b type=submit value='&gt;&gt;&gt;'></form>"
    ;
        if (
    $field_name)
        {
            if (
    $PERIOD_TO$WHERE.=" and $field_name<=$PERIOD_TO ";
            if (
    $PERIOD_FROM$WHERE.=" and $field_name>=$PERIOD_FROM ";
        }
        return 
    eregi_replace("^ and "," where ",$WHERE);
    }
    ?>


    А какой вариант рефакторинга предложили бы вы?

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

      +2
      Обратитесь с этим вопрос в Q&A
        +13
        У ТРУшного (целевого) программиста при взгляде на такой код должны встать дыбом волосы и по телу мурашки пройтись. Так их и вычисляют. Всем, кто пытается код рефакторить — отказ. :)
          0
          Что-то вроде «При виде нашего тестового задания программист упал в обморок. Пришлось откачивать Бандой Четырёх и Бучем»? :)
            +1
            Можно распечатать и перед сном вместо ужастиков читать =)
            Автор, разберись, что делает функция, и перепиши с нуля. Другого варианта рефакторинга тут нет.
            +2
            Отличное тестовое задание. Нормальный спец просто переписал бы всё нафиг с нуля, а недокодер попытался бы исправить. В любом случае по результату было бы хорошо видно как мыслит человек и сколько у него опыта.
              –3
              Кстати, как минимум, человек должен будет сказать, что под 5.3 код не запустится, и поэтому пусть дают ему нормальное тестовое задание, которое можно запустить под всеми версяими PHP
                +1
                Это если известно, что код должен делать. В противном случае нормальный спец отрефакторил бы код в процессе его понимания. После чего переписывание может и не понадобится (не знаком с php, потому не знаю какова ситуация в этом случае). Поэтому я бы советовал автору создать svn-репозиторий и коммитить туда после каждого рефакторинга. В случае переписывания функции с нуля, следует в одном коммите добавить комментарий с описанием того, что делает функция, а в следующем уже полностью заменить ее тело. Результат послать нанимателям. Это даст им на много больше информации, чем несколько лаконичных строчек кода, которые окажутся в итоговой программе.
                  0
                  Ну конкретно этот код так просто не отрефакторишь, так как он вместо/помимо параметров использует переменные из глобальной области видимости, причем три на вход, и одну на выход, соответственно если это переписывать через руки — придется изменить не только код функции, но и те места где она вызывается.
                    0
                    Я бы написал об этом в комментариях к коммиту. По моему опыту, изменение способа вызова функции в сотне мест вполне решаемая за час-два задача.
                      0
                      Ну если изменилось только количество параметров — это одно, с глобальными переменными ситуация ведь в корне иная, глобальные переменные могут быть инициализированы не только в вызывающей функции, а в любом другом месте до вызова choose_period(), и выходное значение может быть использовано где угодно.
                      $PERIOD_FROM = '01.01.1970';
                      $PERIOD_TO = '19.01.2038';
                      
                      // где-нибудь там
                      function somefunction() {
                      	// эта функция и понятия не имеет о том что существуют глобальные переменные $PERIOD_FROM, $PERIOD_TO и $TEXT
                      	choose_period();
                      }
                      
                      // десять тысяч строк спустя
                      echo $TEXT;
                      

                      Потому чтобы получить доступ к этим значениям — придется объявление глобальных переменных вынести на уровень выше, а-ля:
                      $PERIOD_FROM = '01.01.1970';
                      $PERIOD_TO = '19.01.2038';
                      
                      function somefunction() {
                      	global $PERIOD_FROM, $PERIOD_TO, $TEXT; 
                      	$TEXT = choose_period($PERIOD_FROM, $PERIOD_TO);
                      }
                      
                      echo $TEXT;
                      

                      Получается шило на мыло, от глобальных переменных в итоге не избавились (для этого придется перековырять весь код где они используются), да и к тому-же есть шанс столкнуться с тем что названные таким образом переменные уже используются в somefunction(), например $TEXT — вполне себе безымянная переменная.
                        0
                        В данном кокретном случае надо r/choose_period/choose_period_new/ а потом r/somefunction/choose_period/
                        И отдельно рефакторить функции и глобальные переменные.

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

                            function choose_period() {
                            	global $PERIOD_FROM, $PERIOD_TO, $TEXT; 
                            	$TEXT = choose_period_new($PERIOD_FROM, $PERIOD_TO);
                            }


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

                            Вот и тянется сей «балласт» многие годы. Иногда, правда, можно заменять старый код на новый по частям, но далеко не всегда можно обеспечить такую «обратную совместимость».
                              0
                              В таком случае не надо делать рефакторинг just for fun. Для него нужно иметь четкую цель — упростить развитие или расширение, решить глобальную проблему типа производительности. И эта цель, обоснованная требованиями бизнеса и найдет и время и возможности. А нет — значить и не надо. Красота кода, сама по себе денег не приносит. Работает, кушать не просит — и нехай пусть живет. Переписывать можно и кусочками в качестве решения проблем, но обойдется это дороже, хоть и размазано по времени.

                              Это все конечно не про открытые проекты для себя :)
                  +3
                  Ну вот каждому соискателю раздашь по одной функции — глядишь рефакторинг и готов :) Хуже-то точно не напишут :)
                    0
                    Бывает пишут и хуже (если не брать в расчёт вышеприведённый код).
                    Пишут как просто плохо, так и ООП ради ООП.

                    Идея раздать по одной функции, конечно, хороша, но т.к. у каждого соискателя свой стиль, свои правила оформления, да если ещё и кода плохого мегабайт 5-10… — потом будет ад в поддержке.
                      0
                      С такими мыслями, уже ничем не поможешь. А рефакторить разными людьми без общей идеи и кооперации — лучше сразу застрелиться.
                      0
                      Это еще что… мне недавно вот такое прислали :-))
                      Я долго думал: они шутят или тут правда надо исправить ВСЕ ошибки: pastebin.ru/314666
                        0
                        В каком бреду (и году) это задание писалось?..
                          0
                          Кстати, отличный вопрос к соискателю. На эрудицию.
                          0
                          Это не необычное, а самое обычное скучное тестовое задание.
                            0
                            я бы отказался такое рефакторить
                              0
                              «Хороший» фаршик. К сожалению, часто приходится видеть такое в старых и серьезных проектах, например, мегафон.

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

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