пятница, 25 октября 2013 г.

[prog.flame] Почти что о сортах говна. Ну ли о запахах кода, если выражаться политкорректно :)

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

Итак, пойдем по порядку возрастания потенциальной опасности. Т.е. сначала описываются самые безобидные намеки, а в конце те, которые наверняка связаны с серьезными проблемами.

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

layer_handler_info_t
layer_handler_manipulator_t::query_layer_handler_info() const
{
   return layer_handler_info_t( 
      m_ref_dll->query_file(),
      query_registration_name(),
      m_used_cfg_name,
      state_to_string( m_reg_state ),
      m_reg_state );
}

Однако, если этот стиль нарушается не понятным для меня образом:

layer_handler_info_t
layer_handler_manipulator_t::query_layer_handler_info() const
{
   return layer_handler_info_t( 
      m_ref_dll->query_file (),
      query_registration_name() , m_used_cfg_name,
      state_to_string(m_reg_state ),
      m_reg_state);
}

то это заставляет насторожиться. Конкретно в этом примере: почему-то перед скобками при вызове query_file поставлен пробел, которого нет в других случаях; два аргумента указано на одной строке, хотя другие аргументы размещаются каждый на своей строке; почему-то после query_registration_name() перед запятой стоит пробел; перед аргументом m_reg_state функции state_to_string нет пробела, а после, почему-то, есть.

Все это, конечно, смахивает на grammar nazy ;) Но для меня это признак того, что над кодом либо работали разные люди (и тогда есть шанс, что кто-то чего-то мог не знать), либо же код правился в очень большой спешке (что чревато наличием глупых, но труднообнаруживаемых ошибок).

Выполнение одних и тех действий разными способами. Например, пусть в C++ есть класс, который обеспечивает защиту от многопоточности захватом в начале каждого публичного метода специального мутекса. Если в части методов этот мутекс захватывается одной конструкцией, а в другой части методов иной конструкцией, то это подозрительно. Например:

void
task_queue_t::push( ref_task_t && task )
{
   ACE_Guard< ACE_Thread_Mutex > lock( m_queue_mutex );
   ...
}

void
task_queue_t::push( task_vector_t && task_vector )
{
   ACE_GUARD( ACE_Thread_Mutex, lock, m_queue_mutex );
   ...
}

bool
task_queue_t::pop( task_vector_t & task_vector )
{
   ACE_Guard<ACE_Thread_Mutex> lock( m_queue_mutex );
   ...
}

Настороженность вызывает тот факт, что в одном из push-ей вместо явного объявления guard-переменной зачем-то использован макрос. Почему? Может макрос делает что-то большее, что не может сделать обычная переменная и для этого варианта push-а это очень важно. Но раз это так важно в одном push-е, то почему макрос не используется во втором push-е?

Наличие кусков закомментированного кода. Особенно если не описаны причины того, почему этот код закомментирован, но оставлен в исходнике. Т.е. когда я вижу что-то вроде:

void
task_processor_t::do_dereg_coop(
   const std::string & coop_name )
{
   ACE_Guard< ACE_Thread_Mutex > guard( m_lock );

   std::string error_desc;

/*
   m_logger->on_dereg_coop(
      false,
      coop_name,
      START_OPERATION_STAGE,
      error_desc );
*/

   try
   {
      initiate_coop_dereg_operation( coop_name );
   }
   catchconst std::exception & x )
   {
      error_desc = x.what();
   }

/*
   m_logger->on_dereg_coop(
      false,
      coop_name,
      FINISH_OPERATION_STAGE,
      error_desc );
*/
}

То начинаю очень сильно подозревать, что код кем-то был не доведен до ума. Но сколько еще осталось сделать (и что именно еще осталось сделать), все это мне неизвестно.

Несоответствие комментариев и кода. Тут все очевидно, если я вижу комментарий, который расходится с последующим кодом, то значит, что в Интернетах кто-то неправ :)

//! Класс для добавления слоев в SObjectizer.
class SO_SYSCONF_4_TYPE layer_handler_t : public cpp_util_2::nocopy_t
{
   public :
      layer_handler_t(
         //! Имя кооперации.
         const std::string & layer_name );
      virtual ~layer_handler_t();

      //! Получить имя слоя.
      /*!
         \return Полученное в конструкторе имя кооперации.
      */
      const std::string &
      query_name() const;

      //! Добавить слой в SObjectizer.
      virtual void
      add(
         //! SObjectizer Environment с которым нужно работать.
         so_5::rt::so_environment_t & env,
         //! Конфигурационный файл для кооперации.
         const std::string & cfg_file ) = 0;

   private :
      //! Имя кооперации.
      const std::string m_layer_name;
};

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

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

Очень большие по объему функции/методы. Ну это классика. Чем больше кода в функции/методе, тем больше шансов, что какая-то ситуация в функции не обработана.

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


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

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

Такие дела. Может кому-то это поможет внимательнее относиться к написанию своего или сопровождению чужого кода.

Комментариев нет: