По жизни получилось, что я около года не программировал. Пару месяцев назад вернулся к этому занятию. Оказалось, что руки-то еще помнят. Хотя мозги на начальственный лад очень сильно перестроились и думаю я теперь несколько иначе. Что приводит к интересным эффектам. Например, перелопачивая тоны старого кода (как своего, так и чужого), быстро стали формироваться некоторые внешние признаки, которые прозрачно намекают на наличие проблем. Причем поначалу смотришь в код, вроде все нормально. Но когда копнешь поглубже, то говнище, оказывается еще то. И как раз описанные ниже запахи признаки для меня теперь являются указанием на необходимость более тщательного и внимательного разбирательства с кодом.
Итак, пойдем по порядку возрастания потенциальной опасности. Т.е. сначала описываются самые безобидные намеки, а в конце те, которые наверняка связаны с серьезными проблемами.
Непоследовательность в оформлении кода. Обычно программист пишет в каком-то устоявшемся стиле. Не важно, нравится ли он мне или нет, но если стиль устоялся, то в нем прослеживается строгая последовательность и, временами, логичность ;) Но когда эта последовательность нарушается, то это плохой знак. Например, обычно разработчики оформляют передачу аргументов в функцию в каком-то одном стиле. Скажем, как-то так:
|
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 ); } catch( const 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% указывает, что за внешне рабочим кодом, в котором встречается что-то из перечисленного (особенно, когда там есть все из вышеуказанного), скрываются дефекты разной степени серьезности.
Полагаю, что все это говорит о недостаточно тщательном отношении к коду. Т.е. разработчику просто было не до того, чтобы спокойно заниматься кодом и аккуратно доводить его до ума. Более того, зная, в каких условиях создавались куски, примеры из которых я (утрируя и преувеличивая) преобразовывал в показанные выше иллюстрации, я уверен, что все это следствие сильно сжатых сроков и административного давления на разработчиков. Но такие условия, к сожалению, ведут не только к наличию тупых ошибок в коде (вроде забытого присваивания или не закрытого вовремя файла). Гораздо хуже, что такие условия провоцируют серьезные архитектурые или алгоритмические ошибки. Поэтому погружение в неряшливо оформленный код заставляет меня выискивать и, к сожалению, находить более серьезные и опасные дефекты.
Такие дела. Может кому-то это поможет внимательнее относиться к написанию своего или сопровождению чужого кода.
Комментариев нет:
Отправить комментарий