Code review

Нам предложили проверить с помощью анализатора PVS-Studio коллекцию открытых библиотек PMDK, предназначенную для разработки и отладки приложений с поддержкой энергонезависимой памяти. Собственно, почему бы и нет. Тем более это небольшой проект на языке C и C++ с общим размером кодовой базы около 170 KLOC, если не считать комментарии. А значит, обзор результатов анализа не займёт много сил и времени. Let's go.

Для анализа исходного кода будет использован инструмент PVS-Studio версии 7.08. Читатели нашего блога, естественно, давно знакомы с нашим инструментом, и я не буду на нём останавливаться. Для тех, кто впервые зашёл к нам, предлагаю обратиться к статье "Как быстро посмотреть интересные предупреждения, которые выдает анализатор PVS-Studio для C и C++ кода?" и попробовать бесплатную триальную версию анализатора.

В этот раз я загляну внутрь проекта PMDK и расскажу о замеченных ошибках и недочётах. По моему внутреннему ощущению, их было немного, что говорит о хорошем качестве кода этого проекта. Из интересного можно отметить, что найдено несколько фрагментов неправильного кода, который тем не менее работает правильно :). Что я имею в виду станет понятнее из дальнейшего повествования.

Итак, PMDK это коллекция библиотек и инструментальных средств с открытым исходным кодом, предназначенных для упрощения разработки, отладки и управления приложениями с поддержкой энергонезависимой памяти. Подробнее здесь: PMDK Introduction. Исходники здесь: pmdk.

Давайте посмотрим, какие ошибки и недочёты я смогу в нём найти. Сразу скажу, что я был далеко не всегда внимателен при анализе отчёта и мог многое пропустить. Поэтому призываю авторов проекта не руководствоваться при исправлении дефектов исключительно этой статьёй, а перепроверить код самостоятельно. А для написания статьи мне будет достаточно и того, что я выписал, просматривая список предупреждений :).

Неправильный код, который работает

Размер выделяемой памяти

Программисты нередко проводят время за отладкой кода, когда программа ведёт себя не так, как надо. Однако, иногда бывают ситуации, когда программа и работает корректно, но при этом код содержит ошибку. Программисту просто повезло, и ошибка не проявляет себя. В проекте PMDK я встретил сразу несколько таких интересных ситуаций и поэтому решил собрать их вместе в отдельной главе.

int main(int argc, char *argv[]){  ....  struct pool *pop = malloc(sizeof(pop));  ....}

Предупреждение PVS-Studio: V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'pop' class object. util_ctl.c 717

Классическая опечатка, из-за которой выделяется неверное количество памяти. Оператор sizeof вернёт не размер структуры, а размер указателя на эту структуру. Правильным вариантом будет:

struct pool *pop = malloc(sizeof(pool));

или

struct pool *pop = malloc(sizeof(*pop));

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

struct pool {  struct ctl *ctl;};

Получается, что структура занимает ровно столько, сколько и указатель. Всё хорошо.

Длина строки

Перейдём к следующему случаю, где вновь допущена ошибка с использованием оператора sizeof.

typedef void *(*pmem2_memcpy_fn)(void *pmemdest, const void *src, size_t len,    unsigned flags);static const char *initial_state = "No code.";static inttest_rwx_prot_map_priv_do_execute(const struct test_case *tc,  int argc, char *argv[]){  ....  char *addr_map = pmem2_map_get_address(map);  map->memcpy_fn(addr_map, initial_state, sizeof(initial_state), 0);  ....}

Предупреждение PVS-Studio: V579 [CWE-687] The memcpy_fn function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. pmem2_map_prot.c 513

Для копирования строки используется указатель на специальную функцию копирования. Обратите внимание на вызов этой функции, а вернее на её третий аргумент.

Программист предполагает, что оператор sizeof вычислит размер строкового литерала. Но, на самом деле, вновь вычисляется размер указателя.

Везение в том, что строка состоит из 8 символов, и её размер совпадает с размером указателя, если происходит сборка 64-битного приложения. В результате все 8 символов строки "No code." будут успешно скопированы.

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

Сценарий 1. Нужно было скопировать терминальный ноль. Тогда я неправ, и это вовсе не безобидная ошибка, которая себя не проявляет. Скопировано не 9 байт, а только 8. Терминального нуля нет, и последствия предсказать невозможно. В этом случае код можно исправить, изменив определение константной строки initial_state следующим образом:

static const char initial_state [] = "No code.";

Теперь значение sizeof(initial_state) равно 9.

Сценарий 2. Терминальный ноль вовсе и не требуется. Например, ниже можно увидеть вот такую строчку кода:

UT_ASSERTeq(memcmp(addr_map, initial_state, strlen(initial_state)), 0);

Как видите, функция strlen вернёт значение 8 и терминальный ноль не участвует в сравнении. Тогда это действительно везение и всё хорошо.

Побитовый сдвиг

Следующий пример связан с операцией побитового сдвига.

static intclo_parse_single_uint(struct benchmark_clo *clo, const char *arg, void *ptr){  ....  uint64_t tmax = ~0 >> (64 - 8 * clo->type_uint.size);  ....}

Предупреждение PVS-Studio: V610 [CWE-758] Unspecified behavior. Check the shift operator '>>'. The left operand '~0' is negative. clo.cpp 205

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

Приоритет операций

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

#define BTT_CREATE_DEF_SIZE  (20 * 1UL << 20) /* 20 MB */

Предупреждение PVS-Studio: V634 [CWE-783] The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. bttcreate.c 204

Чтобы получить константу, равную значению 20 MB, программист решил выполнить следующие действия:

• Сдвинул 1 на 20 разрядов, чтобы получить значение 1048576, т.е. 1 MB.
• Умножил 1 MB на 20.

Другими словами, программист думает, что вычисления происходят так: (20 * (1UL << 20))

Но на самом деле приоритет оператора умножения выше, чем приоритет оператора сдвига и выражение вычисляется так: ((20 * 1UL) << 20).

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

Но эта ошибка никак не проявит себя. Неважно, как написать:

• (20 * 1UL << 20)
• (20 * (1UL << 20))
• ((20 * 1UL) << 20)

Результат всё равно всегда одинаковый! Всегда получается нужное значение 20971520 и программа работает совершенно корректно.

Другие ошибки

Не там поставленная скобка

#define STATUS_INFO_LENGTH_MISMATCH 0xc0000004static voidenum_handles(int op){  ....  NTSTATUS status;  while ((status = NtQuerySystemInformation(      SystemExtendedHandleInformation,      hndl_info, hi_size, &req_size)        == STATUS_INFO_LENGTH_MISMATCH)) {    hi_size = req_size + 4096;    hndl_info = (PSYSTEM_HANDLE_INFORMATION_EX)REALLOC(hndl_info,        hi_size);  }  UT_ASSERT(status >= 0);  ....}

Предупреждение PVS-Studio: V593 [CWE-783] Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. ut.c 641

Внимательно посмотрите вот сюда:

while ((status = NtQuerySystemInformation(....) == STATUS_INFO_LENGTH_MISMATCH))

Программист хотел сохранить в переменной status значение, которое возвращает функцию NtQuerySystemInformation, а затем сравнить его с константой.

Программист скорее всего знал, что приоритет оператора сравнения (==) выше, чем у оператора присваивания (=), и поэтому следует использовать скобки. Но опечатался и поставил их не там, где надо. В результате скобки никак не помогают. Корректный код:

while ((status = NtQuerySystemInformation(....)) == STATUS_INFO_LENGTH_MISMATCH)

Из-за этой ошибки, макрос UT_ASSERT никогда не сработает. Ведь в переменную status всегда заносится результат сравнения, то есть ложь (0) или истина (1). Значит условие ([0..1] >= 0) всегда истинно.

Потенциальная утечка памяти

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  if (!oidp)    return POCLI_ERR_PARS;  ....}

Предупреждение PVS-Studio: V773 [CWE-401] The function was exited without releasing the 'input' pointer. A memory leak is possible. pmemobjcli.c 238

Если oidp окажется нулевым указателем, то будет потеряна копия строки, созданная с помощью вызова функции strdup. Лучше всего будет перенести проверку до выделения памяти:

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  if (!oidp)    return POCLI_ERR_PARS;  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  ....}

Или можно явно освобождать память:

static enum pocli_retpocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp){  char *input = strdup(in);  if (!input)    return POCLI_ERR_MALLOC;  if (!oidp)  {    free(input);    return POCLI_ERR_PARS;  }  ....}

Потенциальное переполнение

typedef long long os_off_t;voiddo_memcpy(...., int dest_off, ....., size_t mapped_len, .....){  ....  LSEEK(fd, (os_off_t)(dest_off + (int)(mapped_len / 2)), SEEK_SET);  ....}

Предупреждение PVS-Studio: V1028 [CWE-190] Possible overflow. Consider casting operands, not the result. memcpy_common.c 62

Явное приведение результата сложения к типу os_off_t не имеет смысла. Во-первых, это не защищает от потенциального переполнения, которое может возникнуть при сложении двух значений типа int. Во-вторых, результат сложения и так бы отлично неявно расширился до типа os_off_t. Явное приведение типа просто избыточно.

Думаю, правильнее будет написать так:

LSEEK(fd, dest_off + (os_off_t)(mapped_len) / 2, SEEK_SET);

Здесь беззнаковое значение типа size_t превращается в знаковое (чтоб не было какого-нибудь предупреждения от компилятора). И заодно точно не возникнет переполнение при сложении.

Неправильная защита от переполнения

static DWORDget_rel_wait(const struct timespec *abstime){  struct __timeb64 t;  _ftime64_s(&t);  time_t now_ms = t.time * 1000 + t.millitm;  time_t ms = (time_t)(abstime->tv_sec * 1000 +    abstime->tv_nsec / 1000000);  DWORD rel_wait = (DWORD)(ms - now_ms);  return rel_wait < 0 ? 0 : rel_wait;}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'rel_wait < 0' is always false. Unsigned type value is never < 0. os_thread_windows.c 359

Мне не очень понятно, от какой именно ситуации должна защищать проверка, но она в любом случае не работает. Переменная rel_wait имеет беззнаковый тип DWORD. А значит, сравнение rel_wait < 0 не имеет смысла, так как результатом всегда является истина.

Отсутствие проверки, что память успешно выделена

Проверка того, что память выделена, осуществляется с помощью макросов assert, которые ничего не делают, если компилируется Release версия приложения. Так что можно сказать, что нет никакой обработки ситуации, когда функции malloc возвращают NULL. Пример:

static voidremove_extra_node(TOID(struct tree_map_node) *node){  ....  unsigned char *new_key = (unsigned char *)malloc(new_key_size);  assert(new_key != NULL);  memcpy(new_key, D_RO(tmp)->key, D_RO(tmp)->key_size);  ....}

Предупреждение PVS-Studio: V575 [CWE-628] The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 340, 338. rtree_map.c 340

В других местах нет даже assert:

static voidcalc_pi_mt(void){  ....  HANDLE *workers = (HANDLE *) malloc(sizeof(HANDLE) * pending);  for (i = 0; i < pending; ++i) {    workers[i] = CreateThread(NULL, 0, calc_pi,      &tasks[i], 0, NULL);    if (workers[i] == NULL)      break;  }  ....}

Предупреждение PVS-Studio: V522 [CWE-690] There might be dereferencing of a potential null pointer 'workers'. Check lines: 126, 124. pi.c 126

Таких фрагментов кода я насчитал минимум 37 штук. Так что я не вижу смысла перечислять все их в статье.

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

Код с запахом

Двойной вызов CloseHandle

static voidprepare_map(struct pmem2_map **map_ptr,  struct pmem2_config *cfg, struct pmem2_source *src){  ....  HANDLE mh = CreateFileMapping(....);  ....  UT_ASSERTne(CloseHandle(mh), 0);  ....}

Предупреждение PVS-Studio: V586 [CWE-675] The 'CloseHandle' function is called twice for deallocation of the same resource. pmem2_map.c 76

Смотря на этот код и предупреждение PVS-Studio понятно, что ничего непонятно. Где тут возможен повторный вызов CloseHandle? Чтобы найти ответ, давайте посмотрим на реализацию макроса UT_ASSERTne.

#define UT_ASSERTne(lhs, rhs)\  do {\    /* See comment in UT_ASSERT. */\    if (__builtin_constant_p(lhs) && __builtin_constant_p(rhs))\      UT_ASSERT_COMPILE_ERROR_ON((lhs) != (rhs));\    UT_ASSERTne_rt(lhs, rhs);\  } while (0)

Сильно понятнее не стало. Что такое UT_ASSERT_COMPILE_ERROR_ON? Что такое UT_ASSERTne_rt?

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

do {  if (0 && 0) (void)((CloseHandle(mh)) != (0));  ((void)(((CloseHandle(mh)) != (0)) ||    (ut_fatal(".....", 76, __FUNCTION__, "......: %s (0x%llx) != %s (0x%llx)",              "CloseHandle(mh)", (unsigned long long)(CloseHandle(mh)), "0",              (unsigned long long)(0)), 0))); } while (0);

Удалим всегда ложное условие (0 && 0) и, вообще, всё не относящееся к делу. Получается:

((void)(((CloseHandle(mh)) != (0)) ||  (ut_fatal(...., "assertion failure: %s (0x%llx) != %s (0x%llx)",            ....., (unsigned long long)(CloseHandle(mh)), .... ), 0)));

Происходит закрытие хендла. Если при этом произошла ошибка, то генерируется отладочное сообщение и, чтобы получить код ошибки вновь, вызывается CloseHandle для этого же самого некорректного хендла.

Ошибки, вроде как, и нет. Раз хендл невалиден, то ничего страшного, что для него дважды вызывается функция CloseHandle. Однако, этот код с запахом. Более идеологически верным было бы вызывать функцию только один раз и сохранять статус, который она вернула, чтобы затем при необходимости вывести его значение в сообщении.

Несоответствие интерфейса реализации (снятие константности)

static intstatus_push(PMEMpoolcheck *ppc, struct check_status *st, uint32_t question){  ....  } else {    status_msg_info_and_question(st->msg);            // <=    st->question = question;    ppc->result = CHECK_RESULT_ASK_QUESTIONS;    st->answer = PMEMPOOL_CHECK_ANSWER_EMPTY;    PMDK_TAILQ_INSERT_TAIL(&ppc->data->questions, st, next);  }  ....}

Анализатор выдаёт сообщение: V530 [CWE-252] The return value of function 'status_msg_info_and_question' is required to be utilized. check_util.c 293

Причина в том, что функция status_msg_info_and_question, с точки зрения анализатора, не меняет состояние внешних по отношению к ней объектов, в том числе переданную константную строчку. Т.е. функция просто что-то считает и возвращает результат. А раз так, то странно не использовать результат, который возвращает эта функция. И, хотя анализатор в этот раз не прав, он указывает на код с запахом. Давайте посмотрим, как устроена вызываемая функция status_msg_info_and_question.

static inline intstatus_msg_info_and_question(const char *msg){  char *sep = strchr(msg, MSG_SEPARATOR);  if (sep) {    *sep = ' ';    return 0;  }  return -1;}

При вызове функции strchr происходит неявное снятие константности. Дело в том, что в C она объявлена так:

char * strchr ( const char *, int );

Не лучшее решение. Но язык C такой, какой есть :).

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

Тем не менее, хоть анализатор и запутался, он указывает на код с запахом. То, что сбивает с толку анализатор, может сбивать с толку и человека, который сопровождает код. Лучше было бы объявить функцию более честно, убрав const:

static inline intstatus_msg_info_and_question(char *msg){  char *sep = strchr(msg, MSG_SEPARATOR);  if (sep) {    *sep = ' ';    return 0;  }  return -1;}

Так и намерения сразу понятнее, и анализатор будет молчать.

Переусложненный код

static struct memory_blockheap_coalesce(struct palloc_heap *heap,  const struct memory_block *blocks[], int n){  struct memory_block ret = MEMORY_BLOCK_NONE;  const struct memory_block *b = NULL;  ret.size_idx = 0;  for (int i = 0; i < n; ++i) {    if (blocks[i] == NULL)      continue;    b = b ? b : blocks[i];    ret.size_idx += blocks[i] ? blocks[i]->size_idx : 0;  }  ....}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'blocks[i]' is always true. heap.c 1054

Если blocks[i] == NULL, то сработает оператор continue и цикл начнёт следующую итерацию. Поэтому повторная проверка элемента blocks[i] не имеет смысла и тернарный оператор является лишним. Код можно упростить:

....for (int i = 0; i < n; ++i) {  if (blocks[i] == NULL)    continue;  b = b ? b : blocks[i];  ret.size_idx += blocks[i]->size_idx;}....

Подозрительное использование нулевого указателя

void win_mmap_fini(void){  ....  if (mt->BaseAddress != NULL)    UnmapViewOfFile(mt->BaseAddress);  size_t release_size =    (char *)mt->EndAddress - (char *)mt->BaseAddress;  void *release_addr = (char *)mt->BaseAddress + mt->FileLen;  mmap_unreserve(release_addr, release_size - mt->FileLen);  ....}

Предупреждение PVS-Studio: V1004 [CWE-119] The '(char *) mt->BaseAddress' pointer was used unsafely after it was verified against nullptr. Check lines: 226, 235. win_mmap.c 235

Указатель mt->BaseAddress может быть нулевым, о чём свидетельствует проверка:

if (mt->BaseAddress != NULL)

Однако ниже этот указатель уже используется в арифметических действиях без проверки. Например, здесь:

size_t release_size =  (char *)mt->EndAddress - (char *)mt->BaseAddress;

Будет получено некое большое целочисленное значение, равное фактически значению указателя mt->EndAddress. Возможно, это и не ошибка, но выглядит всё это очень подозрительно, и мне кажется, код следует перепроверить. Запах заключается в том, что код непонятен и ему явно не хватает поясняющих комментариев.

Короткие имена глобальных переменных

Я считаю, что код пахнет, если содержит глобальные переменные с короткими именами. Легко опечататься и случайно использовать в какой-то функции не локальную, а глобальную переменную. Пример:

static struct critnib *c;

Предупреждения PVS-Studio на такие переменные:

• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'ri' variable. map.c 131
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'c' variable. obj_critnib_mt.c 56
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.h 68
• V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.c 34

Странное

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

voiddo_memmove(char *dst, char *src, const char *file_name,    size_t dest_off, size_t src_off, size_t bytes,    memmove_fn fn, unsigned flags, persist_fn persist){  ....  /* do the same using regular memmove and verify that buffers match */  memmove(dstshadow + dest_off, dstshadow + dest_off, bytes / 2);  verify_contents(file_name, 0, dstshadow, dst, bytes);  verify_contents(file_name, 1, srcshadow, src, bytes);  ....}

Предупреждение PVS-Studio: V549 [CWE-688] The first argument of 'memmove' function is equal to the second argument. memmove_common.c 71

Обратите внимание, что первый и второй аргумент функции совпадают. Таким образом, функция по факту ничего не делает. Какие мне на ум приходят варианты:

• Хотелось "потрогать" блок памяти. Но произойдёт ли это в реальности? Не удалит ли оптимизирующий компилятор код, который копирует блок памяти сам в себя?
• Это какой-то юнит-тест на работу функции memmove.
• Код содержит опечатку.

А вот не менее странный фрагмент в этой же функции:

voiddo_memmove(char *dst, char *src, const char *file_name,    size_t dest_off, size_t src_off, size_t bytes,    memmove_fn fn, unsigned flags, persist_fn persist){  ....  /* do the same using regular memmove and verify that buffers match */  memmove(dstshadow + dest_off, srcshadow + src_off, 0);  verify_contents(file_name, 2, dstshadow, dst, bytes);  verify_contents(file_name, 3, srcshadow, src, bytes);  ....}

Предупреждение PVS-Studio: V575 [CWE-628] The 'memmove' function processes '0' elements. Inspect the third argument. memmove_common.c 82

Функция перемещает 0 байт. Что это? Юнит-тест? Опечатка?

Для меня этот код непонятен и странен.

Зачем использовать анализаторы кода?

Может показаться, что раз найдено мало ошибок, то и внедрение анализатора в процесс разработки кода малообоснованно. Но смысл использования инструментов статического анализа не в разовых проверках, а в регулярном выявлении ошибок ещё на этапе написания кода. В противном случае, эти ошибки выявляются более дорогими и медленными способами (отладка, тестирование, отзывы пользователей и так далее). Более подробно эта мысль изложена в статье "Ошибки, которые не находит статический анализ кода, потому что он не используется", с которой я рекомендую познакомиться. А затем приходите к нам на сайт скачать и попробовать PVS-Studio для проверки своих проектов.

Спасибо за внимание!


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Static code analysis of the PMDK library collection by Intel and errors that are not actual errors.

В 2019 году на DUMP в Казани выступал Никита Соболев технический директор компании Мы делаем сервисы. И Никита на протяжении почти 40 минут пытался вскипятить мозги слушателей секции Backend, рассуждая о code review. Сегодня хотим привести расшифровку этого взрывного доклада, чтобы если уж мозги бурлили, то у всех сразу :)

А вот, кстати, и сам Никита Соболев во время своего выступления.

Для удобства вот ссылка на презентацию.
Доклад начался с постановки того, о чем же он будет. И внезапно прозвучала фраза: Я не буду рассказывать о code review. Доклад будет про хардкорные технические инструменты и методы автоматизации. Главный тезис если вы делаете code review, то вы уже проиграли. Интригующе звучит? :) Давайте вместе с Никитой разбираться, что же за этим стоит.

За основу были взяты понятия, которые предложил американский писатель Карлос Кастанеда, делание и неделание. Что же это значит? Делание это привычка, внутри которой мы существуем. Эдакое хомячье колесо. А неделание взгляд на те же самые вещи, но под другим углом. Никита ловко наложил эти понятия на просмотр кода. И получилось, что мы имеем делание code review и, соответственно, неделание code review. Вот как раз последнему и посвящен доклад.

Делание становится рутиной, чем-то сложным, где нужно применить свои коммуникативные навыки, быть эмпатичным. И встает вопрос: а нафига я буду смотреть какой-то код? Неделание же это гармония. Ты смотришь на код и восхищаешься им. Ты понимаешь, что вот этот вот код станет частью продукта и принесет новые деньги. И конечная цель, к которой подводит нас Никита всеми этими речами в том, чтобы делать code review за 15 минут. И даже есть живой пример.

Но для начала нужно разобраться, почему же все так плохо с code review?

Когда ты узнал, что есть code review, то начинаешь пытаться решить все проблемы с его помощью. А проблем на самом деле очень много, как и целей. Какие же цели ставят перед code review?

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

Казалось бы, вполне себе нормальные цели, но нет. Никита считает, что все эти цели неправильные. Причем как только вы это поймете, то измените свое отношение с делание на неделание. Встают резонные вопросы: а какая тогда правильная цель? Чем же плохи эти цели? Пойдем разбираться с конца.

Обучение людей с помощью code review.

Обычно новым людям дают какую-то неважную задачу, а это плохая идея. Она тянет за собой изоляцию (посиди вон там, сам поразбирайся) и отсутствие контроля (задача же неважная, поэтому всем пофиг).

Решение: маленькие задачи. Вы можете дать новичку сразу что-то важное, но при этом небольшое. Это ускоряет обратную связь, увеличивает вероятность найти косяк в коде. Ведь чем массивнее код, тем сложнее поиск ошибок говорит статистика. Под маленькими подразумеваются задачи от 15 минут до 2х, максимум 4х часов. Соответственно, это все предполагает наличие онбординга и хорошей документации. За примером хорошего онбординга (без нудного code review по неделям) идем на Open-Source. Что у них есть? Чек-лист:

• Contributing.md документ, в котором описаны самые верхнеуровневые шаги;
• Developer Docs документы с описанием всех api-методов и вообще всего;
• Architecture Decision Records история решений, принятых по поводу архитектуры. Очень помогает въезжать в процесс новичкам;
• Wiki с ответами на популярные вопросы;
• База данных задач и pull requests;
• И главное понятные для людей тесты.

Если ваш онбординг построен как-то не так, то вам есть куда расти, потому что на Open-Source люди стремятся вот к этому.

Для вдохновения Никита предлагает ряд хороших мест:

1. Gatsby.js одна из лучших и самых быстрорастущих по количеству контрибьюторов систем, которая описала вообще все;
2. Dev.to сайт со статьями, документцией, видосами и т.д. У них тоже все прекрасно;
3. Wemake-python-styleguide творение, к которому сам Никита относится, и по его словам большое количество новых контрибьюторов и довольно мало ошибок.

Перейдем к обсуждению следующей проблемы review архитектуры. Обычно оно корявое. Какие с ним могут быть проблемы?

• Долго. Когда мы говорим о том, что у нас есть review архитектуры, то это значит, что человек должен пойти и подумать, написать, все перерефакторить и через месяц прислать нам pull request. И как мы будем его ревьюить? Естественно, никак.
  
• Дорого переделывать.
• Участвует один человек.

Решение: проектировать и прототипировать design до review. Design до review это когда вы делаете не саму архитектуру, а ее скелет. Текстом или фиксируете в каком-нибудь файлике. Человек по скелету сможет сказать нравится\не нравится. Это намного быстрее и эффективнее, чем code review в данном смысле.

И, конечно, автоматизация! Никита рассказал о том, как можно автоматизировать архитектурные проверки.

В каждом приложении есть слои (тут, кстати, Никита проводил аналогию с офигенным бургером). И чтобы проверять их есть все возможности. Далее примеры пойдут на языке python.

• Есть такая штука называется importlinter. Она умеет проверять вашу архитектуру на основе ваших импортов. Когда вы делаете импорт, значит, эти два модуля связаны между собой. Эта связанность и проверяется. Как это делается? Указываете название вашего пакета, после этого вы определяете тип контракта. Все абсолютно декларативно, без кода. Это все текстовый файл. У нас есть такое то имя, вот такой-то тип контракта, который называется layers. И мы будем проверять пакет django_project. У него есть слои: urls, views, forms, models, logic. Соответственно, logic может импортировать ничего. Models могут импортировать logics. Forms могут импортировать models и logic и так далее.

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

• Проверка абстракций, которые не должны перетекать. Например, у нас есть логика. И логика чистая, которая не знает ни про какие джанги, фласки или любые другие ваши пакеты. Она просто работает с какими-то абстрактными классами, функциями и т.д. Тогда выборочно запретите импортировать.

• Для более сложных случаев, когда вот здесь можно, а здесь нельзя, а сверху еще требуются дополнительные проверки напишите свой контракт. Есть api, вы пишите свой контракт готово!

Таким образом можно ревьюить архитектуру автоматически.

Следующая проблема неповторение грабель.

Часто code review нужен для того, чтобы показать новым разработчикам, что мы так уже делали. Как правило, приходится потом повторять как попугай одно и то же. Решение: документация и регрессионное тестирование.

В понимании нашего чудесного докладчика, хорошая документация это когда вот так.

Все начинается с какого-то бизнес-требования, затем это превращается в BDDSpec. Вкратце, BDDSpec это то же самое требование, но написанное структурированным языком, которое потом превращается в тест. Этот тест является вашей документацией, потому что структурированный текст понятен человеку. Собственно, BDD инструмент общения, при помощи которого вы пишите и тест, и документацию. Отсюда появляется еще одна сущность реализация, связанная с тестом и документацией. Все это помогает избежать совершения одних и тех же ошибок, ведь все уже указано в тестах.

В ситуациях, когда что-то сломалось, можно написать регрессионный тест. И выходит вот такая документашка.

И теперь другая автоматизация! Часто люди делают ошибки не только в коде. Для решения этой проблемы есть замечательный проект Danger. С ним схема будет выглядеть вот так:

На 2020 год danger-правило можно написать на следующих языках: JS, Swift, Ruby, Kotlin и Python. Никита в своем докладе привел примеры на JS.

Валим CI проверки:

• Pull request не в мастере, а надо в мастер.
• Pull request не мерджится из-за конфликтов.

Предупреждающие проверки:

• Pull request без описания.
• Забыли указать номер issue в теле.

Так это выглядит в итоге. Здесь исключено человеческое взаимодействие, все автоматизировано.

И еще одна фишечка утилита bellybutton. Она есть для всех языков (проверено Никитой). Допустим, у вас есть некая deprecated_fn(), которую не надо вызывать. Она старая и странная, и существует просто потому что так надо. Вы говорите человеку, чтоб не трогал ее, но он все равно трогает. Когда-нибудь надоест это повторять, и тогда вы берете декларативный файл формата yaml и пишите:

Декларативный файл можно поместить в репозиторий, описать там все правила для текущего проекта и больше человеку по code review не говорить об этом вообще никогда.

Контроль качества разбираем еще одну проблему. Для человека, который пишет статические анализаторы, слышать фразу о том, что человек может контролировать качество анекдот.

Почему все так? Во-первых, люди не умеют писать код. Загвоздка в том, что человек должен писать понятно и для другого человека, и для компьютера, а последние два говорят на разных языках. Отсюда и большая сложность. Во-вторых, эта сложность быть понятным для всех и сразу постепенно накапливается. В-третьих, скрытость метрик. Чтобы проверить качество кода, нужно сначала вывести метрики куда-то на экран, посмотреть все это дело, а затем еще долго исправлять. Именно поэтому проблема качества кода это очень и очень серьезная проблема, и к ней нужно подходить с самого начала проекта. Решение: самый жесткий линтер из доступных. Чаще придется писать свой: под разные языки + существующий может быть недостаточно строг. Как говорит Никита: чем больше вы ненавидите линтер, тем он лучше.

Проблема баланса кода и архитектуры. Тут могут возникать такие ситуации:

• При слишком сложной архитектуре приложения перестаешь понимать, что происходит и куда писать наш код;
• Архитектуры много, а кода 20 строчек;
• Наоборот, кода много отсюда сложность работы с ним.

Решение: Architecture on Demand. Оно позволит писать ровно столько архитектуры, сколько нужно. Простая архитектура и простой код под текущую ситуацию. Подробнее Никита этот способ разобрал здесь.

И last but not least проблема контроля выполнения. Обычно говорят, что контроль выполнения это, когда code review, прогон всех тестов, запуск на компьютере, но нет. Перед нами встают проблемы:

• Я не умею запускать код в мозгу;
• Я не знаю что, нужно клиенту;
• Не все нюансы понятны из кода.

Решение: BDD (эффективный способ общения и с заказчиком, и с программистами, и с кем угодно) и Review Apps. Последнее нужно для посмотреть глазками, потому что мало просто прогнать тест. При таком подходе вы сможете сделать ревью уже задеплоенного приложения. Например, использовать ZEIT для GitLab. Так появляется возможность получить на каждый pull request версию приложения.

Только тогда, когда вы прошли все тесты, клиент посмотрел и сказал, что это то, что нужно, вы сами посмотрели, что это работает, можно сказать, что задача выполнена. В итоге получается, что это все не про code review. Код еще даже никто не смотрел.

А теперь подведем итоги. Были разобраны все обозначенные проблемы. Как можно увидеть, ни одна из них к code review отношения не имеет. Для каждой из проблем есть свое целевое решение. Зачем тогда нужно code review? Code review нужно для контроля корректности автоматики. Автоматики у нас полно. Она, естественно, сбоит. И нам нужно глазами проверять, что человек при ней сделал неправильно.

Инструменты, о которых мы говорили:

1. маленькие задачи (от 15 мин до 2ч., max 4ч.);
2. review apps чтоб контролировать исполнение;
3. суровый статический анализ, чтоб проверять качество кода и вообще все, что с ним связано;
4. строгие архитектурные правила, которые будут декларативно описаны, чтобы четко понимать, где и что мы делаем;
5. глаза человека как самый последний рубеж. То есть пока не пройдет вся автоматика, на review код даже не попадает.

А нужно ли тогда код вообще смотреть, спросите вы? Ответ конечно, ведь нужен контроль некоторых нюансов:

• корректность имен;
• проверка гипотез;
• проверка общей адекватности.

Вот это основные задачи code review. Все остальное вполне может решить автоматизация. При таком подходе делание code review превращается в неделание, причем за 15 минут. И последние несколько слов от Никиты: созерцайте ваш код, смотрите на то, как можно сделать лучше вашу кодовую базу и делайте это.

Заревьюить доклад своими глазками можно тут :)

А вы что думаете по поводу философского взгляда на code review?

P.S. Как организаторы DUMP`а хотим выразить благодарность Никите Соболеву за классный доклад :) Надеемся увидеться с ним, и с нашими участниками на DUMP 2020 в Екатеринбурге 20 ноября.

Одно из первых наставлений, которое молодой падаван получает вместе с доступом к git-репозиториям, звучит так: никогда не ешь жёлтый снег делай git push -f. Поскольку это одна из сотен максим, которые нужно усвоить начинающему инженеру-разработчику ПО, никто не тратит время на уточнение, почему именно так нельзя делать. Это как младенцы и огонь: спички детям не игрушки и баста. Но мы растём и развиваемся как люди и как профессионалы, и однажды вопрос а почему, собственно? встаёт в полный рост. Эта статья написана по мотивам нашего внутреннего митапа, на тему: Когда можно и нужно переписывать историю коммитов.



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

Для этого, в свою очередь, нам понадобится быстрый экскурс в физическую структуру git-репозитория. Если вы точно уверены, что знаете об устройстве репо всё, то можете пропустить эту часть, но даже я в процессе выяснения узнал для себя довольно много нового, а кое-что старое оказалось не вполне релевантным.

На самом низком уровне git-репо представляет собой набор объектов и указателей на них. Каждый объект имеет свой уникальный 40-значный хэш (20 байт в 16-ричной системе), который вычисляется на основе содержимого объекта.



Иллюстрация взята из The Git Community Book

Основные типы объектов это blob (просто содержимое файла), tree (набор указателей на blobs и другие trees) и commit. Объект типа commit представляет собой только указатель на tree, на предыдущий коммит и служебную информацию: дата/время, автор и комментарий.

Где здесь ветки и тэги, которыми мы привыкли оперировать? А они не являются объектами, они являются просто указателями: ветка указывает на последний коммит в ней, тэг на произвольный коммит в репо. То есть когда мы в IDE или GUI-клиенте видим красиво нарисованные веточки с кружочками-коммитами на них они строятся на лету, пробегая по цепочкам коммитов от концов веток вниз к корню. Самый первый коммит в репо не имеет предыдущего, вместо указателя там null.

Важный для понимания момент: один и тот же коммит может фигурировать в нескольких ветках одновременно. Коммиты не копируются при создании новой ветки, она просто начинает расти с того места, где был HEAD в момент отдачи команды git checkout -b <branch-name>.

Итак, почему же переписывание истории репозитория вредно?



Во-первых, и это очевидно, при загрузке новой истории в репозитории, с которым работает команда инженеров, другие люди могут просто потерять свои изменения. Команда git push -f удаляет из ветки на сервере все коммиты, которых нет в локальной версии, и записывает новые.

Почему-то мало кто знает, что довольно давно у команды git push существует безопасный ключ --force-with-lease, который заставляет команду завершиться с ошибкой, если в удалённом репозитории есть коммиты, добавленные другими пользователями. Я всегда рекомендую использовать его вместо -f/--force.

Вторая причина, по которой команда git push -f считается вредной, заключается в том, что при попытке слияния (merge) ветки с переписанной историей с ветками, где она сохранилась (точнее, сохранились коммиты, удалённые из переписанной истории), мы получим адское число конфликтов (по числу коммитов, собственно). На это есть простой ответ: если аккуратно соблюдать Gitflow или Gitlab Flow, то такие ситуации, скорее всего, даже не возникнут.

И наконец есть неприятная побочка переписывания истории: те коммиты, которые как бы удаляются при этом из ветки, на самом деле, никуда не исчезают и просто остаются навечно висеть в репо. Мелочь, но неприятно. К счастью, эту проблему разработчики git тоже предусмотрели, введя команду сборки мусора git gc --prune. Большинство git-хостингов, как минимум GitHub и GitLab, время от времени,
производят эту операцию в фоне.

Итак, развеяв опасения перед изменением истории репозитория, можно, наконец, перейти к главному вопросу: зачем оно нужно и когда оправдано?

На самом деле, я уверен, что практически каждый из более-менее активных пользователей git хоть раз, да изменял историю, когда вдруг оказывалось, что в последнем коммите что-то пошло не так: вкралась досадная опечатка в код, сделал коммит не от того пользователя (с личного e-mail вместо рабочего или наоборот), забыл добавить новый файл (если вы, как я, любите пользоваться git commit -a). Даже изменение описания коммита приводит к необходимости его перезаписи, ведь хэш считается и от описания тоже!

Но это тривиальный случай. Давайте рассмотрим более интересные.

Допустим, вы сделали большую фичу, которую пилили несколько дней, отсылая ежедневно результаты работы в репозиторий на сервере (4-5 коммитов), и отправили свои изменения на ревью. Двое-трое неутомимых ревьюверов закидали вас крупными и мелкими рекомендациями правок, а то и вовсе нашли косяки (ещё 4-5 коммитов). Затем QA нашли несколько краевых случаев, тоже требующих исправлений (ещё 2-3 коммита). И наконец при интеграции выяснились какие-то несовместимости или попадали автотесты, которые тоже надо пофиксить.

Если теперь нажать, не глядя, кнопку Merge, то в главную ветку (у многих она по старинке называется master) вольются полтора десятка коммитов типа My feature, day 1, Day 2, Fix tests, Fix review и т.д. От этого, конечно, помогает режим squash, который сейчас есть и в GitHub, и в GitLab, но с ним надо быть осторожными: во-первых, он может заменить описание коммита на что-то непредсказуемое, а во-вторых заменить автора фичи на того, кто нажал кнопку Merge (у нас это вообще робот, помогающий релиз-инженеру собрать сегодняшний деплой). Поэтому самым простым будет перед окончательной интеграцией в релиз схлопнуть все коммиты ветки в один при помощи git rebase.

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



У меня рука машинально потянулась к кнопке Report abuse, потому что как ещё можно охарактеризовать реквест из 50 коммитов с почти 2000 изменённых строк? И как его, спрашивается, ревьюить?

Честно говоря, у меня ушло два дня просто на то, чтобы заставить себя приступить к этому ревью. И это нормальная реакция для инженера; кто-то в подобной ситуации, просто не глядя, жмёт Approve, понимая, что за разумное время всё равно не сможет сделать работу по обзору этого изменения с достаточным качеством.

Но есть способ облегчить жизнь товарищу. Помимо предварительной работы по лучшей декомпозиции задачи, уже после завершения написания основного кода можно привести историю его написания в более логичный вид, разбив на атомарные коммиты с зелёными тестами в каждом: создал новый сервис и транспортный уровень для него, построил модели и написал проверку инвариантов, добавил валидацию и обработку исключений, написал тесты.
Каждый из таких коммитов можно ревьюить по отдельности (и GitHub, и GitLab это умеют) и делать это набегами в моменты переключения между своими задачами или в перерывах.

Сделать это всё нам поможет всё тот же git rebase с ключом --interactive. В качестве параметра надо передать ему хэш коммита, начиная с которого нужно будет переписать историю. Если речь о последних 50 коммитах, как в примере на картинке, можно написать git rebase --interactive HEAD~50 (подставьте вместо 50 вашу цифру).

Кстати, если вы в процессе работы над задачей подливали к себе ветку master, то сначала надо будет сделать rebase на эту ветку, чтобы merge-коммиты и коммиты из мастера не путались у вас под ногами.

Вооружившись знаниями о внутреннем устройстве git-репозитория, понять принцип действия rebase на master будет несложно. Эта команда берёт все коммиты в нашей ветке и меняет родителя первого из них на последний коммит в ветке master. См. схему:




Иллюстрации взяты из книги Pro Git

Если изменения в C4 и C3 конфликтуют, то после разрешения конфликтов коммит C4 изменит своё содержание, поэтому он переименован на второй схеме в C4.

Таким образом, вы получите ветку, состоящую только из ваших изменений, и растущую из вершины master. Само собой, master должен быть актуальным. Можно просто использовать версию с сервера: git pull --rebase origin/master (как известно, git pull равносилен git fetch && git merge, а ключ --rebase заставит git сделать rebase вместо merge).

Вернёмся наконец к git rebase --interactive. Его делали программисты для программистов, и понимая, какой стресс люди будут испытывать в процессе, постарались максимально сохранить нервы пользователя и избавить его от необходимости чрезмерно напрягаться. Вот что вы увидите на экране:


Это репозиторий популярного пакета Guzzle. Похоже, что rebase ему не помешал бы

В текстовом редакторе открывается сформированный файл. Внизу вас ожидает подробная справка о том, что тут вообще делать. Далее в режиме простого редактирования вы решаете, что делать с коммитами в вашей ветке. Всё просто, как палка: pick оставить как есть, reword поменять описание коммита, squash слить воедино с предыдущим (процесс работает снизу вверх, то есть предыдущий это который строчкой ниже), drop вообще удалить, edit и это самое интересное остановиться и замереть. После того, как git встретит команду edit, он встанет в позицию, когда изменения в коммите уже добавлены в режим staged. Вы можете поменять всё, что угодно в этом коммите, добавить поверх него ещё несколько, и после этого скомандовать git rebase --continue, чтобы продолжить процесс rebase.

Да, и кстати, вы можете поменять коммиты местами. Возможно, это создаст конфликты, но в целом процесс rebase редко обходится совсем уж без конфликтов. Как говорится, снявши голову, по волосам не плачут.

Если вы запутались и кажется, что всё пропало, у вас есть кнопка аварийного катапультирования git rebase --abort, которая немедленно вернёт всё как было.

Вы можете повторять rebase несколько раз, затрагивая только части истории, и оставляя остальные нетронутыми при помощи pick, придавая своей истории всё более и более законченный вид, как гончар кувшину. Хорошим тоном, как я уже написал выше, будет сделать так, что тесты в каждом коммите будут зелёными (для этого отлично помогает edit и на следующем проходе squash).

Ещё одна фигура высшего пилотажа, полезная в случае, если надо несколько изменений в одном и том же файле разложить по разным коммитам git add --patch. Она бывает полезна и сама по себе, но в сочетании с директивой edit она позволит вам разделить один коммит на несколько, причём сделать это на уровне отдельных строк, чего не позволяет, если я не ошибаюсь, ни один GUI-клиент и ни одна IDE.

Убедившись ещё раз, что всё в порядке, вы наконец можете со спокойной душой сделать то, с чего начался этот туториал: git push --force. Ой, то есть, разумеется, --force-with-lease!



Поначалу вы, скорее всего, будете тратить на этот процесс (включая первоначальный rebase на master) час, а то и два, если фича реально развесистая. Но даже это намного лучше, чем ждать два дня, когда ревьювер заставит себя наконец взяться за ваш реквест, и ещё пару дней, пока он сквозь него продерётся. В будущем же вы, скорее всего, будете укладываться в 30-40 минут. Особенно помогают в этом продукты линейки IntelliJ со встроенным инструментом разрешения конфликтов (full disclosure: компания FunCorp оплачивает эти продукяы своим сотрудникам).

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

Спасибо за внимание всем, кто дочитал до конца! Надеюсь, что статья будет полезна не только вам, но и коллегам, которым ваш код попадает на ревью. Если у вас есть клёвые хаки для git делитесь ими в комментариях!

points of view by sanja

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

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

Команда Mail.ru Cloud Solutions перевела статью, автор которой несколько лет занимался обнаружением типовых сбоев в коде на продакшене и изучал причины, приведшие к такому результату. В статье рекомендации по проверке кода, которые автор использует в качестве базового контрольного списка.

Удаленная система выходит из строя

Независимо от того, насколько тщательно спроектирована система, в какой-то момент произойдет сбой это факт при запуске программного обеспечения в продакшен.

Сбои случаются по разным причинам: из-за ошибок, проблем в инфраструктуре, внезапного всплеска трафика, decay of neglect, но они случаются почти всегда. От того, как вызывающие модули обрабатывают ошибки, зависит устойчивость и надежность всей архитектуры:

1. Определите путь обработки ошибок. В коде должны быть явно определены средства для обработки ошибок. Например, правильно разработанная страница ошибок, журнал исключений с метриками ошибок или автоматическое выключение с механизмом резервирования. Главное ошибки должны обрабатываться явно. Нельзя допускать, чтобы пользователи видели ошибки работы вашего кода.
2. Составьте план восстановления. Рассмотрите каждое удаленное взаимодействие в вашем коде и выясните, что нужно сделать для восстановления прерванной работы. Запускается ли рабочий процесс с точки сбоя? Публикуются ли все полезные данные во время сбоя в таблице очередей или таблице повторов? И повторяются ли каждый раз, когда удаленная система восстанавливается? Есть ли скрипт для сравнения баз данных двух систем и их синхронизации? Системный план восстановления нужно разработать и реализовать до развертывания фактического кода.

Удаленная система медленно отвечает

Эта ситуация хуже, чем полный сбой, поскольку неизвестно, работает ли удаленная система. Поэтому для обработки этого сценария всегда нужно проверять проблемы, описанные ниже.

Некоторые из проблем можно решить прозрачно для кода приложения, если использовать технологии Service Mesh, такие как Istio. Тем не менее нужно убедиться, что такие проблемы обрабатываются вне зависимости от метода:

1. Установите тайм-ауты для удаленных системных вызовов. Это касается и тайм-аутов для удаленного вызова API и базы данных, публикации событий. Проверьте, установлены ли конечные и разумные тайм-ауты для всех удаленных систем в вызовах. Это позволит не тратить ресурсы на ожидание, если удаленная система перестала отвечать на запросы.
2. Используйте повторные попытки после тайм-аута. Сеть и системы ненадежны, повторные попытки обязательное условие устойчивости системы. Как правило, повторные попытки устраняют множество пробелов в межсистемном взаимодействии.
   
   Если возможно, используйте какой-то алгоритм в ваших попытках (фиксированный, экспоненциальный). Добавление небольшого джиттера в механизм повтора даст передышку вызываемой системе, если она под нагрузкой, и приведет к увеличению скорости ответа. Обратная сторона повторных попыток идемпотентность, о которой расскажу позже в этой статье.
3. Применяйте автоматический размыкатель (Circuit Breaker). Существует не так много реализаций, которые поставляются с этим функционалом, например Hystrix. В некоторых компаниях пишут собственных внутренних обработчиков. Если есть возможность, обязательно используйте Circuit Breaker или инвестируйте в его создание. Четкая структура для определения запасных вариантов в случае ошибки хороший вариант.
4. Не обрабатывайте тайм-ауты как сбои. Тайм-ауты не сбои, а неопределенные сценарии. Их следует обрабатывать способом, который поддерживает разрешение неопределенности. Стоит создавать явные механизмы разрешения, которые позволят системам синхронизироваться в случае тайм-аута. Механизмы могут варьироваться от простых сценариев согласования до рабочих процессов с отслеживанием состояния, очередей недоставленных писем и многого другого.
5. Не вызывайте удаленные системы внутри транзакций. Когда удаленная система замедляется, вы дольше удерживаете соединение с базой данных. Это может быстро привести к исчерпанию соединений с базой данных и, как следствие, к сбою в работе вашей собственной системы.
6. Используйте интеллектуальное пакетирование. Если работаете с большим количеством данных, выполняйте пакетные удаленные вызовы (вызовы API, чтение БД), а не последовательные это устранит нагрузку на сеть. Но помните: чем больше размер пакета, тем выше общая задержка и больше единица работы, которая может дать сбой. Так что оптимизируйте размеры пакета для производительности и отказоустойчивости.

Построение системы, которую вызывают другие системы

1. Убедитесь, что все API идемпотентны. Это обратная сторона повторов при тайм-аутах API. Вызывающие абоненты могут повторить попытку, только если ваши API безопасны для повторения и не вызывают неожиданных побочных эффектов. Под API я имею в виду как синхронные API, так и любые интерфейсы обмена сообщениями клиент может опубликовать одно и то же сообщение дважды.
2. Определите SLA согласованный уровень качества предоставления услуги. Потребуется SLA для времени отклика и пропускной способности, а также код для их соблюдения. В распределенных системах гораздо лучше быстро потерпеть неудачу, чем позволить абонентам ждать.
   
   SLA с высокой пропускной способностью сложно реализовать: ограничение распределенной скорости сама по себе сложная задача. Но стоит помнить об этом и иметь возможность обрывать вызовы, если выходите за пределы SLA. Другой важный аспект знание времени отклика нижестоящих систем, чтобы определить, какую скорость ожидать от вашей системы.
3. Определите и ограничьте пакетирование для API-интерфейсов. Максимальные размеры пакетов явно определяют и ограничивают обещанным SLA это следствие соблюдения SLA.
4. Подумайте заранее о наблюдаемости системы. Наблюдаемость способность анализировать поведение системы, не обращая внимания на ее внутреннее устройство. Подумайте заранее, какие метрики и данные о системе нужны, чтобы ответить на вопросы, на которые ранее невозможно было получить ответы. И продумайте инструменты для получения этих данных.
   
   Мощный механизм разбить логику системы на домены и публиковать события каждый раз, когда в домене происходит событие. Например, получен запрос с id = 123, возвращен ответ на запрос с id =123. Обратите внимание, как можно использовать эти два доменных события, чтобы получить новую метрику под названием время отклика. Из необработанных данных следуют заранее определенные агрегации.

Общие рекомендации

1. Используйте агрессивное кэширование. Сеть нестабильна, поэтому кэшируйте столько данных, сколько возможно. Ваш механизм кэширования может быть удаленным, например сервер Redis, работающий на отдельной машине. По крайней мере, вы перенесете данные в свою область управления и уменьшите нагрузку на другие системы.
2. Определите единицу сбоя. Если API или сообщение представляет несколько единиц работы (пакет), то какова единица сбоя? В случае сбоя вся полезная нагрузка выйдет из строя или, напротив, отдельные блоки будут успешны или потерпят неудачу независимо? Отвечает ли API кодом успеха или неудачей в случае частичного успеха?
3. Изолируйте внешние доменные объекты на границе системы. Еще один пункт, который, по моему опыту, вызывает проблемы в долгосрочной перспективе. Не стоит разрешать использование доменных объектов других систем во всей вашей системе для повторного использования. Это связывает вашу систему с работоспособностью другой системы. И каждый раз, когда меняется другая система, нужно рефакторить код. Поэтому стоит создавать собственную внутреннюю схему данных и преобразовывать внешние полезные данные в эту схему. И затем использовать внутри собственной системы.

Безопасность

1. Проверяйте ввод на каждой точке входа. В распределенной среде любая часть системы может быть скомпрометирована с точки зрения безопасности или иметь ошибки. Следовательно, каждый модуль должен проверять, что поступает ему на вход. И не предполагать, что он получит чистый, то есть безопасный ввод.
2. Никогда не храните учетные данные в репозитории кода. Это очень частая ошибка, от которой трудно избавиться. Но учетные данные всегда следует загружать в среду выполнения системы из внешнего, предпочтительно защищенного, хранилища.

Надеюсь, вы найдете эти рекомендации полезными для сокращения количества часто встречающихся ошибок в коде распределенных систем.

Удачи!

Что еще почитать:

1. Как спокойно спать, когда у вас облачный сервис: основные архитектурные советы.
2. Как технический долг и лже-Agile убивают ваши проекты.
3. Наш канале в Телеграме о цифровой трансформации.

Представьте ситуацию ваше задание на работе изменить формат логирования. Сначала всё кажется просто. Ровно до того момента, пока все эти изменения не нужно вносить в 80+ микросервисах И так легкая на первый взгляд задача превращается в длинную и рутинную. Что можно с этим делать?

Или вот еще задачка какими фичами можно обеспечить скорость, качество и удобство code review?

Обо всём этом рассказывают Java-разработчики ЮMoney в своих докладах. Добавляйте в закладки или смотрите прямо сейчас. Видео с таймкодами уже ждут под катом.

---

Automate it! Внесение типовых изменений в микросервисы

Вита Комарова, старший Java-разработчик
Эксперт рассказывает, как запустить массовые изменения, поддерживать стандарты и сберечь нервы всей команды

1:03 О микросервисной архитектуре в ЮMoney и не только
2:17 Задача изменить формат логирования. Что нужно сделать?
3:00 Разделяем работу по командам: плюсы и минусы
4:02 Примеры массовых изменений
4:34 Решение автоматизация
4:47 Шаги раскатки изменений
6:05 Реализация автоматизации: о роботе Modernizer
8:09 Что такое Flow? Различные сценарии патчинга
8:58 Что умеет Modernizer? Техническая реализация
10:26 Результаты работы
10:57 Контроль патча
12:34 Путь задачи после merge
13:15 Давайте автоматизируем тестирование
14:40 Про Automerge и автоматизация релиза
16:13 История глазами разработчика
17:42 Итоги: жизнь до и после Modernizer
20:18 В чем секрет нашего успеха?
21:42 Наши выводы и советы команды ЮMoney

---

Автоматизация код-ревью. Два года спустя

Григорий Скобелев, Java-разработчик
Два года назад мы начали процесс улучшения код-ревью. Мы хотим заглянуть в прошлое и поделиться результатами работы покажем эволюцию системы код-ревью, наиболее и наименее востребованную функциональность и расскажем, как улучшенная система повлияла на нашу работу.

1:31 О бэкенде в ЮMoney, наши инструменты
3:29 О чем пойдет речь в докладе
5:12 О code review и зачем его улучшать
7:25 Код-ревью на словах и на деле. Личный опыт
10:07 Система и функциональность код-ревью. Фичи ЮMoney
10:30 Скорость
14:42 Качество
18:47 Удобство
24:22 Топ фичей в код-ревью
25:07 План доработок

---

Все доклады с большой ИТ-конференции ЮMoneyDay. На подходе материалы про SQL, DevOps, фронтенд, PM, тестирование и мобильную разработку.

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

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

Но я не хочу, чтобы инспекторы кода пылали ко мне любовью

А они всё равно будут. Смиритесь. Никто в истории человечества ещё не сетовал на смертном одре, что при жизни его слишком уж любили.

Зачем совершенствовать инспекцию кода?

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

• Ускоренный процесс передачи знаний. Если как следует подготовить набор изменений, внимание инспектора будут притягивать именно те области, в которых вам нужно расти, а не скучные стилистические огрехи. А когда вы даёте понять, что цените конструктивную критику, проверяющий начинает вкладывать больше сил в обратную связь.
• Пример для окружающих. Эффективные практики инспекции кода в вашем исполнении зададут планки для окружающих. Хорошие техники обычно перенимаются, а значит, вам будет проще, когда кто-нибудь из коллег вышлет код вам на проверку.
• Минимум конфликтов. Инспекция кода часто вызывает напряжение в команде. Если подходить к делу ответственно и сознательно, споры будут возникать реже.

Золотое правило: цените время проверяющего

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

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

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

Техники

1. Сначала просмотрите код сами

Прежде чем отсылать код коллеге, прочитайте его. Не ограничивайтесь вылавливанием ошибок представьте, что видите этот фрагмент впервые. Что в нём может сбить с толку?

По моему опыту, бывает полезно сделать перерыв между написанием и редактированием кода. Люди часто отсылают свежую порцию кода под конец дня, но именно в этот период наиболее высока вероятность что-то упустить по небрежности. Дождитесь утра, посмотрите на набор изменений свежим взглядом и тогда уж передавайте его коллеге.



Какой идиот это написал?
Синхронизация Cron Jobs с лунным циклом: Я добавил логику с синхронизацией, чтобы наш пайплайн ETL оставался в гармонии с природой

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

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

2. Составьте ясное описание набора изменений

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

И он был прав. Я писал документ с расчётом на разработчиков из своей команды, но не учёл, что его, возможно, будут читать и люди со стороны. Аудитория не ограничивалась пределами моего отдела оставались ещё партнёрские команды, наставники, люди, которые принимали решения о повышениях. Документ следовало писать так, чтобы и им всё было ясно. После этого разговора я всегда стараюсь подать свою работу в необходимом для понимания контексте.

Описание набора изменений должно включать в себя всю фоновую информацию, которая может понадобиться читателю. Даже если вы составляете описание прицельно для проверяющего, имейте в виду, что у него может быть меньше сведений о контексте, чем вам кажется. Кроме того, не исключено, что другим коллегам тоже придётся работать с этим кодом в будущем при просмотре истории изменений им должны быть понятны ваши намерения.

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

Если вы хотите глубже погрузиться в искусство составления превосходных описаний, прочитайте How to Write a Git Commit Message Криса Бимса и My favourite Git commit Дэвида Томпсона.

3. Автоматизируйте простые вещи

Если вы полагаетесь на инспектора в таких мелочах, как уплывшая фигурная скобка или проблемы с автоматическими тестами, значит тратите его время впустую.



Проверишь, всё ли там в порядке с синтаксисом? Я бы к компилятору обратился, но жаль тратить его время.

Автоматические тесты должны считаться в команде частью стандартной процедуры. Инспекция начинается только тогда, когда пройдена вся серия автоматических проверок в среде непрерывной интеграции.

Если же ваша команда пребывает в трагическом заблуждении и отказывается от непрерывной интеграции, организуйте автоматизированную проверку своими силами. Внедрите в среду разработки pre-commit хуки, линтеры и форматтеры, чтобы удостовериться, что все соглашения соблюдены и предполагаемое поведение сохраняется от коммита к коммиту.

4. Позаботьтесь о том, чтобы код сам отвечал на вопросы

Что не так на этой картинке?



Мне непонятно назначение функции.
А, это на случай если при вызове передаётся Frombobulator при отсутствии имплементации frombobulate

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

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



Алло?
Когда ты шесть лет назад писал bill.py, почему у тебя t=6?
Рад, что ты позвонил! Это из-за налога на продажи в 6%.
Ну разумеется!
Отличный способ обосновать решение по реализации.

Лучший ответ на любой вопрос это рефакторинг кода, который его снимает. Что можно переименовать, чтобы стало понятнее, как изменить логику? Комментарии приемлемое решение, но они однозначно проигрывают на фоне кода, который естественным образом сам себя документирует.

5. Вводите изменения дробно

Разрастание границ это дурная практика, которую часто можно наблюдать при инспекции кода. Разработчик берётся за устранение бага в логике, но по ходу дела ему на глаза попадается какой-то изъян в интерфейсе. Ну, раз уж всё равно с этим куском работаю, думает он, заодно и его исправлю. А в итоге начинается путаница. Проверяющему теперь придётся вникать, какие изменения работают на задачу А, а какие на задачу Б.

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

6. Отделяйте функциональные изменения от нефункциональных

Из ограничения масштабов следует другое правило функциональные и нефункциональные изменения не должны смешиваться.

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



А вы найдёте функциональное изменение?

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

Вдобавок разработчики любят несвоевременно примешивать к основным изменениям рефакторинг. Рефакторинг кода со стороны коллег я всячески одобряю, но не когда они параллельно реализуют новое поведение.



Изменение в поведении, погребённое среди рефакторинга

Если фрагмент кода нуждается и в рефакторинге, и в изменениях поведения, необходимо разбить процесс на два-три набора:

1. Добавить тесты для текущего поведения (если их нет)
2. Провести рефакторинг кода, ничего не меняя в тестах
3. Изменить поведение и соответствующим образом обновить тесты

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

7. Разбивайте слишком крупные наборы изменений

Раздутые наборы изменений это что-то вроде страшненьких родственников разрастающихся границ. Представьте себе ситуацию: разработчик приходит к выводу, что для внедрения функциональности А ему сначала придётся подкорректировать семантику библиотек Б и В. Если изменений немного, то ещё куда ни шло, но очень часто эти модификации расползаются и на выходе получается огромный список изменений.

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

Может, вместо того чтобы делать всё разом, удастся сначала поработать с зависимостями, а уже в следующем наборе реализовать новую функциональность? Реально ли сохранить код в здравом состоянии, если внедрить половину функциональности сейчас, а другую половину на следующем этапе?

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

8. Принимайте критику доброжелательно

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

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

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



Для января и февраля 1900 года это не сработает.
Точно, хорошо подмечено!

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

9. Проявляйте терпение, если инспектор допускает ошибку

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

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



Здесь переполнение буфера, потому что не проводилась верификация, хватит ли памяти в name, чтобы вместить все символы NewNameLen.
Это в моём-то коде? Немыслимо! Конструктор вызывает PurchaseHats, а она вызывает CheckWeather, которая выдала бы ошибку, если бы длина буфера не соответствовала. Ты бы сначала прочитал 200 000 строк кода, а потом уже осмеливался допускать возможность, что я где-то ошибся.

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

10. Давайте доходчивую реакцию

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

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



Исправляю заголовки > Обновляю код в соответствии с замечаниями > Всё готово, посмотри, пожалуйста

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



Некоторые инструменты вроде Reviewable и Gerritt предлагают ставить метки на обработанные замечания

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

11. Умело запрашивайте недостающую информацию

Иногда комментарии инспектора оставляют слишком много возможностей для толкования. Когда тебе пишут что-то в духе Мне непонятна эта функция, сразу возникает вопрос, что именно человеку непонятно. Слишком длинная? Название неудачное? Нужно лучше задокументировать?

Долгое время я ломал голову, как внести ясность в туманные комментарии и при этом ненароком не вступать в конфронтацию. Первое, что приходило в голову спросить: Что в ней непонятного?, но это звучит сварливо.

Как-то раз я умышленно отправил неопределённую реплику такого рода коллеге, и он обезоружил меня своим ответом:

Что изменить, чтобы стало лучше?

Эта фраза нравится мне тем, что выражает открытость к критике и отсутствие враждебности. Теперь, когда я получаю туманную обратную связь, то отвечаю какой-нибудь вариацией базового мотива Что изменить, чтобы стало лучше?.

Другая полезная техника попытаться угадать, что имел в виду проверяющий, и взять на себя инициативу, исправив код соответствующим образом. Если вы получили замечание в духе непонятно, просмотрите код повнимательнее. Обычно что-нибудь, что можно улучшить в плане ясности, находится. Правки дают инспектору понять, что вы готовы вносить изменения, пусть даже он представлял себе итоговый результат иначе.

12. Отдавайте преимущество проверяющему

В теннисе, если не совсем ясно попал ли мяч в аут при подаче противника, принято трактовать ситуацию в его пользу. При инспекции кода тоже следует придерживаться этого правила.

Некоторые решения в разработке дело вкуса. Если проверяющий считает, что вашу функцию в десять строк лучше бы разделить на две пятистрочных, объективная истина не принадлежит ни вам, ни ему. Какой вариант лучше зависит от предпочтений.

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

13. Сокращайте время передачи кода

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

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



Интеллектуальные затраты на инспекцию кода при коротких (слева) и долгих (справа) сроках передачи
Ось x время; ось y память инспектора; синяя штриховка восстановление контекста; голубая заливка проверка кода; серая заливка ожидание; красная штриховка дополнительные издержки

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

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

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

В заключение

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

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

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

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