Прочти меня: код, который не выбесит соседа

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

Я расскажу о подходах, которые мы используем в Яндекс.Такси для написания читаемого кода на 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, {});
}

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

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, {});
}

Боль

Поначалу казалось, что много комментариев — это хорошо. Однако при отладке всё выглядит иначе. Код написан на двух языках: на английском и 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, а потом удивляться, что это за страшное число, заводим именованную константу для плохого дескриптора. Теперь мы сразу видим, что получили новый дескриптор, и проверяем, плохой он или хороший.

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

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

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

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

Приёмы

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

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

1) Compute(payload, 1023) — нечитаемо. Что такое 1023?

Compute(payload, kComputeTimeout)

Compute(payload=payload, timeout=1023)

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

Compute(payload=payload, timeout=MAX_TEST_RUN_TIME);

2) Compute(payload, false) — нечитаемо. Что такое false?

Compute(payload, Device::kGPU)

Compute(payload=payload, is_cpu=False);

3) Compute(data.second.first, data.second.second) или Compute(data[1][0], data[1][1]) — что вообще тут происходит?

Compute(data.node_id, data.chunk_id)

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

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

4) void Accept(int , int); — что это за два числа?

void Accept(Descriptor, std::chrono::milliseconds)

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

5) void Compute(Data data) — функция есть в модуле или заголовке, но должны ли мы ей пользоваться?

namespace detail { void Compute(Data data); }

def _compute(data: Data) -> None:

6) d, cb, mut, Get — что это?

descriptor, callback, mutator, GetDestination

7) connection = address.Connect(timeout) — отладчик завёл нас в эту строчку кода. Что там за переменные, откуда они и куда мы их присваиваем?

connection_ = address.Connect(kTimeout);

self._connection = address.Connect(TIMEOUT);

8) connection_ = address.Connect(timeout / attempts) — есть ли тут ошибка?

ASSERT(attempts > 0);

assert timeout > 0

8.1) connection_ = address.Connect(timeout / attempts) — есть ли тут ошибка?

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

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

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

9) v = foo(); bar(v); baz(v); assert(v); — а функциям bar() и baz() точно хорошо?

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]

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

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

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

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

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

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

Автор: Antony Polukhin

Источник