среда, 3 февраля 2010 г.

[comp.prog.flame] Впечатления после очередного code review

Провел на работе очередной code review. Парочкой впечатлений хочу поделиться. Предупреждаю сразу, примеров кода будет много ;)


Во-первых, очень удивляет меня, насколько сильно система отступов и переносов строк влияет на читабельность большого объема кода (порядка нескольких тысяч строк). Когда просматриваешь только одну функцию, то непривычный для тебя стиль переносов не играет никакой роли. Ну написано так, а не как у тебя. Ну и ладно.

Но потом в действие вступает закон больших чисел. Усталость ли, замыливание ли глаз или рассеивание внимания, но из-за непривычного оформления изучать чужой код становиться гораздо сложнее. Целые куски просто выпадают из поля зрения.

Например, вот в этом случае:

return get_valid_path( filename.substr( 0,
   filename.size() - get_name_of_file( filename ).size() ) +
   "sqlldr" );

В чем здесь проблема для меня? В том, что не понятно, что именно является параметрами substr – выражение сложное и глазу не за что в нем зацепиться. Вот как этот же фрагмент написал бы я:

return get_valid_path(
   filename.substr(
         0,
         filename.size() - get_name_of_file( filename ).size() ) +
   "sqlldr" );

В этом случае filename.substr() и “sqlldr” находятся на одном уровне вложенности и это подсказывает мне, что они являются равноправными частями одного выражения.

Еще напрягают вот такие выражения:

static bool
check( const msg_check_report_wait_t * msg ) {
   return (
      (msg->m_operation_id != c_unknown_operation_id)
      &&
      (msg->m_uuid != c_unknown_uuid)
   );
}

Я бы переписал это чуть компактнее:

static bool
check( const msg_check_report_wait_t * msg ) {
   return msg->m_operation_id != c_unknown_operation_id && 
      msg->m_uuid != c_unknown_uuid;
}

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

int
detect_actual_code(
   int result,
   int code,
   const std::string & type_result )
{
   if ( result == c_more_one_answer )
   {
      // Более одного ответа.
      return e_fatal_error;
   }
   else 
   {
      // Если был результат 2204050, то
      // код ответа не может быть 0, т.е. все плохо.
      if ( (type_result == "2204050") && (code == 0) )
      {
         return e_task_error;
      }

      if ( code != e_all_correct )
      {
          return e_task_error;
      }
   }

   return code;
}

встречается слишком часто, то воспринимается он гораздо хуже, чем вот такой:

int
detect_actual_code(
   int result,
   int code,
   const std::string & type_result )
{
   if ( result == c_more_one_answer ) 
      // Более одного ответа.
      return e_fatal_error; 
   else 
   {
      // Если был результат 2204050, то
      // код ответа не может быть 0, т.е. все плохо.
      if ( (type_result == "2204050") && (code == 0) ) 
         return e_task_error; 

      if ( code != e_all_correct ) 
          return e_task_error; 
   }

   return code;
}

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


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

Вот пример такой функции. Ее задача – сформировать идентификатор из 11 цифр на основе текущей даты и времени. Оригинальный вариант:

std::string
make_timestamped_uid()
{
   // Текущие msec в формат '000' с лидирующими нулями.
   std::string msec = cpp_util_2::slexcast(
      ACE_OS::gettimeofday().msec() % 1000 );
   // Число чисмолов в '000'.
   const int c_count_000 = 3;
   while ( msec.size() < c_count_000 )
   {
      msec = "0" + msec;
   }

   // Секунды с какого-то года в виде строки плюсуем к '000'.
   std::string uid = cpp_util_2::slexcast( ACE_OS::gettimeofday().sec() ) + msec;

   // Должно быть число в 11 цифр с лидирующими нулями.
   const int c_count_digits = 11;
   // Добиваем если не хватило.
   while ( uid.size() < c_count_digits )
   {
      uid = "0" + uid;
   }
   // Обрезаем старшее если получилось больше, чем 11 символов.
   if ( uid.size() > c_count_digits )
   {
      uid = uid.substr( uid.size() - c_count_digits );
   }
  
   return uid;
}

Первое, что мне бросилось в глаза – это два идентичных цикла для формирования нужного количества лидирующих нулей. Это действие сразу же нужно было бы выносить в отдельную inline-функцию. Которая бы при этом работала не через while, а через метод basic_string::insert.

Второе – это двойной вызов ACE_OS::gettimeofday() для получения текущего времени. Вполне достаточно было бы и одного.

Потом я задался вопросом – а зачем мы сначала выравнивали значения msec и uid до точных размеров, а потом обрабатывали превышение этого размера? Что-то в этом не то. Без этого вполне можно было бы обойтись. Ведь можно же сразу получать из даты/времени целочисленные значения в нужных диапазонах, а потом просто использовать стандартные средства C++ для форматирования.

Update. Три следущих абзаца оказались гнусным поклепом с моей стороны, поскольку я не заметил операции взятия остатка от деления на 1000. Что, к сожалению доказывает, что в переусложненном коде сложно разбираться

Тут-то всплыла ошибка, допущенная разработчиком make_timestamped_uid. Функция ACE_OS::gettimeofday() возвращает объект ACE_Time_Value. Который является парой <sec,usec>. Где sec – это текущая секунда от 1 января 1970, а вот usec – это микросекунда в рамках текущей секунды. Т.е. точное время – это (sec+usec).

Разработчик думал, что обращение к msec() – возвращает usec/1000, т.е. миллисекунды в рамках текущей секунды. Но ACE_Time_Value::msec() возвращает <sec,usec> преобразованные в миллисекунды (т.е. sec*1000+usec/1000). И даже не это значение, а ту его часть, которая помещается в unsigned long. Получается, что переменная msec всегда будет иметь длину больше трех символов. И что значение этой переменной будет иметь косвенное отношение к текущей миллисекунде.

Счетчик секунд от 1 января 1970 года сейчас – это десять десятичных цифр. К которым добавляется всего один символ из msec. Т.к. этот символ первый (т.е. самый старший разряд), то он будет одним и тем же для очень длительного временного интервала. Значит, несколько последовательных вызовов make_timestamped_uid в течении одной секуды (даже с интервалом в сотни миллисекунд) будет приводить к генерации одного и того же идентификатора. Т.е. функция не будет делать то, на что она была расчитана.

А ведь можно было написать все гораздо проще. Имхо, вот так:

std::string
make_timestamped_uid()
   {
      const ACE_Time_Value now = ACE_OS::gettimeofday();
      char buf[ 16 ];
      // Поскольку нам нужно строго 11-ти значное значение,
      // в котором три самых младших разряда будут занимать
      // миллисекунды, то от счетчика секунд оставляем
      // только 8 младших разрядов.
      const unsigned long s = now.sec() % 100000000ul;
      // Микросекунды должны быть преобразованы в миллисекунды.
      // Остаток от деления на 1000 для гарантии в случае, если
      // usec возвращает значение больше 1000000.
      const unsigned long m = ( now.usec() / 1000 ) % 1000;
      std::sprintf( buf, "%08lu%03lu", s, m );

      return buf;
   }

Если повыбрасывать комментарии, и промежуточные константы s и m, то можно вообще тремя-четырьмя строками обойтись. (Те, кому не нравится sprintf, могут получить то же самое через std::ostringstream, setfill и setw).

Вспоминается замечательный афоризм (за авторством, C.A.R.Hoare):

Есть два способа создания программы. Можно сделать ее настолько простой, что в ней совершенно очевидно нет ошибок. А можно сделать настолько сложной, что в нет совершенно очевидных ошибок.


Ну и в качестве бонуса для тех, что смог дочитать до этого места. В обсуждавшейся выше функции make_timestamped_uid() есть просто смешной фрагмент:

// Число чисмолов в '000'.
const int c_count_000 = 3;
Отправить комментарий