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