16.03.2021 08:44
antoshkka 69 16900 Источник


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

Я расскажу о подходах, которые мы используем в Яндекс.Такси для написания читаемого кода на C++, Python, JavaScript и других языках.

Обычный рабочий процесс


Допустим, вы работаете в компании, пишете код. Написали функцию, начинаете обкладывать её тестами и понимаете, что что-то глючит. Ну что ж, отлаживаем… Оказывается, что плохо работает не ваш код, а функция sample от другого разработчика, которую вы используете.

Выглядит функция sample как-то так:

std::string sample(int d, std::string (*cb)(int, std::string)) {
  // Getting new descriptors from d with a timeout
  auto result = get(d, 1000);

  if (result != -13) {
    // Descriptor is fine, processing with non bulk options
    auto data = process(result, true, false, true);

    // Passing processing result to the callback
    return cb(data.second.first, data.second.second);
  }

  // Notifying callback on error
  return cb(result, {});
}

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

Нам повезёт, мы сможем исправить функцию, не разобравшись до конца в её работе.
Начинаем читать первый комментарий, где написано: получаем какие-то новые дескрипторы из d с заданным timeout, d — дескриптор. То есть часть разбросанных по коду int на самом деле не int — они относятся к отдельному классу данных.

Решаем заменить часть int на отдельный тип данных Descriptor в надежде, что компилятор сам найдёт ошибки и нам не придётся дальше отлаживать код. Зовём автора кода, просим его подсказать, где дескрипторы, а где числа. Он сейчас работает над другим проектом, но после долгих уговоров неохотно помогает и быстро ретируется:

enum class Descriptor : int {};

std::string sample(Descriptor d, std::string (*cb)(Descriptor, std::string)) {
  // Getting new descriptors from d with a timeout
  auto result = get(d, 1000);

  if (result != Descriptor(-13)) {
    // Descriptor is fine, processing with non bulk options
    auto data = process(result, true, false, true);  // <== ERROR

    // Passing processing result to the callback
    return cb(data.second.first, data.second.second);  // <== ERROR
  }

  // Notifying callback on error
  return cb(result, {});
}

И тут понеслось:

  • Компилятор нашёл сразу две ошибки. Это очень подозрительно, может, мы не так типы расставили?
  • А что вообще такое data.second.first и data.second.second? В комментариях не написано.

Делать нечего, придётся прочитать весь код и комментарии, чтобы понять, как исправить ошибки.

Боль


Поначалу казалось, что много комментариев — это хорошо. Однако при отладке всё выглядит иначе. Код написан на двух языках: на английском и C++. Когда мы пытаемся понять, что происходит в коде, и отладить его, нужно прочитать английский, перевести его в голове на русский. Затем прочитать код на C++, его тоже перевести в голове на русский и сравнить эти два перевода. Убедиться, что смысл комментария совпадает с тем, что написано в коде, а если код делает что-то другое, то, возможно, там и кроется ошибка.

Ещё неприятно, что по коду разбросаны волшебные константы. Вот, например, -13 — что это такое? Почему -13? Комментарии не помогают. Волшебные константы только смущают и делают разбор функции сложнее. Но это цветочки, сейчас пойдут ягодки.

Попробуйте угадать, что значат булевые флажки true, false, true в функции process? В комментариях о них ни слова. Чтобы разобраться, нужно пойти в header file, где объявлена функция process:

std::pair<Descriptor, std::pair<int, std::string>> process(bool, Descriptor, bool, bool);

… И там мы увидим, что у булевых переменных нет осмысленных имён.

Чтобы понять, что происходит, нужно перейти в соседний файл, прочитать код функции process и разобраться в нём. Возможно, походить по соседним функциям и почитать их. На исследование смежных файлов тратится уйма времени, что мешает осознанию функции sample и портит настроение.

Наконец, data.second.first и data.second.second. Чтобы выяснить их назначение, нужно отмотать назад — туда, где мы получаем переменную data. Пойти в место, где объявлена функция process, увидеть, что комментариев нет, а process возвращает пару от пары. Пойти в исходники, узнать, что обозначают переменные int и string, — и на всё это снова уходит очень много нашего времени.

Ещё одна маленькая боль — код обработки ошибок перемешан с основной логикой. Это мешает ориентироваться. Обработка ошибок функции sample находится внизу, а в середине, внутри блока if, с большими отступами находится happy path.

Получается, что код, который на первый взгляд казался хорошим, красивым и легкочитаемым, на самом деле совсем не такой.

Выжимка проблем


  • Код написан на двух языках:
    – Его в два раза больше.
    – При отладке возникают проблемы со сверкой двух языков.

  • Комментариев всё ещё недостаточно:
    – Приходится читать код смежных функций.
    – Есть магические константы.

  • Код обработки ошибок и основной логики перемешаны:
    – Большие блоки кода с большими отступами.

Читаемый код


Перепишем sample. Постараемся сделать её настолько хорошей, чтобы всё было понятно из кода самой функции, без необходимости залезать в соседние файлы, читать документацию или комментарии.

Поехали:

std::string Sample(Descriptor listener,
                   std::string (*on_data)(Descriptor, std::string))
{
  UASSERT_MSG(on_data, "Callback must be a non-zero function pointer");

  const auto new_descriptor = Accept(listener, kAcceptTimeout);
  if (new_descriptor == kBadDescriptor) {
    return on_data(kBadDescriptor, {});
  }

  auto data = Process(Options::kSync, new_descriptor);
  return on_data(data.descriptor, data.payload);
}

В первой же строчке проверяем входные параметры. Это мини-подсказка/документация по тому, какие данные ожидаются на входе функции.

Следующая правка: вместо функции с непонятным именем Get появляется Accept, широко известная в кругах сетевых программистов. Затем страшную константу 1000 превращаем в именованную константу с осмысленным читаемым именем.

Теперь строка прекрасно читается без дополнительных комментариев: из listener мы принимаем новый дескриптор, на эту операцию даётся kAcceptTimeout.

Вместо того, чтобы писать в коде -13, а потом удивляться, что это за страшное число, заводим именованную константу для плохого дескриптора. Теперь мы сразу видим, что получили новый дескриптор, и проверяем, плохой он или хороший.

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

Специфика C++
Маленький бонус — большинство компиляторов в C++ считают одиночные if без блока else холодным путём.

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

В итоге мы незначительно (а то и вовсе незаметно) ускорили приложение. Пустячок, но приятно.

Дальше. Функция process преобразовалась. Вместо true, false, true теперь есть перечисление возможных опций для process. Код можно прочитать глазами. Сразу видно, что из дескриптора мы процессим какие-то данные в синхронном режиме и получаем их в переменную data.

Функция process вместо пары возвращает структуру с публичными полями, у каждого из которых есть осмысленное читаемое имя.

В результате код стал почти в два раза короче. Он стал понятнее. Больше не нужно ходить в соседние функции и файлы. Читать такой код куда приятнее.

Приёмы


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

На самом деле нет. Есть небольшой набор приёмов, и если их придерживаться, код уже будет получаться достаточно хорошим и читаемым. Расскажу о некоторых из них.

1) Compute(payload, 1023) — нечитаемо. Что такое 1023?
Используйте именованные константы. 
Compute(payload, kComputeTimeout)

Альтернативным решением может быть явное использование имён параметров. Например, Python позволяет писать:

Compute(payload=payload, timeout=1023)

Ну и C++20 не отстаёт: 

Compute({.payload=payload, .timeout=1023});

Идеальный результат получается, если пользоваться сразу двумя приёмами: 

Compute(payload=payload, timeout=MAX_TEST_RUN_TIME);

2) Compute(payload, false) — нечитаемо. Что такое false?
Используйте перечисления или именованные константы вместо bool.
У bool не всегда понятна семантика. Введение перечисления даже из двух значений явно описывает смысл конструкции.

Compute(payload, Device::kGPU)

Именованные аргументы в этом месте не всегда спасают:

Compute(payload=payload, is_cpu=False);

Всё ещё непонятно, что False заставляет считать на GPU.

3) Compute(data.second.first, data.second.second) или Compute(data[1][0], data[1][1]) — что вообще тут происходит?
Используйте типы с информативными именами полей, избегайте кортежей. 
Compute(data.node_id, data.chunk_id)

Совет помогает разобраться не только в том, что происходит в месте вызова, но ещё и полезен для документирования функции.

Попробуйте угадать, какой смысл у возвращаемых int и std::string в коде.

std::tuple<int, std::string> Receive();

int — это дескриптор устройства? Код возврата?

А вот так всё становится кристально ясно:

struct Response {
    int pending_bytes;
    std::string payload;
};
Response Receive();

4) void Accept(int , int); — что это за два числа?
Заводите отдельные типы данных для разных по смыслу вещей. 
void Accept(Descriptor, std::chrono::milliseconds)

Или для Python:

def accept(listener: Descriptor, timeout: datetime.timedelta) -> None:

На самом деле это совет не столько про читаемость кода, сколько про отлов ошибок. Многие (но далеко не все) современные языки программирования позволяют статически проверять типы и узнавать об ошибках ещё до запуска приложения или тестов. В C++ эта функциональность доступна из коробки, в Python нужно пользоваться линтерами и typing.

Думать в терминах системы типов — это определённое ментальное усилие. Чтобы получалось эффективно, нужна практика, как и для закрепления любого другого навыка. Но если этому научиться, вы сможете писать более понятный и менее бажный код.

5) void Compute(Data data) — функция есть в модуле или заголовке, но должны ли мы ей пользоваться?
Используйте особый namespace или именование для служебных вещей. 
namespace detail { void Compute(Data data); }

Или для Python:

def _compute(data: Data) -> None:

С namespace detail/impl или с особым наименованием пользователь поймёт, что функцию использовать не нужно.

6) d, cb, mut, Get — что это?
Придумывайте информативные имена переменных, классов и функций. 
descriptor, callback, mutator, GetDestination

Самый избитый, но важный совет — давайте осмысленные и информативные имена переменным, функциям и классам.

Кажется, что писать код, в котором переменные короче, получается быстрее. Под каждую переменную пара символов, можно быстро всё это напечатать. Но простая истина гласит: «Мы пишем код один раз, а читаем несколько раз». Потом, возможно, переписываем и снова несколько раз читаем.

Так вот, через неделю или месяц будет сложно вспомнить, что такое d или cd, что делает метод Get (или это вообще класс Get?). Что он возвращает?

Информативные имена вам очень помогут. При чтении будет сразу видно, где descriptor, callback и mutator, а где функция под именем GetDestination() возвращает какой-то Destination.

7) connection = address.Connect(timeout) — отладчик завёл нас в эту строчку кода. Что там за переменные, откуда они и куда мы их присваиваем?
Закрепите разные стили за переменными класса, аргументами функций и константами.
Если закрепить отдельные стили за разными типами переменных, код читается лучше. В большинстве распространённых code style именно так и делают:

connection_ = address.Connect(kTimeout);

Мы сразу видим, что переменная address — локальная; что мы пытаемся соединиться с kTimeout, который является константой. Результат соединения присваиваем переменной класса connection_. Поменяли буквально пару символов, а код стал понятнее.

Для Python стоит дополнительно придерживаться правила, что приватные поля начинаются с нижнего подчёркивания:

self._connection = address.Connect(TIMEOUT);

8) connection_ = address.Connect(timeout / attempts) — есть ли тут ошибка?
Используйте assert, чтобы проверить, правильно ли используют ваш код.
Если количество attempts будет равно нулю, то нативное приложение, скорее всего, рухнет. Где-то возникнет stack trace или корка. С помощью дополнительных телодвижений можно добраться до stack trace и понять, что падение произошло именно в этой строчке.

Если attempts окажется отрицательным числом, оно будет передано внутрь функции Connect. В лучшем случае внутри будет проверка — тогда мы сможем диагностировать проблему, вернувшись на пару шагов назад.

Однако если внутри Connect не будет проверки, всё станет сильно-сильно сложнее. Приложение не упадёт, но будет работать неправильно, не так, как мы ожидаем.

Коду явно не хватает проверок:

ASSERT(attempts > 0);

assert timeout > 0

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

Assert не только позволяет быстро находить ошибки, но и добавляет читаемости. Выражение assert timeout > 0 прямо говорит, что код ниже будет работать неправильно с отрицательными числами и 0.

8.1) connection_ = address.Connect(timeout / attempts) — есть ли тут ошибка?
НЕ используйте assert для проверки пользовательского ввода.
Будет невежливо (и опасно!), если ваша библиотека уронит весь сервер потому, что кто-то на сайте ввёл неправильное число. Здесь стоит использовать другие механизмы для сообщения об ошибках:

if (attempts <= 0) throw NegativeAttempts();

if (attempts <= 0) raise NegativeAttempts();

Как отличить неправильное использование функции программистом от неправильного ввода?

Вот несколько примеров:

  • функция init() должна быть вызвана перед первым использованием функции foo() — assert,
  • мьютекс не должен дважды захватываться из одного и того же потока — assert,
  • баланс на карте должен быть положительным — НЕ assert,
  • стоимость поездки не должна превышать миллиона доллларов — НЕ assert.

Если не уверены — не используйте assert.

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

ASSERT(attempts > 0);
if (attempts <= 0) throw NegativeAttempts();

9) v = foo(); bar(v); baz(v); assert(v); — а функциям bar() и baz() точно хорошо?
Не тяните с обработкой ошибок, не несите их в блок else.
Как только получили значение — сразу проверяйте и обрабатывайте ошибки, а дальше работайте как ни в чём не бывало.

Такой подход поможет избежать излишней вложенности конструкций — за вашей мыслью будет проще следить.

Пример:

if (value != BAD) {
    auto a = foo(value);
    auto b = bar(value);
    if (a + b < 0) {
         return a * b;
    } else {
         return a / b + baz();
    }
}

return 0;

Сравните:

if (value == BAD) {
    return 0;
}

auto a = foo(value);
auto b = bar(value);
if (a + b < 0) {
     return a * b;
}

return a / b + baz();

10) [(x, y) for x in [1,2,3] for y in [3,1,4] if x != y]
Придерживайтесь вменяемой вложенности конструкций.
Если в вашем коде есть if, внутри которого for, внутри которого if, внутри которого while, — это сложно читать. Стоит разнести какие-то внутренности на отдельные функции и дать функциям осмысленные имена. Так код станет красивее и приятнее для чтения.

11) Самая важная часть, самая большая хитрость, о которой мало где написано!
Если вам хочется поставить комментарий...
Попробуйте переделать код так, чтобы не хотелось.

Как правило, если у вас получается, код становится проще воспринимать. Если нет — ничего страшного, бывает, ставьте комментарий. Иногда без этого не обойтись.

Вместо заключения


Разумеется, это общие советы. У каждого языка есть своя специфика. В C++ важно писать понятные имена шаблонных параметров, в Python не стоит злоупотреблять тем, что переменные предоставляют возможности для общего модифицирующего владения.

Даже в рамках одной компании практики могут расходиться (например, в userver у нас пожёстче с наименованями и bool).

Пожалуйста, расскажите о принятых у вас практиках в комментариях! Будет интересно обсудить и взять их на вооружение.

UPD.
Правило от dkuch:
12) Сужать скоуп переменных до минимально возможного.