Критерии оценки качества кода при ревью

По каким характеристикам, ревьюер понимает, что данный код - хороший, а этот код - плохой?

Ревьюер оценивает код по совокупности характеристик: читаемость, поддерживаемость, соответствие стандартам проекта, эффективность, тестируемость и безопасность

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

Главный критерий — минимальное количество строк кода; чем короче решение, тем оно лучше.

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

Теория: что такое code review и “хороший код”

Code review — это проверка изменений в коде другим разработчиком (или несколькими) до слияния в основную ветку, чтобы снизить риск ошибок и улучшить качество решения.

Под “хорошим кодом” обычно понимается код, который:

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

“Плохой код” чаще всего проявляется так:

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

Основные критерии ревью

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

Критерий	Что означает простыми словами	Что обычно проверяется
Корректность	Решение делает именно то, что требуется	Крайние случаи, ошибки, соответствие требованиям, отсутствие регрессий
Читаемость	Код легко понять	Имена, структура, понятные шаги, отсутствие “загадок”
Поддерживаемость	Код легко менять	Разделение ответственностей, слабая связанность, отсутствие дублирования
Соответствие стандартам проекта	Код “как принято” в команде	Линтер, форматтер, архитектурные правила, единый стиль
Тестируемость	Поведение можно проверять автоматически	Наличие и качество тестов, изоляция логики, контролируемые зависимости
Безопасность	Нет очевидных уязвимостей	Валидация ввода, безопасная работа с данными, отсутствие секретов
Эффективность	Достаточно быстро и экономно	Нет явных “тяжелых” мест, но оптимизация делается по измерениям

Фиксация на одной метрике (только скорость, только длина, только новизна) обычно ухудшает остальные качества и повышает риск ошибок в будущем.

Почему варианты 2–4 ошибочны

Почему “всегда главное — производительность” неверно

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

Типовой безопасный порядок работы с производительностью:

Сначала обеспечить корректное поведение и тесты (чтобы оптимизацией не сломать смысл).
Затем измерить проблему (профилирование, метрики времени ответа, нагрузочное тестирование).
После этого оптимизировать конкретное узкое место, а не “все подряд”.
Сравнить результаты измерениями до/после.

Почему “меньше строк — лучше” неверно

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

Однострочный пример “плохо читаемо”: const ok = a && b && (c ? f(x) : g(y)) && !err;
Однострочный пример “лучше по смыслу”: const isValid = hasAuth && hasData && isAllowed && !hasError;

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

Почему “самое новое — значит хорошее” неверно

Новые технологии полезны, когда решают конкретную проблему и подходят проекту, но могут:

Повысить сложность и порог входа для команды.
Усложнить сборку/деплой и поддержку.
Привести к несовместимости с окружением или библиотеками.
Ввести “разнородность” в кодовой базе, если остальные части проекта написаны иначе.

Критерий “уместность” важнее “новизны”: одинаково хорошие решения могут существовать на разных версиях синтаксиса и с разными библиотеками.

Практика: как выглядит “плохой” и “хороший” код

Ниже показан типичный пример из веб-разработки: обработка входных данных и выполнение запроса. Цель — увидеть, как проявляются критерии (понятность, тестируемость, безопасность).

Пример 1: плохой код (смешаны ответственности)

// Пример условного Node.js/Express обработчика (плохо)
app.post("/search", async (req, res) => {
  // 1) Нет явной валидации входных данных
  const q = (req.body && req.body.q) || "";

  // 2) Бизнес-логика, SQL и формат ответа смешаны в одном месте
  // 3) Потенциальная проблема безопасности: конкатенация строки запроса
  const sql = "SELECT * FROM products WHERE name LIKE '%" + q + "%'";

  try {
    const rows = await db.query(sql);

    // 4) Неочевидные преобразования и “магия”
    res.json({ ok: 1, data: rows, count: rows.length, ts: Date.now() });
  } catch (e) {
    // 5) Ошибка теряет контекст, контракт API непоследователен
    res.status(500).send("error");
  }
});

Почему это считается плохим решением на ревью:

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

Пример 2: хороший код (разделены обязанности)

// TypeScript-подход: разделение на функции и явные контракты

type SearchRequest = { q: string };
type Product = { id: string; name: string };

function parseSearchRequest(body: unknown): SearchRequest {
  // Минимальная ручная валидация для примера
  if (!body || typeof body !== "object") return { q: "" };

  const q = body.q;
  if (typeof q !== "string") return { q: "" };

  return { q: q.trim() };
}

function buildSearchParams(q: string) {
  // Подготовка параметров отдельно от SQL
  // Конкретный синтаксис параметров зависит от драйвера
  return { like: `%${q}%` };
}

async function searchProducts(db: any, q: string): Promise<Product[]> {
  const params = buildSearchParams(q);

  // Параметризация вместо конкатенации
  const sql = "SELECT id, name FROM products WHERE name LIKE :like";

  const rows = await db.query(sql, params);
  return rows;
}

app.post("/search", async (req, res) => {
  const { q } = parseSearchRequest(req.body);

  try {
    const products = await searchProducts(db, q);
    res.json({ ok: true, items: products, total: products.length });
  } catch (e) {
    // Единый формат ошибок и корректный статус
    res.status(500).json({ ok: false, error: "internal_error" });
  }
});

Почему это ближе к хорошему коду:

Читаемость: каждая функция делает одну понятную вещь.
Поддерживаемость: изменение одной части меньше влияет на другие.
Тестируемость: parseSearchRequest и buildSearchParams легко тестируются без HTTP и базы.
Безопасность: параметризация и нормализация ввода снижают риск ошибок.

Пример 3: минимальные тесты (проверка контракта)

// Пример unit-тестов (псевдо Jest/Vitest)

describe("parseSearchRequest", () => {
  test("returns empty query for invalid body", () => {
    expect(parseSearchRequest(null).q).toBe("");
    expect(parseSearchRequest(123 as any).q).toBe("");
    expect(parseSearchRequest({ q: 1 } as any).q).toBe("");
  });

  test("trims query", () => {
    expect(parseSearchRequest({ q: "  phone  " }).q).toBe("phone");
  });
});

describe("buildSearchParams", () => {
  test("wraps q with percent for LIKE", () => {
    expect(buildSearchParams("abc")).toEqual({ like: "%abc%" });
  });
});

Тесты — это часть продукта: чем проще и надежнее тесты, тем меньше стоимость поддержки и тем выше уверенность в изменениях.

Схема мышления ревьюера

Изменение (PR)
   |
   v
[1] Понимание: ясно ли, что делает код?
   |
   +-- нет -> запрос на упрощение/переименование/рефакторинг
   |
   v
[2] Корректность: нет ли ошибок и крайних случаев?
   |
   v
[3] Дизайн: правильно ли выделены модули и ответственности?
   |
   v
[4] Тесты: есть ли тесты, которые упадут при поломке?
   |
   v
[5] Безопасность: нет ли очевидных уязвимостей и утечек?
   |
   v
[6] Эффективность: достаточно ли быстро по требованиям/метрикам?
   |
   v
Approve / Request changes

Итого: хороший код определяется совокупностью характеристик (корректность, читаемость, дизайн, тестируемость, безопасность и согласованность со стандартами), а подходы “главное — скорость”, “главное — меньше строк” и “главное — самое новое” подменяют качество одной упрощенной идеей и часто ухудшают поддержку.