четверг, 11 марта 2021 г.

[prog.c++] Наглядно про "могу помочь вашей команде улучшить качество кода"

В связи с поиском подработок за последнее время мне несколько раз довелось отвечать на вопросы, суть которых сводилась к следующему: "а чем вы можете оказаться полезны нашей команде?"

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

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

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

Первый фрагмент. Один из конструкторов самописного умного указателя, одной из фишек которого является приведение типов с использованием собственного механизма RTTI (штатный RTTI C++ не задействуется вообще):

// Example: B::ptr b(std::move(a));
template <class T1> Ptr(Ptr<T1>&& p) {
  if (!p) {
    ptr_ = nullptr;
    return;
  }
  T* ptr = static_cast<T*>(p.ptr_->DynamicCast(T::kId));
  if (ptr) {
    ptr_ = ptr;
    p.ptr_ = nullptr;
  } else {
    ptr_ = nullptr;
    // Can't cast to this pointer so just release the original pointer.
    p.Release();
  }
}

В чем здесь проблема?

Проблема в избыточной сложности и объеме этого конструктора. Слишком много кода для выполнения, в общем-то, простого действия. А сложный и объемный код со временем становится проблемой, т.к. лет через 5-10 после его написания какому-то новому человеку на проекте придется этот код сопровождать и вероятность что-либо поломать неизбежно устремится к единице.

Как мне представляется, все тоже самое может быть записано гораздо компактнее (спасибо ув.тов.svr за указание на ошибку в комментарии):

// Example: B::ptr b(std::move(a));
template <class T1> Ptr(Ptr<T1>&& p)
  : ptr_(p ? static_cast<T*>(p.ptr_->DynamicCast(T::kId)) : nullptr)
{
  if (ptr) {
    p.ptr_ = nullptr;
  }
}

И это все.

Можно, конечно, перестраховаться и вручную вызвать p.Release(), если по каким-то причинам это очень важно в сценариях использования такого специализированного умного указателя:

// Example: B::ptr b(std::move(a));
template <class T1> Ptr(Ptr<T1>&& p)
  : ptr_(p ? static_cast<T*>(p.ptr_->DynamicCast(T::kId)) : nullptr)
{
  if (ptr) {
    p.ptr_ = nullptr;
  }
  else {
    // Can't cast to this pointer so just release the original pointer.
    p.Release();
  }
}

Но даже такой вариант существенно проще и понятнее, чем исходный.

Второй фрагмент несколько серьезнее. От моих предложений по поводу первого фрагмента запросто можно отмахнуться "оно же работает!" и такому аргументу сложно что-то весомое противопоставить. Действительно, проблемы могут быть, а могут и не быть. Но, даже если они и будут, то где-то в будущем, до которого еще дожить нужно.

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

Итак, смотрим:

Obj::Registry<void>::first_release_ = false;
is >> *this;
Obj::Registry<void>::first_release_ = true;

Это кусочек кода по десериализации объекта. И проблема здесь в том, что из operator>>() может выскочить исключение. А когда оно выскочит, значение first_release_ остается не таким, каким оно должно было бы быть.

Хуже всего с этим фрагментом то, что никаких исключений в нем не будет возникать годами. А в какой-то прекрасный момент Хоп! И непойми какое поведение. Которое затем еще и фиг повторишь. Такой себе Багсон Хиггса в чистом виде.

Исправить можно несколькими способами. Самый тривиальный (и, вероятно, самый неэффективный):

try {
  Obj::Registry<void>::first_release_ = false;
  is >> *this;
  Obj::Registry<void>::first_release_ = true;
catch(...) {
  Obj::Registry<void>::first_release_ = true;
  throw;
}

Можно сделать класс, который будет изменять содержимое first_release_ в RAII стиле:

struct FirstReleaseFlagSentinel {
  FirstReleaseFlagSentinel() {
    Obj::Registry<void>::first_release_ = false;
  }
  ~FirstReleaseFlagSentinel() {
    Obj::Registry<void>::first_release_ = true;
  }
};
...
{
  FirstReleaseFlagSentinel sentinel;
  is >> *this;
}

Можно использовать какую-то реализацию SCOPE_GUARD/finally:

{
  Obj::Registry<void>::first_release_ = false;
  const auto sentinel = finally([]() { Obj::Registry<void>::first_release_ = true; });
  is >> *this;
}

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

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


Ну а теперь главное: заглядывая в чужой код я обнаруживаю подобные проблемные места не потому, что я какой-то супер внимательный или шибко умный. И не потому, что у меня за плечами почти 30 лет работы с C++ (хотя этот опыт позволяет тонко чувствовать оттенки запахов известной субстанции).

Все гораздо проще: со стороны виднее. Действительно виднее.

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

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

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

Только и всего.

2 комментария:

svr комментирует...

В первом коде после исправления отсутствует проверка на p (видимо, != nullptr). В результате при p.ptr->... может произойти беда.

eao197 комментирует...

@svr:

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