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

в 19: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() {
    if ($device->token != "" && $device->expire <= $now) {
        return true;  // explicitly boolean
    } else { 
        return false;
    }
}

…

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

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

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

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

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

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

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

function isDeviceExpired() {
    if ($this->expireAtUnixtime >= $now) {
        return true;  // explicitly boolean
    } else {
        return false;
    }
}

function isOurTokenValid() {
    if ($token == "") {
        return false;  // explicitly boolean
    } else { 
        return true; 
    }
}

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

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

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

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

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

Почему?

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

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

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

Во-вторых, мозг по-разному осмысливает стратегические (“что делать?”) и тактические (“как делать?”) задачи. Когда ты пишешь метод isDeviceLocalizable(), ты сфокусирован лишь на одной маленькой и четкой задаче: как определить, может ли девайс быть обнаружен или нет. Это — тактическое мышление, предельно внимательное к деталям и весьма техничное. А вот когда ты пишешь “главный” метод 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.

Автор: egorF

Поделиться

* - обязательные к заполнению поля