Предисловие

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

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

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

Буду очень сильно давить в сторону ruby/rails из-за того, что этими инструментами я пользуюсь постоянно.

Заранее должен предупредить, что никаких книг на тему я не читал, так что смело можно это ставить мне в вину. Всё “на живом опыте”. Ну разве что когда-то давно читал “Чистый код” дядюшки Боба.

По теме “поддерживаемого кода” рекомендую всем посмотреть доклад “Ментальное программирование”. Очень многие доводы в этом посте взяты из этого доклада (и личного опыта).

Цель код-ревью

Основной целью код-ревью я считаю поддерживаемость кода. Но не его работоспособность, которую должен обеспечивать сам разработчик и отдел QA/QC (кстати, кто-то понимает разницу?).

Задачи код-ревью

Соответственно, задачами код-ревью являются:

  1. Обеспечение читаемости кода
  2. Обеспечение модифицируемости кода

Что я ищу во время код-ревью

Корректное и полное именование

Когда-то фразу “работа программиста — придумывать названия переменным” считал шуткой. Но давно уже перестал. А всякие “как корабль назовёшь — так он и поплывёт”… Не буду продолжать.

А суть-то проблемы в следующем:

  1. Не надо стесняться длинных названий. Предел длины строки от 80 до 120 символов (в зависимости от соглашений), из которых можно где-то треть (30-40 символов) потратить на название переменной. И это нормально! Надо в названии переменной написать целое предложение — пишите. Лучше написать больше, чем меньше. Прямо сейчас. Не потом. Даже не через пять минут.
  2. Не надо бояться тратить время на придумывание названий. Это тоже работа программиста. Я бы даже сказал, что называть вещи своими именами — важнейший навык, который нужно применять везде и всегда.
  3. Нет нормальных однобуквенных названий. И не может быть. Даже счётчики i — это отстой. Их надо называть нормально.

Приведу два примера.

Пример про наименование переменной

Вот за такое я не очень люблю императивный подход:

@users = User.all
@users = @users.map(:name)

Я сторонник того, что нужно стараться работать с переменными так, будто они иммутабельны. И придумывать новые названия для новых значений, а не переписывать переменную по 10 раз. Нет, конечно ruby — не clojure. Но никто же не запрещает писать в иммутабельном стиле хоть на каком-то уровне?

Например, так:

@users = User.all
@user_names = @users.map(:name)

Почему это проблема именования? Потому что причина переназначения переменной чаще всего — лень и нежелание придумывать новые названия.

Пример про наименование теста

test 'should not allowed' do

Все поняли, о чём это? Вот и я не понял. А подразумевали на самом деле вот это:

test 'should not let User with role "guest" create and update post' do

Тесты

Несмотря на то, что я писал в посте про TDD, тесты всё-таки должны проверять корректность программы.

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

Поэтому я предпочитаю проверить наличие тестов (хоть каких-нибудь), и их содержимое.

Основных критерия проверки конкретного теста два: хрупкость и осмысленность.

Что я считаю хрупкими тестами? Ну, например, проверка вёрстки при рендере. И вообще, всё, что может поменяться не сломав при этом логику приложения. Вроде http-заголовков (кроме некоторых случаев).

Осмысленность? Например тестирование стабов или mock-объектов. Ну и, конечно, тестирование библиотечного кода. Регулярно находятся люди (включая меня когда-то), которые любят потестировать работу валидаций в рельсовых моделях.

Соблюдение модульности и уровней абстракции

Пропустим любителей написания всего кода приложения или всех тестов в одном файле.

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

Например, сырые sql-запросы в контроллере — сомнительная практика. Как и http-запросы в рельсовой модели.

Многословность

Пока лень описывать этот пункт. Хотя он — важнейший из списка (после, конечно, наименований) и при этом самый неочевидный. Потому и объяснять сложно.

Отсутствие сомнительных практик

Мета-программирование а-ля ruby

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

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

Макросы

Как-то я пробовал ковырять гниющий труп common-lisp’а. И решил я посмотреть содержимое библиотеки одной.
Зачем-то мне нужно было понять, как конкретная функция работает. Так вот на разбор макроса в три строки (вместе с названием) у меня ушло полтора часа. Я понимаю, что это можно было сделать быстрее при наличии интеллекта. Но зачем? Гораздо проще было бы отказаться от макросов совсем. А если это невозможно, то делать их максимально топорными.

Та же история у меня приключилась, когда я полез смотреть кишки Rails эликсировского Phoenix’а. Я был очень огорчён объёмными макросами, которые ссылались друг на друга.

Императивщина

some_variable = 1

if some_condition
  some_variable = 2
else
  some_variable = 3
end

return some_variable

Без комментариев.

ООП

ООП? В 2k17? Серьёзно?

Во-первых, ООП сосёт. Во-вторых ООП тоже сосёт. А в третьих…

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

Так что обычно я стараюсь остановить любые попытки запиливать лютое ООП в проекте.

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

Злоупотребление ФП

Ну здравствуйте, дороие мои любители анонимных функций. Как вам такое, а?

maybe_iteration(some_variable, anonymous_function1(params1) {
  something(anonymous_function2(params2) {
    something_else(anonymous_function3(params3) {
      get_some_result(params1, params2, params3);
    });
  });
});

Ну и примерный аналог для рубистов:

some_variable.something do |param1|
  param1.something_else {|param2| param2.what_is_this?}
end.another_fancy_method {|param3| param3.is_this_a_good_code?}

Понравилось? Вот и мне не очень.

Неявные code-conventions

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

Отсутствие дублирования

Немного холиварная тема. Но всё же.

Есть два уровня дублирования, которые можно опустить: дублирование на уровне кода и дублирование на уровне логики.

Дублирование на уровне кода

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

Конечно, некоторые считают погоню за дублированием “предварительной оптимизацией”. Я сторонник “лучше сейчас вынести, чем потом где-то пропустить”.

Дублирование на уровне логики

Вот тут всё не так очевидно. Хотя проблему видел несколько раз.

Простой (утрированный) пример на уровне Rails:

# app/models/post.rb

class Post < ApplicationRecord

  validates :title, presence: true
end

# app/controllers/posts_controller.rb

class PostsController < ApplicationController
  def create
    @post = Post.new(params)

    if @post.title.present? && @post.save
      redirect_to post_path(@post)
    else
      render :new
    end
  end

Явное дублирование логики. Ведь мы уже поставили валидацию в модели. Зачем ещё и в контроллере проверять?

Да, пример высосан из пальца, но аналогии провести не составит труда.

Отсутствие ненужных велосипедов

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

А суть проста: если есть готовое решение (библиотека или даже готовое приложение), которое будет дешевле и быстрее, чем велосипед, то зачем пилить велосипед?

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

Нужно ли ревьюить собственный код?

Да. Определённо.

Конечно, “сам себе ревьюер” — не решение проблемы собственного говнокода. Но это даёт возможность неплохо пошевелить шестерёнками в голове. Ну и всё-таки отсеивает часть говнокода.

Методика определения корректности кода

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

  1. Проверяю код на соответствие всем пунктам (см. выше)
  2. “Компилирую” его в голове. Как бы мне ни хотелось, чтобы код был безошибочным, иногда избавиться от обидных ошибок можно только так.
  3. Пытаюсь понять, почему код был написан именно так, если что-то зацепило мой взгляд. Ведь, возможно, для конкретного костыля были причины. Особенно сложно выполнять этот пункт во время ревью своего кода.