[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++ (хотя этот опыт позволяет тонко чувствовать оттенки запахов известной субстанции).

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

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

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

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

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