Code Review

Вы пишите код для разработчиков или только для себя?

 

Обзоры кода во имя добра

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

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

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

 

Ваш код — вам не принадлежит

Вы когда-нибудь задумывались о том, кто будет читать ваш код? Насколько сложно им будет разобраться в вашем коде? Вы задумываетесь о читабельности своего кода?

Любой дурак может написать код, понятный машине. Хороший программист ― пишет код, понятный человеку. ― Martin Fowler

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

Недавно мне попался такой код:

defmodule Util.Combinators do
  def then(a, b) do
    fn data -> b.(a.(data)) end
  end

  def a ~> b, do: a |> then(b)
end

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

import Util.{Reset, Combinators}

# ...

conn = conn!()

Benchee.run(
  # ...
  time: 40,
  warmup: 10,
  inputs: inputs,
  before_scenario: do_reset!(conn) ~> init,
  formatter_options: %{console: %{extended_statistics: true}}
)

Я добавил не только символ ~>, но и функции conn!/0 и do_reset/1. Давайте посмотрим на модуль Reset:

defmodule Util.Reset  do
  alias EventStore.{Config, Storage.Initializer}
  
  def conn! do
    {:ok, conn} = Config.parsed() |> Config.default_postgrex_opts() |> Postgrex.start_link()
     conn
  end
  
  def do_reset!(conn) do
    fn data ->
      Initializer.reset!(conn)
      data
    end
  end
end

Касательно conn!, есть пара способов сделать проще, но и сейчас это понятно написано, поэтому я не буду придираться. Чего не скажешь о do_reset!/1. У неё даже имя сложное. Это функция, которая возвращает функцию, которая возвращает свой аргумент, а ещё сбрасывает Initializer.

 

Я решил разобраться. Согласно документации benchee, хук before_scenario принимает входные данные сценария в качестве аргумента. Возвращаемое значение становится входным для следующих шагов. Вероятно, автор задумывал следующее:

  1. Инициализировать Postgrex соединение
  2. Сбросить EventStore
  3. Принять входящие значения в качестве конфигурации (количество аккаунтов)
  4. Подготовить данные для тестирования (например, создать пользователей, залогинить их в приложении)
  5. Запустить тест

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

Первым шагом было сделать выполнение алиасов явным, вместо неявного импорта. Мне никогда не нравились магические функции в моём коде, даже импорт Ecto.Query, делающий запросы действительно элегантно. Теперь модуль Connection выглядит вот так:

defmodule Benchmarks.Util.Connection do
  alias EventStore.{Config, Storage.Initializer}

  def init! do
    with {:ok, conn} =
           Config.parsed()
           |> Config.default_postgrex_opts()
           |> Postgrex.start_link() do
      conn
    end
  end

  def reset!(conn),
    do: Initializer.reset!(conn)
end

Далее я хотел написать хук, в соответствии с документацией:

before_scenario: fn inputs -> inputs end

Теперь осталось подготовить данные. Финальный результат:

alias Benchmarks.Util.Connection

conn = Connection.init!()

# ...

Benchee.run(
  inputs: inputs,
  before_scenario: fn inputs ->
    Connection.reset!(conn)

    init.(inputs)
  end,
  formatter_options: %{console: %{extended_statistics: true}}
)

Connection.reset!(conn)

Стал ли этот код идеальным? Вероятно, нет. Стал ли он понятней? Надеюсь, что да. Можно ли было написать так с самого начала? Определённо!

В чем твоя проблема чувак?

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

Тот код был рабочим, в этом и есть моя проблема. У меня не было причин его переписывать, кроме того, что он был усложнён, а это можно сказать ― мое субъективное мнение. Чтобы переубедить разработчика, он должен тоже не понимать код и не иметь возможности применять его в своих проектах. А если нужно убеждать бизнес, я могу только сказать, что не понимаю этот код, но скорее всего мне ответят, что я плохой разработчик ¯\_(ツ)_/¯.

 

Это (не) ошибка менеджмента

Все знают, что бизнес интересует результат. Чем дешевле и быстрее, тем лучше. На разработчиков оказывают давление. Для менеджера, как правило, разработка ПО — это сроки, бюджет и скорость. Это не обвинение, я только пытаюсь объяснить откуда берётся плохой код. Менеджерам, чаще всего, нет дела до качества кода (по крайней мере, напрямую). Бизнес интересует найти большого клиента, провести сделку, сократить расходы и сдать проект ещё вчера.

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

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

 

Быстрые правки, быстрые победы?

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

Роль эмпатии

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

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

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

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

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

 

Заключение

Недавно я написал такой код на Elixir:

result = calculate_results()
Connection.close(conn)
result

Я тут же подумал о методе tap в Ruby, который помог бы мне переписать этот код:

calculate_result().tap do
  Connection.close(conn)
end

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

with result = calculate_results(),
     Connection.close(conn),
  do: result

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

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

Всё что я хотел вам сказать в этой статье: Пожалуйста, пожалуйста всегда думайте о других, когда пишите свой код.

 

Перевод статьи Kamil Lelonek: How to write code which others will understand?