Пользователь
0,0
рейтинг
4 сентября 2012 в 23:03

Разработка → Главный принцип хорошего кода

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

Прочтение этой статьи: 15 минут
Осмысление методики: 10 минут
Ощутимые результаты: 30 минут

Итак,

Зачем?


Что можно сказать о плохом коде?

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

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

I Wake Up Screaming

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

Следовательно, одним из важнейших признаков хорошего кода является его понятность. Понятность же – это понятие сугубо человеческое. Компилятору все равно как что называется, он не вникает в суть; только человек читает код. Только человек может из названия метода представить себе, что именно метод делает. Только человек читает название переменной и сразу видит суть хранимых в ней данных.

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

Например, есть вот такой код для отсылки уведомления на мобильное устройство:

function locator($device) {
    // Can we locate the device?
    if ($device->token != "" && $device->expire <= $now) {
        return false;
    }

    $modelNotifier = new ModelNotifier($device); 
    return $modelNotifier->go();
}

Вроде кажется, что это более-менее чистый код. Но это только так кажется.

  • Каков тип $device?
  • Какой тип у $device->expire и $now? Это точно число или там происходит неявное сравнение дат?
  • Что возвращает $modelNotifier->go()?

Это код из реального проекта. Для того чтобы понять, что на самом деле делает этот код, тебе нужно будет поискать в проекте все упоминания $device->expire и найти место, где оно инициализируется. Тебе нужно будет открыть ModelNotifier->go() и по коду выяснить, что он возвращает. Более того, если ты впервые открываешь этот проект, то ты не знаешь что такое token и что означает в нем пустая строка. На все это ты в лучшем случае потеряешь время, а в худшем случае сломаешь рабочую систему.

Мало того, многоэтажные конструкции условий и вызовы методов без переменной создают когнитивную нагрузку: тебе нужно в уме откомпилировать несколько кусочков кода типа token!="", а потом в уме же собрать цепочку условий из результатов. Это происходит автоматом, но нагружает мозг, даже если ты этого не чувствуешь.

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

Такая методика существует.

Как?


Все, что можно отделить и назвать — должно быть отделено и названо.

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

Посмотри еще раз:

function locator($device) {
    // Can we locate the device?
    if ($device->token != "" && $device->expire <= $now) {
        return false;
    }

    $modelNotifier = new ModelNotifier($device); 
    return $modelNotifier->go();
}

В начале этого метода идет отдельный кусочек кода, который решает отдельную задачу, дает ответ на отдельный вопрос: “можно ли найти девайс?”

Этот код можно отделить и назвать:

function ModelDevice::isLocatable() {
    return ($this->token != "" && $this->expire <= $now);
}

…

function locateAndNotifyDevice(ModelDevice $device) {
    if (!$device->isLocatable()) {
        return false;
    }

    $modelNotifier = new ModelNotifier($device);
    return $modelNotifier->go();
}

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

Идем дальше. Давай посмотрим внутрь isLocatable(). В начале видим тоже самое: в этом методе есть два алгоритма, которые отвечают на совершенно разные вопросы:

  1. Есть ли у нас токен?
  2. Не истек ли срок действия?

Обрати внимание: эти вопросы вообще не касаются того, возможно ли найти девайс.

Выделяем методы:

function isDeviceExpired() {
    return ($this->expireAtUnixtime >= $now);
}

function isOurTokenValid() {
    return ($this->token != "");
}

function isLocatable() {
    return ($this->isOurTokenValid() && !$this->isDeviceExpired());
}

Этот код уже похож на нормальную английскую речь. Его когнитивная нагрузка очень низкая: читая метод isLocatable() ты видишь, какое он принимает решение и фокусируешь свое внимание только на высоком уровне. Читая isDeviceExpired(), ты понимаешь, как это решение принимается и твое внимание сосредоточено на конкретных данных.

Этот код уже не требует анализа — его достаточно лишь только прочитать.

Заодно, кстати, ты узнал что означает, когда token пустой. Обрати внимание, каким элегантным и надежным способом это знание закреплено в проекте: названием метода.

Ах да, заметь: этому коду уже не нужны комментарии. Instant win!

Почему?


Это только кажется, что это все слишком просто и тривиально. Отнюдь! Этот принцип гораздо глубже, чем тебе показалось.

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

Во-вторых, мозг по-разному осмысливает стратегические (“что делать?”) и тактические (“как делать?”) задачи. Когда ты пишешь метод isLocatable(), ты сфокусирован лишь на одной маленькой и четкой задаче: как определить, может ли девайс быть обнаружен или нет. Это — тактическое мышление, предельно внимательное к деталям и весьма техничное. А вот когда ты пишешь “главный” метод locateDevice(), ты не думаешь над мелкими вопросами; ты создаешь, собственно, прикладную логику приложения. Это — стратегическое мышление, оно творческое и мыслит абстракциями.

В-третьих, мозг человека, как и CPU, не может одновременно думать о разных вещах и вынужден между ними переключаться. В отличие от процессора, твой мозг переключает контексты очень медленно — около 1/3 секунды*. Более того, переключение контекста затратно, а потому дискомфортно. Ты этого не осознаешь и не ощущаешь, но подсознательно ты будешь его избегать. Вот почему тебя дико бесит, когда тебе задают левый вопрос в то время, как ты глубоко погружен в код. Вот почему успешными менеджерами становятся люди, которых не напрягает постоянно отвечать по телефону на совершенно разные вопросы.

*300ms — средняя скорость переключения контекста мышления по разным оценкам и исследованиям.

Примеры


Инициализация конфига:

function getConfig() {
    if ($this->hasOption('configs')) {
        $configs = $this->getOption('configs');
    } else if ($this->hasOption('config')) {
        $configs = [ $this->getOption('config') ];
    }

    if (isset($configs)) {
        $root = $this->getOption('root', null);
        if (!$root) {
            if (isset($_SERVER['PWD'])) {
                $root = $_SERVER['PWD'];
            } else if (isset($_SERVER['DOCUMENT_ROOT'])) {
                $root = $_SERVER['DOCUMENT_ROOT'];
            }
        }

        $configs->root = $root;
    }

    return $configs;
}

Два алгоритма: один читает конфигурацию в одном из двух вариантов (множественный и единичный), второй находит корневой каталог проекта. Разделяем и даем имена:

function grabConfiguration() {
    if ($this->hasOption('configs')) {
        return $this->getOption('configs');
    } else if ($this->hasOption('config')) {
        return [ $this->getOption('config') ];
    }
}

function figureOutRootFolder() {
    $root = $this->getOption('root', null);
    if (!$root) {
        $root = $_SERVER['PWD'];
    }
    if (!$root) {
        $root = $_SERVER['DOCUMENT_ROOT'];
    }
    return $root;
}

function getConfigs() {
    $configs = $this->grabConfiguration();  
    if (!is_null($configs)) {
        $configs->rootFolder = $this->figureOutRootFolder();        
    }
    return $configs;
}

Код поиска картинки в мобильном приложении на iOS:

if ([imageEntry objectForKey:@"fullscreenUrl"]==nil) {
    NSString *kind = [imageEntry objectForKey:@"kind"];
    if ([kind isEqualToString:@"pdf"]) {
        UIImage *i = [UIImage imageNamed:@"galleryPdfIcon"];
        [self.uiImagesCache setObject:i forKey:@(photoIndex)];
        *photoSize = NIPhotoScrollViewPhotoSizeOriginal;
        return i;
    } else if ([kind isEqualToString:@"video"]) {
        UIImage *i = [UIImage imageNamed:@"galleryVideoIcon"];
        [self.uiImagesCache setObject:i forKey:@(photoIndex)];
        *photoSize = NIPhotoScrollViewPhotoSizeOriginal;
        return i;
    } else {
        UIImage *i = [UIImage imageNamed:@"galleryBrokenImage"];
        [self.uiImagesCache setObject:i forKey:@(photoIndex)];
        *photoSize = NIPhotoScrollViewPhotoSizeOriginal;
        return i;
    }
}

Если картинки нет, то нужно взять общую картинку. Результат положить в кэш. Разделяем:

- (UIImage *) genericImageForKind:(NSString *) imageKind {
    UIImage *candidate = nil;

    if ([kind isEqualToString:@"pdf"]) {
        candidate = [UIImage imageNamed:@"galleryPdfIcon"];
    } else if ([kind isEqualToString:@"video"]) {
        candidate = [UIImage imageNamed:@"galleryVideoIcon"];
    } else {
        candidate = [UIImage imageNamed:@"galleryBrokenImage"];
    }

    return candidate;
}

…

if ([imageEntry objectForKey:@"fullscreenUrl"]==nil) {
    NSString *kind = [imageEntry objectForKey:@"kind"];
    UIImage *genericImage = [self genericImageForKind:kind];
    if (genericImage) {
        [self.uiImagesCache setObject:genericImage forKey:@(photoIndex)];
        *photoSize = NIPhotoScrollViewPhotoSizeOriginal;
    } 

    return nil;
}

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

function(err, dResults) {
    if (err) {
        console.log(err);
        callback(err);
        return;
    }

    var i=0, lastDownloaded = null;
    var unixtimes = Object.keys(dResults);
    for(i=0;i<unixtimes.length;i++) {
        if (dResults[unixtimes[i]].isDownloaded) { 
            lastDownloaded = unixtimes[i];
        }
    }

    for(i=0;i<unixtimes.length;i++) {
        if (unixtimes[i]>lastDownloaded) {
            delete dResults[unixtimes[i]];
        }
    }

    self.getOpenPrice(symbol, function(err, openPrice) {
        callback(err, dResults, openPrice);
    });
}

Разделяем:

function getLastDownloadedUnixtime(entries) {
    var lastDownloadedUnixtime = null;

    var unixtimes = Object.keys(entries);
    unixtimes.forEach(function(unixtime) {
        if (entries[unixtime].isDownloaded) { 
            lastDownloadedUnixtime = unixtime;
        }
    });

    return lastDownloadedUnixtime;
}

function cleanupEntriesNewerThanLastDownloadedUnixtime(entries, lastDownloadedUnixtime) {
    var unixtimes = Object.keys(entries);
    for(var i=0;i<unixtimes.length;i++) {
        if (unixtimes[i]>lastDownloadedUnixtime) {
            delete entries[unixtimes[i]];
        }
    }
}

…

function(err, dResults) {
    if (err) {
        console.log(err);
        callback(err);
        return;
    }

    var lastDownloadedUnixtime = getLastDownloadedUnixtime(dResults);
    cleanupEntriesNewerThanLastDownloadedUnixtime(dResults, lastDownloadedUnixtime);

    self.getOpenPrice(symbol, function(err, openPrice) {
        callback(err, dResults, openPrice);
    });
}

Собираем XML для беседы с Amazon S3. Поскольку мы точно знаем, какие данные мы туда кладем, то XML создается вручную:

strcat(xml, "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n");
strcat(xml, "<Delete>\n");
strcat(xml, "<Quiet>true</Quiet>\n");

for (uint32_t i=0;i<batchCount;i++) {
    char *escapedPath=Deleter::xmlEscape(batch[i]);
    strcat(xml, "\t<Object><Key>");
    strcat(xml, escapedPath);
    free(escapedPath);
    strcat(xml, "</Key></Object>\n");
    LOG(LOG_DBG, "[Delete] %s", batch[i]);
}

strcat(xml, "</Delete>\n");

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

void addPathToDeleteXml(char *xml, char *path) {
    char *escapedPath=Deleter::xmlEscape(path);
    strcat(xml, "\t<Object><Key>");
    strcat(xml, escapedPath);
    strcat(xml, "</Key></Object>\n");
    free(escapedPath);
}

void addBatchOfPathsToXml(char *xml, char **batch, uint32_t batchCount) {
    for (uint32_t i=0;i<batchCount;i++) {
        addPathToDeleteXml(xml, batch[i]);
        LOG(LOG_DBG, "[Delete] %s", batch[i]);
    }
}

…

strcat(xml, "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n");
strcat(xml, "<Delete>\n");
strcat(xml, "<Quiet>true</Quiet>\n");

addBatchOfPathsToXml(xml, batch, batchCount);

strcat(xml, "</Delete>\n");


Альтернативное мнение


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

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

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

Как внедрить


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

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

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

Update: убрал конструкции вида «if (true) return true» из примеров, т.к. суть статьи не в этом, но каждый считает своим долгом прокомментировать этот момент.
Егор Егоров @egorF
карма
18,5
рейтинг 0,0
Реклама помогает поддерживать и развивать наши сервисы

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

Самое читаемое Разработка

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

  • +21
    Одно и тоже, одно и тоже. Сколько можно обсасывать одну и ту же конфетку.

    1) Разделяй и властвуй.
    2) Самокомментируемый код.
    3) Понимание того, что пишешь.
    • +22
      Повторение — мать заикания. Чем больше статей такого рода, тем чаще они будут попадаться на глаза новичкам и тем кому они нужны. Все новое — хорошо забытое старое. А правила хорошего кода — постоянно забывают.
      • +3
        Не забывают, а игнорируют, что печальнее.
        • 0
          Подтверждаю! Сколько раз своим на работе говорил, что такая техника только на пользу идет. Отмахиваются, мол мне и так хорошо. И, мол, всех слушать — только себе во вред, типа у каждого свои советы, и мол буду делать так, как привык.

          Короче, как я понял, есть творческие программисты, а есть программисты-исполнители. Первые всегда открыты для новых методик, ищут, жаждут, как улучшить качество своего кода. Вторым просто нужно выполнить работу, не важно, каким способом.
          • 0
            Исполнительные как раз по идее должны делать тем способом, которым укажут. Самые «опасные», имхо, творческие, но без критического взгляда на свой код — такого натворят :)
            • 0
              Творческий человек — ведь не обязательно такой, который поток своих творческих же мыслей не может грамотно изложить «на листе». Я имел ввиду, что люди творческие, как правило, готовы на перемены в тактике своего кодирования. А те исполнительные, о которых я писал, — они, конечно, справляются, и да, вы правы в том, что они делают так, как им скажешь. Но ведь не будете вы сидеть подле каждого и науськивать его. Во всяком случае, не у всех есть такая возможность.
              • 0
                Ну, если надо постоянно рядом сидеть, то значит не такой уж исполнительный :)
    • +1
      1. Рефаторинг — это именно то, что проделано в этой статье. Для этого существует замечательная книжка «Рефакторинг» Фаулера.
      2. «Совершенный код» Макконнелла, в которой приведено множество замечательных методик проектирования (и многого другого), чтобы по возможности сразу писать хороший код.
      • 0
        Не совсем так. Для рефакторинга в этом коде не хватает одной, но очень важной вещи — тестового покрытия.
        • 0
          Совершенно согласен.
        • 0
          Есть варианты, когда рефакторинг можно осуществлять и без 100% покрытия тестами.
          • 0
            А как вы гарантируете неизменность поведения (а это основное условие рефакторинга)?
            • 0
              Тривиальностью рефакторинга или тем, что он делается не ручками, а средствами IDE.
              • 0
                Ни то, ни другое не является гарантией.

                (если что, проверено на практике)
                • 0
                  Мне на практике хватает. Иначе рефакторинг унаследованного кода был бы очень-очень затруднён.
                  • 0
                    Вообще-то, в «вашей настольной книге» рекомендуют сначала разорвать зависимости, затем покрыть тестами, а затем рефакторить.
                    • 0
                      Разрыв зависимостей разве не является рефакторингом?
                      • 0
                        Да, был не совсем прав, извиняюсь.

                        Сначала покрыть тестами (интеграционными), потом разорвать зависимости, потом покрыть юнит-тестами, потом рефакторить все остальное.
                        • 0
                          Вот первым этапом я зачастую пренебрегаю, ведь для того чтобы понять что писать в тестах нужно разобраться с кодом, а чтобы разобраться с кодом желательно его привести к читаемому виду. Написать интеграционные/функциональные тесты на god-скрипт в несколько тысяч строк, в один «поток» делающий всё от анализа параметров окружения до работы с БД, проблематично. Приходится, как минимум, выделять функции, а то и рвать зависимости без покрытия тестам, а уж потом писать тесты.
                          • 0
                            Вот и зря пренебрегаете. Потому что тесты пишутся по наблюдаемому поведению (и зачастую не класса, а системы целиком), и разбираться в коде для этого не надо.
                            • 0
                              Про классы речь и не идёт, именно о системе целиком. А без чтения кода очень затруднительно понять возможные варианты поведения. Система многопользовательская, с активным взаимодействием пользователей, кучей откладываемых событий (обрабатываются асинхронно, «он деманд»), которые обрабатываются перед собственно выполнением запроса и от результата (и порядка) обработки которых могут зависеть (а могут и нет) результаты проверки пары десятков взаимозависимых пред- и постусловий действия. В общем возможных исходов у обработки одного запроса могут быть десятки, как минимум. Покрывать для каждого возможного запроса все возможные результаты не выглядит разумным. Лучше сначала разорвать зависимости и тестировать более-менее изолировано.
                              • 0
                                Звучит так, как будто вы вообще эту систему не тестируете. Что, прямо скажем, грустно.
                                • 0
                                  Тестируем то, в чём разобрались. А чтобы разобраться приходится проводить более-менее безопасный рефакторинг без тестов, покрывая ими только изменения логики, но не формы её представления.
                                  • 0
                                    Ну то есть систему — не тестируете (поскольку в самой системе не разобрались). Это и грустно.
                                    • 0
                                      А что делать. Надо сначала её к тестируемому виду привести.
                                      • 0
                                        Покрывать верхнеуровневыми тестами, понятное дело. Это в любом случае надо делать, на самом деле, одними юнит-тестами жив не будешь.
                                        • 0
                                          Замкнутый круг:

                                          — чтобы привести приложение в поддерживаемое (читабельное прежде всего) состояние, нужно покрыть его тестами

                                          — чтобы покрыть тестами нужно знать что идёт на вход (в моём случае: данные http-запроса, состояние php-сессии и БД) и что должно быть на выходе (http-ответ и изменение состояния БД).

                                          — чтобы знать что должно быть на выходе, нужно изучать код (документации никакой)

                                          — чтобы изучать код, нужно чтобы он был читабельный.

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

                                          Когда в довольно большом куске оригинального кода остаются практически только вызовы таких функций (покрытых юнит-тестами) и логика этого куска становится понятна, то пишу интеграционные/функциональные тесты, уже без необходимости тестировать работу «низкоуровневых» ветвлений и циклов, после чего можно как рефакторить, так и изменять логику, без страха не учесть побочный эффект в другом месте. Ну или при получении баг-репорта быстро локализовать проблему и понять где всё-таки умудрился понять логику неверно.
                                          • 0
                                            Да нет никакого замкнутого круга. Если вы вообще не знаете, как себя должна вести система, просто не надо ее трогать.

                                            Если у вас есть необходимость ее чинить или дорабатывать, значит есть понимание того, как она себя ведет (должна вести) в конкретных ситуациях. Вот это и покрывается тестами в первую очередь.
                                            • 0
                                              Нельзя сказать, что совсем не знаем. Общее представление конечно есть, нет понимания алгоритмов, формул, путей прохождения событий и т. п. А часть параметров — текущее время, как в основном коде, так и в SQL, а часть вообще рандомные, то есть даже контрольная копия не поможет для нормального тестирования. Нужно рвать зависимости от rand(), time() и NOW() чтобы было что в тестах сравнивать.
                                              • 0
                                                Общее представление конечно есть, нет понимания алгоритмов, формул, путей прохождения событий и т. п.

                                                А какая разница? Мы же говорим о внешнем наблюдаемом поведении.

                                                Нужно рвать зависимости от rand(), time() и NOW() чтобы было что в тестах сравнивать.

                                                Все, кроме rand(), лечится сравниванием с tolerance.
                                                • 0
                                                  Потому что внешнее поведение случайно и не только числа могут быть разные. Грубо говоря, представьте, что у квадратного уравнения случайный дискриминант: может быть два вещественных корня, может быть один, может быть два комплексных, для каждого свой экран вывода.

                                                  Про tolerance не понял, можно по-русски?
                                                  • 0
                                                    Потому что внешнее поведение случайно

                                                    Что это за система такая, простите? Какую бизнес-задачу она решает?

                                                    Про tolerance не понял, можно по-русски?

                                                    Если у вас есть алгоритм, зависимый от текущего времени, то вы можете проверять, что результат теста отличается от контрольного значения не больше, чем время на момент теста отличается от времени замера контрольного значения. Плюс разбег на границы самого теста.
                                                    • 0
                                                      ММО RTS

                                                      Алгоритм зависит от разницы текущего времени и времени какого-то ранее записанного в базе события. С трудом представляю как это можно тестировать с помощью снимков и логирования запросов и ответов.
                                                      • 0
                                                        ММО RTS

                                                        Там поведение не случайно, а выглядит случайным.

                                                        С трудом представляю как это можно тестировать с помощью снимков и логирования запросов и ответов.

                                                        Конкретно это — нельзя. Здесь нужен дополнительный анализ.
                                                        • 0
                                                          Ну да, выглядит случайным, на самом деле используется генератор псевдослучайных чисел :)
                                      • 0
                                        Покрывать верхнеуровневыми тестами, понятное дело. Это в любом случае надо делать, на самом деле, одними юнит-тестами жив не будешь.
                                  • 0
                                    Ну то есть систему — не тестируете (поскольку в самой системе не разобрались). Это и грустно.
                            • 0
                              Про классы речь и не идёт, именно о системе целиком. А без чтения кода очень затруднительно понять возможные варианты поведения. Система многопользовательская, с активным взаимодействием пользователей, кучей откладываемых событий (обрабатываются асинхронно, «он деманд»), которые обрабатываются перед собственно выполнением запроса и от результата (и порядка) обработки которых могут зависеть (а могут и нет) результаты проверки пары десятков взаимозависимых пред- и постусловий действия. В общем возможных исходов у обработки одного запроса могут быть десятки, как минимум. Покрывать для каждого возможного запроса все возможные результаты не выглядит разумным. Лучше сначала разорвать зависимости и тестировать более-менее изолировано.
                            • 0
                              Про классы речь и не идёт, именно о системе целиком. А без чтения кода очень затруднительно понять возможные варианты поведения. Система многопользовательская, с активным взаимодействием пользователей, кучей откладываемых событий (обрабатываются асинхронно, «он деманд»), которые обрабатываются перед собственно выполнением запроса и от результата (и порядка) обработки которых могут зависеть (а могут и нет) результаты проверки пары десятков взаимозависимых пред- и постусловий действия. В общем возможных исходов у обработки одного запроса могут быть десятки, как минимум. Покрывать для каждого возможного запроса все возможные результаты не выглядит разумным. Лучше сначала разорвать зависимости и тестировать более-менее изолировано.
                            • 0
                              Про классы речь и не идёт, именно о системе целиком. А без чтения кода очень затруднительно понять возможные варианты поведения. Система многопользовательская, с активным взаимодействием пользователей, кучей откладываемых событий (обрабатываются асинхронно, «он деманд»), которые обрабатываются перед собственно выполнением запроса и от результата (и порядка) обработки которых могут зависеть (а могут и нет) результаты проверки пары десятков взаимозависимых пред- и постусловий действия. В общем возможных исходов у обработки одного запроса могут быть десятки, как минимум. Покрывать для каждого возможного запроса все возможные результаты не выглядит разумным. Лучше сначала разорвать зависимости и тестировать более-менее изолировано.
                            • 0
                              Про классы речь и не идёт, именно о системе целиком. А без чтения кода очень затруднительно понять возможные варианты поведения. Система многопользовательская, с активным взаимодействием пользователей, кучей откладываемых событий (обрабатываются асинхронно, «он деманд»), которые обрабатываются перед собственно выполнением запроса и от результата (и порядка) обработки которых могут зависеть (а могут и нет) результаты проверки пары десятков взаимозависимых пред- и постусловий действия. В общем возможных исходов у обработки одного запроса могут быть десятки, как минимум. Покрывать для каждого возможного запроса все возможные результаты не выглядит разумным. Лучше сначала разорвать зависимости и тестировать более-менее изолировано.
                              • 0
                                Блин, хабр глючит :(
                          • 0
                            Кстати, у этой проблемы есть тривиальное решение, называется «контрольная копия» (и сравнение поведения на большой выборке данных). Мы так рефакторим (до сих пор) некий god-класс.
                            • 0
                              В наших условиях малореально: динамично развивающийся апстрим, при этом незначительная нагрузка, чтобы можно было считать, что в копии хотя бы за неделю покроется хотя бы половина кода.
                              • 0
                                Э, а кто вообще занимается рефакторингом меняющегося кода?
                        • 0
                          Вот первым этапом я зачастую пренебрегаю, ведь для того чтобы понять что писать в тестах нужно разобраться с кодом, а чтобы разобраться с кодом желательно его привести к читаемому виду. Написать интеграционные/функциональные тесты на god-скрипт в несколько тысяч строк, в один «поток» делающий всё от анализа параметров окружения до работы с БД, проблематично. Приходится, как минимум, выделять функции, а то и рвать зависимости без покрытия тестам, а уж потом писать тесты.
                        • 0
                          Вот первым этапом я зачастую пренебрегаю, ведь для того чтобы понять что писать в тестах нужно разобраться с кодом, а чтобы разобраться с кодом желательно его привести к читаемому виду. Написать интеграционные/функциональные тесты на god-скрипт в несколько тысяч строк, в один «поток» делающий всё от анализа параметров окружения до работы с БД, проблематично. Приходится, как минимум, выделять функции, а то и рвать зависимости без покрытия тестам, а уж потом писать тесты.
  • +28
    if ($device->token != "" && $device->expire <= $now) {
        return true;
    } else { 
        return false;
    }
    

    FACEPALM.JAR
    • +1
      там ниже написано, почему так.
      • +10
        Что-то я не найду где там написано почему мы не можем написать просто:
        return ($device->token != "" && $device->expire <= $now);
        

        Если ваша методика написания «легкочитаемого» кода предлагает мне писать (и читать) четыре строчки вместо одной, предлагает писать функции на каждый элементарный чих, то, извиняюсь, в топку такую методику.
        • +9
          Возможная причина — так можно на return false поставить breakpoint, чтобы потом разобраться в отладчике, почему это девайс не обнаруживается. Проще, чем вспоминать условные брекпойнты.
        • –9
          Слушай, гений, там принцип демонстируется, вместо return может код быть какой-то, что, обезательно пояснять для особо одарённых
          • 0
            Во-первых, прошу проследовать во вконтактики и прочие не особо отдалённые места рунета, в которых допустима продемонстрированная вами лексика и орфографические ошибки.
            Во-вторых, в статье, претендующий на проповедование культуры кода, такие откровенно индусские участки абсолютно недопустимы.
            В-третьих, статья автора, на мой скромный взгляд, состоит из давно известных истин и приправлена сомнительного качества примерами.
      • +2
        Кстати, именно поэтому я пишу if (…) return true else return false. Можно было бы просто return (…), но тогда код было бы чуть сложнее читать, а возвращаемый тип не был бы мгновенно очевиден с первого взгляда.

        А по названию функции возвращаемый тип не очевиден? Если очевиден, то что стало легче читать?
        • 0
          Если очевиден то, конечно же, не нужно писать такую многоэтажную конструкцию. Это вопрос вкуса. Где-то такая конструкция мешает, где-то напротив, позволяет с одного общего взгляда на код увидеть возвращаемый тип.
      • +8
        Не проще ли так?
        return ($device->token != "") && ($device->expire <= $now);
      • +3
        Странно там написано. Человек не читает слова ни по буквам ни по слогам. В каждом языке (не только ЯП) есть свои конструкции, которые являются его частью и которые обязательно нужно знать. Да и return a == b читается намного быстрее. Доводы о том что это сложно воспринять заставляют вспомнить байку про
        #define BEGIN { #define END }
  • +5
    В итоге все сводится к тому, что
    Здесь нужно выработать вкус и тонкое чувство наития, подсказывающее, где стоит выделять код, а где нет.

    А как выработать вкус и чувство наития в статье не объясняется.
    • 0
      Я в последнее время сильно рвусь между обычными foreach с if внутри и однострочным LINQ :(
    • +2
      А как выработать вкус и чувство наития в статье не объясняется.
      Вкус и чувство наития вырабатывается эмпирически-концептуально, то есть путём естественного обобщения собственного опыта.
  • +3
    Спасибо за пару полезный советов. вооружусь на будущее:)
  • +6
    Мне кажется, тривиальность исходных примеров вызвала непринятие методики. Действительно, из одной строки получили страницу кода — ужас. Но если бы исходная процедура занимала строк 100 кода, наверняка вопросов было бы меньше.
    • +1
      Верно, примеры совершенно синтетические. Но сто строк кода в статью тоже ведь не положишь. :)
      • 0
        У меня в последнее время такое ощущение, что, пока не положишь стопятьсот строк кода, до них не дойдет.
    • 0
      Тут ещё есть пропущенный фактор — удобство написания тестов. Для кода из мелких фрагментов, наверно, тесты писать сильно проще.
      • +2
        Тесты писать проще для изолированных фрагментов, их размер особой роли не играет. Например, в текущем проекте у меня куча функций/методов на 5-10 строк, которые тестами покрыты лишь в составе более крупных. Собственно сначала куски по сотне строк покрывал тестами, а потом эти функции выделял, убеждаясь, что тесты не ломаются.
  • +2
    Теперь девайс отвечает на вопрос, можно ли его найти

    Вот это мне не нравится. Класс девайса не должен реализовывать все мыслимые алгоритмы для работы с девайсом. Стоило бы сделать отдельную функцию.
    • 0
      если эта функция будет требовать для работы private данные класса (а токен и время его истечения к таковым я бы отнес), то это плохая функция
      device->isLocatable() мне кажется более правильным, чем isLocatable(device), т.к. locatability зависит от внутренних свойств экземпляра device, значит логичнее спросить об этом сам экземпляр
      • 0
        Зависит от конкретики — что тут значит токен, как он работает, и т.д.
        Вполне возможно, что в данном случае эту функцию действительно стоит включить в класс. Но в типичном случае лучше подходит отдельная функция.
      • 0
        Это просто первая, самая простая стадия рефакторинга, практически гарантирующая неизменение логики без покрытия тестами.
  • +4
    > Этот код уже не требует анализа — его достаточно лишь только прочитать.

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

    function isLocatable() {
        if ($this->isOurTokenValid() && !$this->isDeviceExpired()) {
            return true;  // explicitly boolean
        } else { 
            return false;
        }
    }
    


    Смотря только на это место, всё равно непонятно, что такое токен, как считается Expire, что означает isOurTokenValid (и зачем там кстати Our? бывают ещё и не our?). Без ресёча глубокого понимания всё равно не будет.

    • –3
      Все правильно. Видите — есть куда стремиться!
    • +14
      И ещё здесь нарушена инкапсуляция логики в объектах.
      Не $this->isOurTokenValid() а $token->isValid().
      Не $this->isDeviceExpired() а $device->isExpired().
      • +1
        У вас так же непонятно что именно истекло! Какое событие было пропущено? Что если появится еще одно событие зависящее от времени? Правильней было бы создать метод isSessionExpired().
    • +1
      Смотря только на это место, всё равно непонятно, что такое токен, как считается Expire, что означает isOurTokenValid

      А зачем вам это знать в данном месте? Здесь хочется знать только то, как определяется, какие девайсы можно найти. При этом правила определения валидности токена могут потом измениться, но (обратите внимание!) критерии находимости девайса не изменятся.
    • +4
      по-моему, «раскиданы» — не проблема, особенно если речь идет о разных уровнях абстракции. Кликнул, углубился, прочитал детали.

      Лично я по этому коду понимаю, что locatable-ность зависит от валидности токена и expire-нутости устройства. Я запомнил, что эти штуки есть, и они как-то считаются. Когда в другом месте встречу «token» вспомню, для чего он еще был нужен. Вобщем, необходимый и достаточный объём сведений для прочтения без переходов по ссылкам.
    • +2
      Это только в примере три получившиеся функции идут одна после другой. В реальном проекте они будут раскиданы по файлам

      Поэтому хорошая IDE так же важна.
      * написал нежно поглаживая F12
  • +4
    Кстати, именно поэтому я пишу if (…) return true else return false. Можно было бы просто return (…), но тогда код было бы чуть сложнее читать, а возвращаемый тип не был бы мгновенно очевиден с первого взгляда.

    return (boolean)(...); или return boolval(...);, имхо, читается всё же проще, даже если boolval($val) нужно определить ручками как { if ($val) return true; else return false; }. На одно минимум переключение контекста меньше, по-моему, и просто DRY и следование вашим же принципам — одним словом описали что выражение делает :).
    • +1
      О сколько нам открытий чудных готовит PHP. ©

      TIL про boolval().
      • +1
        По-моему, это бред. Тогда для консистенции придется везде выставлять return (string), return (array) и т.п.

        Чтобы показать, что метод возвращает boolean есть два более простых способа: префикс «is» и/или "* return boolean".
        • +1
          В своем коде я пользуюсь префиксами «is» и подобными.
        • +2
          Используйте PhpDoc секции, наконец, с описаниями входных параметров и возвращаемых значений, ну, или аналоги в других языках, а не извращайтесь велосипедами. А авто дополнение IDE с такой документацией спасет вам кучу времени.
        • +1
          Есть функции, которые возвращают, например, число или boolean. Анализ их результата выглядит примерно так:
          if ($result === true) {
            echo "Positive infinity";
          } elseif ($result === false) {
            echo "Negative infinity";
          } else {
            echo $result;
          }
          


          Я уж молчу про функции, которые возвращают false или null (а null это отдельный тип) в случае ошибки, а если ок, то число, строку или объект.
          • +4
            Функции, которые возвращают иногда int, а иногда bool — ошибка дизайна. Семантически у функции должен быть четко определенный тип возвращаемого значения.
            • +2
              Увы, но в PHP это сплошь и рядом, начиная со стандартной библиотеки.
              • 0
                Ага, strpos, возвращающий то 0, то false — это гениально.
                Половина встроенных функций PHP выглядит ужасно, это так. Но зачем так писать самому, за исключением случаев, когда по какой-то причине нужно поддерживать [несуществующий] стиль стандартной библиотеки?
                • +1
                  Не знаю как вы, но я воспринял пост как советы о том, как работать с, прежде всего, чьим-то непонятным кодом. Как его приводить к читаемому виду без 100% покрытия тестами на все случаи жизни. Как не трогая логику подготовиться к её более-менее безопасному изменению.
            • 0
              Согласен. А об ошибках можно сообщить с помощью исключений.
    • –1
      не туда %(
  • +2
    А вот серьёзная проблема при работе с унаследованным кодом это, когда выделяешь кусок строк в 5-10 в отдельную функцию и не понимаешь, что этот кусок кода делает. Синтаксис понимаешь, а вот семантику нет. Особенно если это работа с БД вида
    SELECT * FROM table JOIN ... WHERE ... AND NOT ...
    
    через переменные с именами вроде a, b, c, и названиями полей таблицы (которое смотришь в sql-дампе или через
    SHOW TABLE ...
    
    ) на неидентифицируемом языке (в смысле человеческом, а не ЯП, но явно не английский и не русский транслит, только по мылам коммитеров можно догадаться что в проекте кроме английского, может быть немецкий, французкий и русский).
    • +3
      Такое не лечится.
      • +1
        Ещё забыл упомянуть, что результаты запроса хранятся в глобальном ассоциативном массиве с именем типа rows, которому по проекту с сотню присваиваний и с тысячу использований, в основном вида
        foreach ($rows as $row) echo "<li>$row['anz']</li>";
        


        Лечится, но с большим трудом, приходится ручками в базе править данные, чтобы получить возможность выполнить этот кусок и понять что же он делает. Хорошо если с первого раза понятно какие какие данные в БД нужно ввести, чтоб достучаться до него.
    • 0
      Переходный этап: class SomeModel { const FIELD_ASIMPLECOLUMNMYGODWHYMUSTITBESOHARD = 'egyszerű'; }
  • +8
    Подход с написанием

    if ($this->isOurTokenValid() && !$this->isDeviceExpired()) {
    return true; // explicitly boolean
    } else {
    return false;
    }

    хуже, чем return в одну строку сразу по десятку причин:
    1. Больше строк.
    2. Больше сущностей.
    3. Дольше время работы (компилятор может соптимизировать, но тут уж как ему хочется)
    4. Последние 4 строки нужно либо копипастить (а значит легко ошибиться), либо писать руками, что долго
    5. Комментарий напротив «true» о том что это тип boolean — это просто отлично. Для слепоглухонемых и тупых кодеров, видимо?
    6. Хуже понимается — проговорите в уме (вслух, напишите на листочке) слова, которые вы думаете в обоих вариантах, когда читаете код. В многострочном варианте слов больше.
    7. Если так написаны ВСЕ функции — элементарный код будет занимать тысячи строк
    8. В мире С\С++ программирования такой подход вообще засмеют. Там любят лаконичность.
    9. Код имеет две точки выхода, что, при прочих равных, хуже чем одна
    10. Самая главная сложность кода — часть конструкции "() && !$" — шесть спецсимволов подряд. Именно эта часть содержит 95% сложности понимания этого выражения. Вместо разбиения кода на две строки или замены оператора "!" на "== false" автор кода экономит на спичках и занимается какой-то фигней.

    Думаю, смог бы написать еще десяток причин при желании.
    • 0
      Ну все, проапдейтил статью и примеры.
      • +3
        и ведь лучше стало
        • +1
          Yep.
          • 0
            Сам пишу в стиле `return $foo && !$bar`, но вот честно, для уставшего слегка поддатого мозга начальный вариант читался понятнее.
            • 0
              Дело привычки. У себя в коде нашел подобие "() && !$" и как-то не задумывался раньше, что эту запись можно привести к более читабельному виду. Видимо ошибался.
    • 0
      > 9. Код имеет две точки выхода, что, при прочих равных, хуже чем одна

      Ложь.
  • +4
    Да, и картинка должна быть вот эта:

    В этой книге большинство приёмов рефакторинга описано, в т.ч и этот.
    • +2
      Моя настольная книга:
      image
      • 0
        О, спасибо за ссылку.
  • +2
    function grabConfiguration() {
        if ($this->hasOption('configs')) {
            return $this->getOption('configs');
        } else if ($this->hasOption('config')) {
            return [ $this->getOption('config') ];
        }
    }
    

    Макконнелл среди прочего, говорил о том, что когда пишешь функцию, нужно мысленно пробежаться по всем вариантам ее работы, зайти в каждый if, в каждый else… Короче, вот что будет если и configs и config отсутствуют? Что будет с таким кодом:

    $configs = grabConfiguration();
    $configs->get("lol");
    

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

    P. S. Надеюсь моих скудных знаний php хватило, чтобы не ошибочно указать на ошибку.
    • 0
      Прошу прощения, ошибочно указал на ошибку. Невнимательность виной тому, а не скудность знаний.
      • 0
        А еще невнимательность является виной предыдущему комментарию. Я умудрился запутаться в вашем простом коде, там явно не хватает
        else {
           return null;
        }
        

        И вопрос на тему остается открытым, только не grabConfiguration, а getConfig.

        P. S. Код с конфигурациями, вне контекста того где он был, плохой и до правок и после. Совсем не понятны сути тех сущностей.
        • +1
          php > function a() {};
          php > var_dump(a());
          NULL
          php > 
          

          • +1
            Я подозревал что-то такое, но автор говорил о явности, удобочитаемости…
            • +3
              С этой точки зрения да, формально нужно else return null; написать. С другой стороны даже python, где принцип «явное лучше неявного» возведён чуть ли не в абсолют, выводит
              >>> def a():
              ...   pass
              ... 
              >>> print(a())
              None
              >>> 
              

              Имхо, return null нужно писать, если функция подразумевает возврат значения (не процедура не терминах Паскаль), но этот return не последний значимый оператор. Если последний, то можно опустить и null, и сам return.

              А если уж всё явно описывать то функция должна быть такой:
              function grabConfiguration() {
                  if ($this->hasOption('configs')) {
                      $configs = $this->getOption('configs');
                  } else if ($this->hasOption('config')) {
                      $configs = [ $this->getOption('config') ];
                  } else {
                      $configs = null;
                  }
                  return $configs;
              }
              
              • 0
                А если прикинуть, что это реальная задача, то в случае отсутствия конфигов, скорее, exception надо кидать.
                • 0
                  Это синтетический пример.
                  • 0
                    Это понятно, поэтому я это не к самому топику написал, а к рассуждениям коллег: если уж доделывать до конца, то не return null, a throw. Добавил, так сказать, свою лепту занудства =)
                • 0
                  Ну я бы не throw делал, а вернул бы в initialize false).
                • 0
                  Я бы бросал исключение уровнем выше, если бы вообще бросал. А вместо null возвращал бы пустой массив, а там пускай приложение думает, что делать если конфигов нет, исключительная ситуация или можно, например захардкоженные дефолтные данные загрузить.
    • –1
      Макконнелл возможно, в чем-то, и прав. Я согласен делать это только подсознательно.
      Тесты с этим справляются эффективнее. Да и TDD пока никто не отменил.
  • +6
    Обычно методы с 'and' в названии — это code smell. Я бы сделал $device->notify(). Функция сама «достает» из device все данные, необходимые для уведомления. Для этого на может вызывать locate() или не вызывать, это деталь реализации, которой незачем торчать наружу.

    Насчет главной идеи поста — выработал такую эвристику: я обычно разрабатываю снизу вверх, и когда у меня есть минимальный кусок кода, который можно тестировать независимо от всего остального — то выношу в отдельную функцию, даже если я не планировал ее создавать, и она не нужна в финальном API. Таким образом получается писать по 300-400 строк кода без багов. Запускаешь программу — и она просто работает. Вторая эвристика — не выносить дублирующийся код сразу, а подождать, пока наберется несколько похожих вариаций. Часто оказывается, что есть лучший способ убрать дублирование, чем я предполагал изначально, и предждевременная попытка это сделать не дала бы мне увидеть такую возможность. И третья эвристика — не разбивать методы исключительно ради разбивки. Часто один длинный метод читается лучше и понятнее, чем тот же метод разбитый на несколько маленьких. В оличие от автора поста я выношу что-то, только если оно начинает мозолить глаза и мешать.

    Искал сейчас источник, но не нашел, исследование показывало, что оптимальный размер метода — от 30 до 50 строк. Размер больше 120 строк коррелирует с большим количеством багов, а множество мелких методов по 5 строк делает программу более сложной для понимания и поддержки.
    • 0
      Насколько я понял, основные тезисы поста о том, как улучшить код, который непонятен, чужой или, внезапно, свой :)
    • +2
      Методы с «and» в названии — это привет из культуры Objective C.
    • 0
      Интересно, это исследование было для какого-то конкретного языка?
      • 0
        ага, brainfuck
  • 0
    Я думаю имеет смысл разделять код на отдельные методы, только если есть необходимость их повторного использования, в ином случае больше минусов, чем плюсов.
    • +1
      Не согласен. Надо выделять отдельные методы там, где они делают какое-то конкретное отдельное действие. Совершенно неважно, будет ли при этом метод вызываться несколько раз, или нет.
      Более того, если (случайно) одинаковые 5 строк кода встречаются в программе дважды — это еще не повод выносить их в отдельный метод, если у них разная семантика.
      • 0
        Иногда от семантики можно абстрагироваться, например используя приёмы ФП. Хотя в некоторых языках это только ухудшит читаемость.
        • 0
          Вроде бы основной идеей ФП является то, что программа вычисляет некоторую функцию, которая представляется в виде композиции других функций. И в нормальном коде каждая функция должна иметь некоторый самостоятельный смысл.
          • 0
            Я ФВП имел в виду, прежде всего. Например при императивной обработке часто встречается выполнение каких-то действий над элементами массива в цикле, в двух местх может быть такой синтаксически идентичный код, но семантический разный. А с помощью ФВП мы может отделить собственно итерирование массива от действий, передавая функцию с действием параметром в функцию-цикл.
            • 0
              Ну, называть всё, использующее map, функциональным программированием — ИМХО, немного перебор. Хотя, конечно, это элемент оттуда.
              Но даже в этом случае ИМХО лучше сделать 2 синтаксически одинаковых функции с разными названиями. Очень вряд ли такие куски кода будут длинными, и нет никакого повода надеяться на то, что и в будущем они будут одинаковы.
              • 0
                Не только map — reduce, fiter и т. п., а главное более сложные действия.
                • 0
                  В любом случае. В императивном программировании основной единицей является переменная, в функциональном — функция. И там, и там каждый элемент должен иметь некоторый смысл.
    • 0
      Объективный минус — снижение быстродействия и повышение потребления памяти, но обычно пренебрежимое. А так разделение на отдельные функции/процедуры читаемость повышает при условии правильной декомпозиции и хороших имён функций. Конечно если писать в стиле $a1 = func(1); $a2 = func2($a1); то читаемость скорее снизится.
  • +1
    cleanupEntriesNewerThanLastDownloadedUnixtime — омг, на этом я сломал моск. Поэтому для внутренних имен обычно использую highly_readable_lower_case_with_undercores
  • +1
    cleanupEntriesNewerThanLastDownloadedUnixtime
    За таки названия методов извините, надо от церкви компа отлучать.
  • +3
    cleanupEntriesNewerThanLastDownloadedUnixtime
    Осталось писать код в названии метода!
    • 0
      См. комментарий прямо сразу над вашим :)
  • +2
    Вместо выноса в отдельные функции для первого способа я бы предпочел вынос в переменные:

    $is_valid_device = $device->token != "";
    $device_expired = $device->expire <= $now;
    
    $is_locatable = $is_valid_device && $device_expired;
    
    return $is_locatable;
    

    Тем более отлаживать это проще и выводить в лог переменные.
    • 0
      А потом где-то забыть что существует такая переменная и забыть ее обновить — и привет, многочасовой дебаг.
      • +2
        Какая разница, если это локальная переменная?
    • +1
      Спорное решение, по-моему, хотя лучше, чем оригинал :)
      • +1
        Я взял это решение в книге Макконнелла «Совершенный код», оно там предлагалось для упрощения понимания сложных условий.
    • –2
      А чем проще?

      У методов есть два плюса:
      1) их легче покрыть автоматическими тестами, при необходимости
      2) их можно использовать повторно
      • 0
        Я говорю про конкретный пример, в котором условия очень простые, что тестами их покрывать даже не нужно. Про второй пункт согласен. Если чуть сложнее или повторно используется, то, конечно, в отдельный метод.
        • +1
          Хе-хе, для вас условия то простые, а тесты автору бы очень помогли. Почему? На первую ошибку уже указал relgames. Вторая ошибка в конечном isDeviceExpired — условие инвертировано неправильно. В исходном примере если текущая дата === expired, то device считается expired. В конечном — еще нет. Мелочь, а поведение после изменения изменилось. Это также подверждение того, что без тестов это не рефакторинг.
          • 0
            Примеры здесь приведены синтетические.
      • 0
        То есть, если мы хотим только читаемость, то переменные вполне подходят для простых случаев.
  • 0
    Прошу прощения, что лезу с ламерским вопросом, но мне кажется, что в коде нужно подняться выше и реализовать getOption так, чтобы оставить config, а если нужно разделять единичный и множественный, добавить опцию типа multiple config. Тогда можно будет написать
    function getConfigs() {
       if $this->hasOption ('config') {
            $configs = getOption ('config);
            $configs->rootFolder = $this->figureOutRootFolder();        
        }
        return $configs;
    

    Нет?
  • +1
    «Есть такое правило: выносить в отдельные методы только тот код, который может использоваться где-то еще.»
    — да, такие «рассуждения» часто встречаются, причем на вопрос «почему такое правило? почему нельзя выносить даже код, использующийся один раз, если это облегчает читаемость?» ответить обычно не могут.

    «Если выносить все методы, то тогда возрастает когнитивная нагрузка из-за того, что программист вынужден будет ездить по файлу вверх-вниз в поисках нужного метода, вместо того, чтобы видеть все перед глазами.»
    — для этого есть контраргументы, первый из которых и приведен в статье: вынос как раз снижает нагрузку, потому что 1) гораздо быстрее въезжаешь в верхнеуровневый смысл кода; 2) не всегда нужно смотреть методы нижнего уровня; 3) даже если посмотрел внутренний метод один раз, больше он тебе не нужен и не мешает, а если не выносить в метод, этот код все время будет перед глазами
    • +3
      почему нельзя выносить даже код, использующийся один раз, если это облегчает читаемость?» ответить обычно не могут.

      Это же увеличение исходного кода на N байт, увеличение времени исполнения на M наносекунд и увеличение потребление памяти на L байт!1!1!!!11!!!
      • +2
        M наносекунд на каждом вызове. И в итоге пользователь, не дождавшись ответа за 15 секунд, идет кормить виртуальную свинью. А потом жалуется, что на работу три часа ушло.
        • 0
          Если Ваш код тормозит из-за операций вызова функций — то либо напишите его нормально, либо смените компилятор или язык.
          • 0
            А еще можно вставить перед функцией inline (если это C++). Или включить оптимизацию. Или посоветовать пользователю купить компьютер пошустрее. А если все это не поможет — заняться другим проектом, где такого быстродействия не нужно.
            • 0
              А еще компилятор может забить на inline, и даже на force inline… Остаются только макросы)
  • +1
    Очень порадовало «figureOutRootFolder».
    Жду реализацию с использованием исскуственного интелекта и методами figureOutWhatToDoNextAndDoIt…
    • 0
      А зачем тут искусственный интеллект? Такое должен уметь любой make, в сущности, в этом его работа и заключается. Разобраться в ситуации, посмотреть, что еще не сделано, выбрать из этого, что надо сделать в первую очередь — и сделать.
  • 0
    function isOurTokenValid() { return ($this->token == ""); }

    Должно быть $this->token != "", разве нет?
    • 0
      Верно, спасибо. Исправил.
  • +1
    По теме — абсолютно согласен с автором. Очень сложно читать даже свой код через пару лет, что уж говорить про чужой.
  • +1
    function locator($device) {
        // Can we locate the device?
        if ($device->token != "" && $device->expire <= $now) {
            return false;
        }
    
        $modelNotifier = new ModelNotifier($device); 
        return $modelNotifier->go();
    }
    


    Каков тип $device?

    А какая разница?

    Какой тип у $device->expire и $now?

    А какая разница?

    Это точно число или там происходит неявное сравнение дат?

    А какая разница?

    Что возвращает $modelNotifier->go()?

    О, добрались до первой реальной проблемы. go(), очевидно, не должен возвращать ничего, потому что из названия метода никак не вытекает, что он должен что-то возвращать.

    То же относится и к самой функции locator. Локатор — это объект, а не действие, функция так называться не может.

    При этом, в принципе, подход «код должен выражать намерение, а не алгоритм» верен. Но базовые посылки потрясают.
  • +2
    > cleanupEntriesNewerThanLastDownloadedUnixtime

    Мои глаза!!!
    • 0
      Не программировать вам под iOS/Cocoa с такой болью. :))
  • +2
    Вы изобрели SLAP — Single Level of Abstraction Principle :)

    BTW, очень хороший принцип программирования. В интернете по этому ключевому слово много статей, обзоров, примеров для разных языков.\
  • –1
    ничего не понятно
    непонятные переменные
    огромные методы
    неочевидные алгоритмы
    хаки, нарушения инкапсуляции
    высокая сложность и запутанность
    изменения приводят к сюрпризам
    ничего не понятно

    Вы забыли про дублирование
  • 0
    «Чистый код», Роберт Мартин. Глава вторая «Содержательные имена». Автор хорошо расписал тему правильного именования, и для чего это нужно.
  • +2
    Начало статьи (где говорится про универсальную механическую методику для программистов любой квалификации) противоречит ее же финалу (где уже начинает требоваться мудрость).

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

    Я такой код видел пару раз. Заходишь в метод — там if-else ветки и вызов других методов, и больше практически никакого кода нет. Заходишь в эти методы — там то же самое. И так рекурсивно на 5-6 (иногда и 10) уровней вниз. Где-то там после 6 уровня вложенности метод делает что-то типа вашего:
    return ($this->token != "");

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

    В общем, не зная меры, такой методикой можно сделать код только хуже.
    • +1
      Любой методикой без вкуса можно сделать хуже. Но в целом да, вы правы, здесь перегибать палку не стоит.
  • +1
    Когда начал читать статью не ожидал узнать что-либо полезное. Но сейчас уверен, что как только следующий раз сяду за код, начну уделять больше внимания названиям переменных и функций. Спасибо.
  • +1
    Я бы написал так:
    function locator($device) {
        if ($device->token != "" && $device->expire <= $now)
            return false;
        else
            return new ModelNotifier($device)->go();
    }
    
    • –1
      А потом вы как-нибудь с бодуна добавите строчку перед первым return, не обратив внимания что там нет блока, и будете затем ловить багу полчаса. А потом хлопнете себя по лбу и пойдете пить кофе.

      Я запрещаю в своих проектах использовать короткие записи if/for без блоков.
      • 0
        Не вам же баги по полчаса искать придется. А если кому-то так удобнее писать, то зачем запрещать.
        • +1
          Важно не то, как кому удобно писать, а как кому потом будет удобно читать и развивать код. Вообще-то об этом человеколюбии вся статья.
          • 0
            Так без скобочек читается же лучше.
            • 0
              Зато оставляет здоровенное место для обыкновенной человеческой ошибки.
              • 0
                В таких случаях можно ещё сделать двойной отступ.
  • 0
    А еще можно прочитать про PSR-0, PSR-1, PSR-2 стандарты оформления кода в случае PHP.

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