[prog] Наиболее часто встречающиеся проблемы в C++ коде, на которые мне приходится указывать

Одной из моих обязанностей на работе является проведение code review для кода, который выдают “на гора” мои подчиненные. Некоторые проблемы встречаются настолько часто, что характерны практически для всех C++ных программистов с небольшим опытом. Под кат я поместил описания наиболее серьезных проблем, но в случайном порядке, без попытки отранжировать их.

Использование длинных шаблонных конструкций без typedef. Поражаюсь я людям, трудолюбие которых позволяет писать что-то вроде:

void some_func(   const std::map< std::string, std::vector< std::fstream * > > & file_map )   {     for( std::map< std::string, std::vector< std::fstream * > >::const_iterator         ... )       ...   }

Вместо таких многоэтажных конструкций следует использовать typedef-ы и вменяемые имена для них. Что позволит получить:

• более компактный и читабельный код;
• более дешевую смену типов под typedef-ом. Например, если под typedef-ом был спрятан std::vector, то во многих случаях мы можем безболезненно и незаметно сменить его на std::deque или std::list.

Функции очень большого размера. Например, функции больше 50 строк. Выполняющие со всеми мельчайшими подробностями четыре-пять-шесть и более различных логических операций. Например, в какой-нибудь утилите на 1500-2000 строк будет огромная функция main(), в которой:

• сначала производится разбор аргументов командной строки (хорошо, если не через один for с тучей if-ов внутри);
• затем проверяется корректность аргументов;
• затем, если задан ключ --help, выполяется печать справки (ее текст размещается прямо тут же, в main-е);
• затем, выполняются практически все действия по прикладной логике утилиты;
• и тут же сосредоточенны большие catch-и, в которых находится весь код по обработке пойманного исключения.

Я утрирую, конечно, но это лишь для более яркой иллюстрации.

Вместо написания больших функций следует придерживаться простых правил:

• в идеале функция должна помещаться на одну экранную страницу, т.е. должна быть не более 20 строк длиной, включая описания параметров и возвращаемого значения. Крайний допустимый размер – один печатный лист формата A4. Это где-то 50-55 строк. Подходит для случаев, кода ради поиска особо злобных багов приходится код распечатывать и тщательно медитировать над ним с карандашом в руках;
• функция должна делать всего одну логическую операцию или
• функция должна быть менеджером нескольких логических операций (но тогда каждая логическая операция должна оформляться в ней одной-двумя строками кода).

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

В самых простых случаях это действительно одинаковые фрагменты:

void handle_request_A(...)   {     ...     m_pending_requests.push_back( request_info );     if( m_pending_requests.size() > m_overload_threshold )       {         m_overload_mode = true;         switch_to_overload_mode_handlers();       }     ...   } void handle_request_B(...)   {     ...     m_pending_requests.push_back( request_info );     if( m_pending_requests.size() > m_overload_threshold )       {         m_overload_mode = true;         switch_to_overload_mode_handlers();       }   }

Которые элементарно преобразовываются в более компактную запись:

void add_to_pending_request_with_overload_control(request_info_t request_info)   {     m_pending_requests.push_back( request_info );     if( m_pending_requests.size() > m_overload_threshold )       {         m_overload_mode = true;         switch_to_overload_mode_handlers();       }   } void handle_request_A(...)   {     ...     add_to_pending_request_with_overload_control(request_info);     ...   } void handle_request_B(...)   {     ...     add_to_pending_request_with_overload_control(request_info);   }

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

void handle_request_A(...)   {     ...     add_to_pending_request_with_overload_control(request_info);     m_request_type_A_stat.inc_pending_requests();     m_total_stat.inc_incoming_request_count( REQUEST_TYPE_A );     m_statistic_monitor.update(m_total_stat, m_request_type_A_stat);     ...   } void handle_request_B(...)   {     ...     add_to_pending_request_with_overload_control(request_info);     m_request_type_B_stat.inc_pending_requests();     m_total_stat.inc_incoming_request_count( REQUEST_TYPE_B );     m_statistic_monitor.update(m_total_stat, m_request_type_B_stat);   }

Тут нужно уже чуть-чуть больше внимания и фантазии, чтобы переделать код, скажем, в такой:

void handle_request_A(...)   {     ...     add_to_pending_request_with_overload_control(request_info);     increment_monitors_for_request_type(         m_request_type_A_stat,         REQUEST_TYPE_A );     ...   } void handle_request_B(...)   {     ...     add_to_pending_request_with_overload_control(request_info);     increment_monitors_for_request_type(         m_request_type_B_stat,         REQUEST_TYPE_B );   }

Ничего сложного в этом нет. Можно было бы пойти и еще дальше (скажем, жестко связать объект m_request_type_A_stat с константой REQUEST_TYPE_A чтобы передавать в increment_monitors_for_request_type всего один параметр), но даже такие простые преобразования повторяющегося кода уже очень сильно облегчают как чтение, так и сопровождение программы.

Использование магических значений и констант в коде. Он же hardcoding. Наиболее распространенный случай:

void some_method()   {     if(some_condition)       {         ...         m_state = 2;       }     else       {         ...         m_state = 3;       }   } void another_method()   {     if(2 == m_state)       ...     if(3 == m_state)       ...     if(4 == m_state)       ...   }

Что это за магические состояния: 2, 3, 4? Какие еще могут быть?

Но бывают и случаи потяжелее:

class input_backlog_t   {     ...   private :     char m_returned_chars[14];     char pop_one_char();   }; char input_backlog_t::pop_one_char()   {     char result = m_returned_chars[0];     for(size_t i = 0; i < 14; ++i)       m_returned_chars[i] = m_returned_chars[i+1];     return result;   }

Причем практически в точно таком же варианте – без каких-либо комментариев о том, почему взято значение 14, а не 13 или 15.

Для магических констант у меня уже давно есть простое правило:

• литерал-константа может использоваться только, если он встречается в коде ровно один раз. И его смысл и происхождение очевиден или четко обозначен. Во всех остальных случаев литерал следует заменить на символьную константу.

Ну, например, нужно проверить корректность введенного пользователем времени и одна из функций проверки будет иметь вид:

void ensure_valid_hour(int hour)   {     if(hour < 0 || hour > 24)       throw ...   }

Здесь использование магических констант 0 и 24 вполне уместно и безопасно. Поскольку их назначение и происхождение очевидно. А вот в следующей функции:

void ensure_valid_timezone(int tz)   {     if(tz < -48 || tz > 56)       throw ...   }

значения –48 и 56 будут представлять проблему, поскольку далеко не каждый разработчик с ходу поймет, что временная зона задается в четвертях часа и почему в качестве разрешенных границ взяты именно такие значения. Поэтому происхождение таких магических значений лучше было бы явно обозначить. Хотя бы так:

void ensure_valid_timezone(int tz)   {     // Временная зона задается в четвертях часа     // и должна укладываться в диапазон от -12:00 до +14:00.     if(tz < -(12*4) || tz > (14*4))       throw ...   }

Но лично я бы предпочел еще более многословный вариант:

/*! * \name Разрешенные границы временной зоны, указанной в четвертях часа. * \{ */ /*! Минимальная граница соответствует -12:00 согласно http://en.wikipedia.org/... */ const int MIN_TIMEZONE_IN_QUARTERS = -12*4; /*! Максимальная граница соответствует 14:00 согласно http://en.wikipedia.org/... */ const int MAX_TIMEZONE_IN_QUARTERS = 14*4; /*! * \} */ void ensure_valid_timezone(int tz)   {     if(tz < MIN_TIMEZONE_IN_QUARTERS || tz > MAX_TIMEZONE_IN_QUARTERS)       throw ...   }

Ручное и многократное освобождение ресурсов. Например, закрытие файла в нескольких местах в одной функции – в конце блока try если исключений не было, и в блоке catch, если исключение было:

void do_some_actions_with_file()   {     ACE_HANDLE hfile = ACE_OS::open(...);     if(ACE_INVALID_HANDLE == hfile)       ...     try       {         ...         ACE_OS::close(hfile);       }     catch(const some_application_exception_t & x)       {         ...         ACE_OS::close(hfile);       }     catch(const std::exception & x)       {         ...         ACE_OS::close(hfile);       }   }

Такой подход является ущербным даже для языков, в которых нет детерминированного уничтожения объектов (вроде Java), а уж в C++ и подавно. Деструкторы в C++ способны устранить потенциальные проблемы подобного кода вообще (ну т.е. как класс) с минимальными усилиями. Достаточно написать небольшую вспомогательную обертку:

class auto_file_closer_t   {     ACE_HANDLE m_hfile;   public :     auto_file_closer_t(ACE_HANDLE hfile) : m_hfile(hfile) {}     ~auto_file_closer_t() { ACE_OS::close(m_hfile); }   }; void do_some_actions_with_file()   {      try       {          ACE_HANDLE hfile = ACE_OS::open(...);         if(ACE_INVALID_HANDLE == hfile)         ...          auto_file_closer_t hfile_closer(hfile);          ...       }     catch(const some_application_exception_t & x)       {         ...       }     catch(const std::exception & x)       {         ...       }   }

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

Ручное управление памятью. Почти что разновидность только что описанной проблемы с освобождением ресурсов. Но, поскольку по облегчению работы с динамической памятью в C++ уже накоплено столько опыта/инструментов/статей/книг, то просто странно видеть в программах обилие вручную вызываемых new+delete. А то и malloc+free.

В подавляющем большинстве случаев все достаточно просто. Нужен динамический буфер? Попробуй сначала std::vector. Нужно создать объект через new? Подумай о том, чтобы сразу сохранить указатель в std::auto_ptr или каком-то из множества shared_ptr. Возвращает какая-то сторонняя библиотека память, которую нужно подчистить через free? Попробуй сделать что-то вроде auto_ptr, чтобы free вызывалась автоматически в деструкторе.

---

Главными причинами описанных выше проблем я считаю недостаток опыта и спешку. И если недостаток опыта – это, в общем-то, временная проблема, то спешка гораздо опаснее. Она будет присутствовать всегда, если ты позволяешь себе ей подчиниться. Спешить нужно медленно. Старая банальность, но менее справедливой она от этого не становится. Для начала нужно приучить себя вычитывать свой код несколько раз. У меня это получается автоматически, поскольку я стараюсь программировать на бумаге. Тем, кто сразу вбивает код в редакторе (да еще по 1K строк в день), вероятно, сложнее.

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