За двадцать лет разнообразного программирования я сформулировал, убежден, главнейший принцип хорошего кода. Опираясь на него, мне и моим коллегам удавалось приводить в порядок самый страшный код, объединять в команде малосовместимых программистов и годами поддерживать системы без лишнего нытья.
Прочтение этой статьи: 15 минут
Осмысление методики: 10 минут
Ощутимые результаты: 30 минут
Итак,
Зачем?
Что можно сказать о плохом коде?
- ничего не понятно
- непонятные переменные
- огромные методы
- неочевидные алгоритмы
- хаки, нарушения инкапсуляции
- высокая сложность и запутанность
- изменения приводят к сюрпризам
- ничего не понятно
Я недаром дважды указал один и тот же момент. Действительно, практически все сводится к тому, что программист не может разобраться с неким кодом.
Плохой код правится медленно, а изменения приносят неожиданности. Это и понятно: программист не видит, что делает код, и ему приходится разбираться вместо того чтобы создавать что-то новое. Собственно, часто так и бывает, что фиксить программу реально способен только ее автор.
Следовательно, одним из важнейших признаков хорошего кода является его понятность. Понятность же – это понятие сугубо человеческое. Компилятору все равно как что называется, он не вникает в суть; только человек читает код. Только человек может из названия метода представить себе, что именно метод делает. Только человек читает название переменной и сразу видит суть хранимых в ней данных.
Я убежден, что понятность кода в большей степени зависит названий. От названий классов, методов, переменных. Чем четче сформулированы имена, и чем их больше — тем легче читается код.
Например, есть вот такой код для отсылки уведомления на мобильное устройство:
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(). В начале видим тоже самое: в этом методе есть два алгоритма, которые отвечают на совершенно разные вопросы:
- Есть ли у нас токен?
- Не истек ли срок действия?
Обрати внимание: эти вопросы вообще не касаются того, возможно ли найти девайс.
Выделяем методы:
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
