Главный принцип хорошего кода

За двадцать лет разнообразного программирования я сформулировал, убежден, главнейший принцип хорошего кода. Опираясь на него, мне и моим коллегам удавалось приводить в порядок самый страшный код, объединять в команде малосовместимых программистов и годами поддерживать системы без лишнего нытья.
Твой план действий:
Прочтение статьи:  15 минут
Осмысление методики:  10 минут
Ощутимые результаты в твоем проекте:  30 минут
Итак,

Зачем?

Что можно сказать о плохом коде?
  • ничего не понятно
  • непонятные переменные
  • огромные методы
  • неочевидные алгоритмы
  • хаки, нарушения инкапсуляции
  • высокая сложность и запутанность
  • изменения приводят к сюрпризам
  • ничего не понятно
I Wake Up Screaming
http://www.flickr.com/photos/69858568@N07/6351235260/
Я недаром дважды указал один и тот же момент. Действительно, практически все сводится к тому, что программист не может разобраться с неким кодом.
Плохой код правится медленно, а изменения приносят неожиданности. Это и понятно: программист не видит, что делает код, и ему приходится разбираться вместо того чтобы создавать что-то новое. Собственно, часто так и бывает, что фиксить программу реально способен только ее автор.
Следовательно, одним из важнейших признаков хорошего кода является его понятность. Понятность же – это понятие сугубо человеческое. Компилятору все равно как что называется, он не вникает в суть; только человек читает код. Только человек может из названия метода представить себе, что именно метод делает. Только человек читает название переменной и сразу видит суть хранимых в ней данных.
Я убежден, что понятность кода в большей степени зависит от названий. От названий классов, методов, переменных. Чем четче сформулированы имена, и чем их больше — тем легче читается код.
Важно!
Все примеры в статье — синтетические. Совершенно нет никакого смысла пытаться мысленно исправлять код, читая примеры. Здесь речь не об этом.
Например, есть вот такой код для отсылки уведомления на мобильное устройство:
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!="", а потом в уме же собрать цепочку условий из результатов. Это происходит автоматом, но нагружает мозг, даже если ты этого не чувствуешь.
Следовательно, нам нужен простой способ писать легкочитаемый код. Желательно — механический. Некая методика, которую может применить и профессиональный и юный программист; пригодная для утра и для вечера, для существующего и для нового кода. Методика, дающая гарантированный результат.
Такая методика существует.

Как?

Все, что можно отделить и назвать — должно быть отделено и названо.
Почти в любом коде можно присмотреться и увидеть кусочки, алгоритмы, принимающие отдельное, вполне самостоятельное решение. Их нужно научиться видеть, выделять и — важнейший момент — давать им понятное имя.
Cut
http://www.flickr.com/photos/circulating/3493082186/
Посмотри еще раз:
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!

Почему?

Это только кажется, что это все слишком просто и тривиально. Отнюдь! Этот принцип гораздо глубже, чем тебе показалось.
Your stupidity
http://www.flickr.com/photos/photonquantique/4139568026/
Во-первых, человек всегда проговаривает внутри себя любой читаемый текст. Именно поэтому очень важно писать такой текст, который можно произнести, последовательный и связный. Текст, который может задействовать один из самых эффективных механизмов мозга: речь. Если текст нельзя пропустить через этот парсер, мозг будет задействовать дополнительные, недостаточно эффективные ресурсы. Отсюда быстрая усталость и головная боль при длительной работе со страшным кодом. Теперь ты понял, да? :)
Во-вторых, мозг по-разному осмысливает стратегические (“что делать?”) и тактические (“как делать?”) задачи. Когда ты пишешь метод 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");
 

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

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

http://www.flickr.com/photos/blimpboy/4051943500/

Как внедрить

Таким рефакторингом можно заниматься в любом коде — он от этого будет становиться гарантированно лучше. Этим можно заниматься в любое время, даже когда голова плохо соображает или устал: ты не меняешь код, ты разделяешь его, это обычно безопасно.
Приступить к этому рефакторингу можно в любой момент без подготовки. Ему можно уделить любой объем времени — даже пяти минут хватит, чтобы что-то изменить к лучшему.
Не стоит бояться длинных имен. Напротив, байты экономить не надо, подробные имена здорово облегчают чтение кода, а набирать их в современном редакторе все равно помогает autocomplete.
Комментарии? Замечания? Прошу на Хабрахабре, в ЖЖ или почтой.
Меня зовут Егор Егоров. Я — техдир и совладелец компании Treebune.net, руководитель проектов и программист с 20+ годами опыта, специалист по системам массового обслуживания и журналист по образованию. Подробнее.