[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;