вторник, 13 марта 2012 г.

[prog] На тему читабельности кода

Написал несколько недель назад фрагмент кода. Отладил. Пошел писать другой код. Через какое-то время пришлось вернутся к нему еще раз. И тут наступили тормоза – не могу увидеть, где делается самое важное действие. Нет его в коде и все. Хотя программа работает и я точно знаю, это действие я писал и что ничего не менял.

Коротко: нужно определить, сколько готовых к отсылке пакетов сообщений можно поднять из БД, вычитать их из БД и отослать. Попутно логируя выполняемые действия.

Все это я в коде видел за исключением самой отсылки пакетов :)

Вот сей фрагмент в первозданном виде:

void
a_out_pkg_sender_t::try_send_or_resend_packages()
   {
      const ACE_Time_Value current_time = ACE_OS::gettimeofday();
      m_load_batcher.check_for_new_period( current_time );

      const unsigned int max_packages_to_send =
            m_load_batcher.remaining_load();
      if( 0 != max_packages_to_send )
         {
            if( m_cfg.m_diagnostic_logging.m_rescan_db_for_resending )
               so_log_msg_same_ns( outgoing_package, load_started )
                     .max_package_count( max_packages_to_send )
                     .finish( *this );

            std::auto_ptr< db::out_message_info_list_t > messages =
                  m_db->select_out_pkg_for_sending(
                        max_packages_to_send,
                        date_time_in_past(
                              current_time,
                              m_cfg.m_notifies.m_out_pkg_throughput.m_resend_timeout ),
                        ACE_Date_Time( current_time ) );

            if( m_cfg.m_diagnostic_logging.m_rescan_db_for_resending )
               so_log_msg_same_ns( outgoing_package, load_finished )
                     .messages_loaded( messages->size() )
                     .finish( *this );

            if( !messages->empty() )
               m_load_batcher.increment_current_load(
                     make_and_send_packages( *messages ) );
         }
   }

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

if( !messages->empty() )
{
   const unsigned int packages_sent =
         make_and_send_packages( *messages );

   m_load_batcher.increment_current_load( packages_sent );
}

76 комментариев:

Andrey Valyaev комментирует...

Помоему очень много буков. :)
Не, когда идентификаторы говорят сами за себя - это хорошо...

Но для большей чистоты кода, ИМХО, надо стараться кратко доносить смысл каждого идентификатора. То есть прилагать усилия к тому, чтобы идентификаторы были короче, а текст был бы прозрачнее.

А то смотришь на код, кода нету, один текст. :)

имя комментирует...

о! кажись мне удалось победить блогстоп и запостить код с нормальной индентацией!

if( !messages->empty() )
{
        auto packages_sent = make_and_send_packages( *messages );
        m_load_batcher.increment_current_load( packages_sent );
}

вот мне какжется как будет понагляднее (понятно, весь этот разговор довольно субъективен)

дальше, if( messages->is_not_empty() ) более читабельно (и not более заметен чем воскл. знак

дальше я пофантазирую на тему "а как это бы выглядело в хорошем языке"

eao197 комментирует...

@Андрей Валяев

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

eao197 комментирует...

@имя

По поводу empty() vs is_not_empty() -- это к Алексу Степанову. Ему почему-то вздумалось в STL-ных контейнерах методы для проверки пустоты контейнера обзывать empty(). Как по мне, так лучше было бы is_empty().

имя комментирует...

это могло бы выглядеть так (может даже и на с++):

if( messages->is_not_empty() )
{
        pipe( *messages,
                make_and_send_packages,
                m_load_batcher.increment_current_load
        );
}
или так:

if( messages->is_not_empty() )
{
        pipe(*messages)
        | make_and_send_packages
        | m_load_batcher.increment_current_load ;
}

имя комментирует...

правда, последние варианты не говорят о том, доходят ли messages до increment_current_load по одной штучке, либо туда доходит всего одно число (т.е. их количество)

это можно рассматривать и как плюс, и как минус

eao197 комментирует...

@имя:

это могло бы выглядеть так

Могло бы. Но зачем?

И как тебе удалось победить блогспот?

имя комментирует...

есть такой символ в юникоде 00A0, и вот похоже браузеры его отображают как пробел, но пробелом не считают :-)

Andrey Valyaev комментирует...

А я пришел например к тому, что фигурные скобки надо ставить всегда... так однозначнее и предсказуемее...

А то с 11 строки до измененного фрагмента очень часто трудно сказать где кончается условие и начинается тело...

С этим ИМХО тоже надо бороться...

Хотя, особенно новый стандарт, очень сильно жрет место по ширине... даже в 100 символов трудно уложиться, но я стараюсь.

имя комментирует...

повторю -- дальше идут в основном фантазии на тему "как это должно быть в правильном с++", а не конкретные предложения писать таким образом в нынешнем с++

> Могло бы. Но зачем?

потому, что в краткой записи

m_load_batcher.increment_current_load( make_and_send_packages( *messages ) )

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

это неудобство можно устранить, и получится pipe

рассмотрим теперь запись

packages_sent = make_and_send_packages( *messages );

packages_sent является комментарием, но меня несколько беспокоит тот момент, что он не является частью функции
make_and_send_packages

т.е. можно представить такой рефакторинг (хотя его вряд ли кто будет делать), что make_and_send_packages начнет возвращать bool

и тогда комментарий будет вводить в заблуждение

вот если бы там было make_and_send_packages( *messages ).number_of_packages_sent тогда этот комментарий был бы частью сигнатуры функции, и соответственно более надежен

т.е. похоже оно должно быть таким:

if( messages->is_not_empty() )
{
      pipe(*messages)
      | make_and_send_packages( _ ).number_of_packages_sent
      | m_load_batcher.increment_current_load( _ ) ;
}

имя комментирует...

да, возвращаясь к реальному (а не фантазийному) с++:

1. я не понял, зачем там auto_ptr?

2. дальше
date_time_in_past(
current_time,
m_cfg.m_notifies.m_out_pkg_throughput.m_resend_timeout )

лучше заменить на

current_time -
m_cfg.m_notifies.m_out_pkg_throughput.m_resend_timeout

не?

Andrey Valyaev комментирует...

Вот это: m_cfg.m_notifies.m_out_pkg_throughput.m_resend_timeout и m_cfg.m_diagnostic_logging.m_rescan_db_for_resending два раза - ну очень длинные имена. :)

eao197 комментирует...

@Андрей Валяев

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

Сокращение в одном приведет к увеличению в другом :)

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

eao197 комментирует...

@имя:

auto_ptr там потому, что возвращается контейнер (который может быть весьма большим). Не по значению же его возвращать.

Функция date_time_in_past добавлена из-за того, что классы ACE_Time_Value не имеют удобных средств арифметики (добавление и вычитание временных величин). А те, что есть, требуют дополнительных приседаний. Так же date_time_in_past преобразует ACE_Time_Value в ACE_Date_Time, то так же требовало бы дополнительной писанины, не будь date_time_in_past.

имя комментирует...

> auto_ptr там потому, что возвращается контейнер (который может быть весьма большим). Не по значению же его возвращать.

по значению вполне возможно

http://en.wikipedia.org/wiki/Return_value_optimization

для msvc это работает

eao197 комментирует...

@имя

AFAIK, работа RVO находится в сильной зависимости от целой кучи факторов, включая фазу Луны. Поэтому если есть сомнения, то я предпочитаю полагаться на собственные руки.

Miroslav комментирует...

прошу прощения но первый листинг это какойто фарш с макаронами. Крупными такими чтобы подавиться и умереть.

eao197 комментирует...

@Miroslav:

Надеюсь, вы понимаете, что это всего лишь эмоциональное высказывания. И практической пользы от него ноль.

имя комментирует...

насчет макарон -- думаю в конце выяснится, что так и есть, но чтобы предложить свой сокращенный вариант мне потребуются ответы на вопросы

в частности:

1. в чем разница между message, package и pkg?

2. что конкретно логируется таким вот интересным образом (кажется, что логируется время запроса, но еще что)?

3. select_out_packages_for_sending эта такая обертка чисто для sql-запроса?

и сразу предложение -- *она же* и должна логировать свое время выполнения

имя комментирует...

> Функция date_time_in_past добавлена из-за того, что классы ACE_Time_Value не имеют удобных средств арифметики (добавление и вычитание временных величин). А те, что есть, требуют дополнительных приседаний. Так же date_time_in_past преобразует ACE_Time_Value в ACE_Date_Time, то так же требовало бы дополнительной писанины, не будь date_time_in_past.

посмотрел я доки этих классы, мой вывод -- надо пользоваться вычитанием в ACE_Time_Value (и она будет основным форматом времени), а конвертацию в ACE_Date_Time должна производить та функция, которой потребовались например дни месяца, часы, минуты, т.е. скажем sql-запрос

имя комментирует...

sql может быть записан например так (и да, у меня от m_ рябит в глазах):

db::out_message_info_list messages =
    db->select_out_pkges
        .from_time(current_time - cfg.notifies.out_pkg_throughput.resend_timeout)
        .up_to_time(current_time)
        .no_more_than(max_pkges_to_send);

select_out_pkges формулирует "уникальную часть запроса" (причем предположу что пофиг for_resend или для какой иной цели); дальше по цепочке добавляются условия по времени и по количеству -- судя по твоей работе, они у тебя встречаются очень часть

имя комментирует...

"_time" по желанию можно выбросить, и оставить вообще только .from и .upto

имя комментирует...

* судя по твоей работе, они у тебя встречаются очень часто

имя комментирует...

кстати, еще насчет длины -- я одобряю коболовское thru и значит thruput вместо throughput, но это по вкусу

имя комментирует...

или с .run(), если ты не любитель вызывать sql-запрос путем неявного приведения типов:

db::out_message_info_list messages =
    db->select_out_pkges
        .from_time(current_time - cfg.notifies.out_pkg_throughput.resend_timeout)
        .up_to_time(current_time)
        .no_more_than(max_pkges_to_send)
        .run();

имя комментирует...

что касается конфига (cfg), то там все интересно

можно ли там сделать namespaces вместо полей? тогда все бы вытаскивалось на один уровень выше (и значит, длина была бы меньше)

или сделать отдельный класс Notifies, и наследовать от него, а не делать его полем -- тогда опять все бы вытаскивалось на один уровень выше

eao197 комментирует...

@имя:

Вопросы отлично показывают стоимоть категоричных оценок чужому коду :) Что, где, как, почему и зачем неизвестно, но код -- отстой ;)

>1. в чем разница между message, package и pkg?

Пакет состоит из сообщений. pkg -- это сокращение для package в случаях, когда идентификатор мог получиться совем уж длинным.

>2. что конкретно логируется таким вот интересным образом (кажется, что логируется время запроса, но еще что)?

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

>3. select_out_packages_for_sending эта такая обертка чисто для sql-запроса?

Это виртуальный метод интерфейса, который скрывает от агента детали работы с БД (в том числе и тип конкретной БД).

eao197 комментирует...

@имя:

а конвертацию в ACE_Date_Time должна производить та функция, которой потребовались например дни месяца, часы, минуты, т.е. скажем sql-запрос

Не катит. Разделение ответственности. Слой работы с БД должен получить конкретную дату/время. Как она получается -- не его забота. И дополнительные преобразования из ACE_Time_Value или еще чего-либо (скажем, из std::tm) ему не нужны.

eao197 комментирует...

@имя:

>sql может быть записан например так (и да, у меня от m_ рябит в глазах):

db::out_message_info_list messages =
db->select_out_pkges
.from_time(current_time - cfg.notifies.out_pkg_throughput.resend_timeout)
.up_to_time(current_time)
.no_more_than(max_pkges_to_send);


Ну и что это за хрень? ;)

Это будет порождаться цепочка временных объектов, деструктор последнего из которых порождает нужную операцию?

Или это будет один объект, у которого туча методов?

Сколько нужно усилий на документирование и текстирование этого? Как будет обеспечиваться корретность работы с этим объектом (т.е. как заставить пользователя работать с этим безобразием только корректными способами)?

Кто вообще сказал, что строится SQL-запрос (один единственный)? Кто вообще сказал, что БД будет SQL-ной? Кто вообще сказал, что приведенный фрагмент будет работать с БД, а не с каким-либо имитационным mock-объектом?

имя комментирует...

> Вопросы отлично показывают стоимоть категоричных оценок чужому коду :) Что, где, как, почему и зачем неизвестно, но код -- отстой ;)

не отстой, а может быть существенно улучшен

и кстати что я не указал -- то, что для его улучшения могут потребоваться существенные усилия, и вовсе человек его не обязан писать *сразу* кратко, особено с учетом enterprise-специфики, сильно подталкивающей к длинным выражениям

но в целом именно так -- яркое *впечатление* есть, а вот чтобы доказать его -- нужно разбираться и разбираться

имя комментирует...

> Пакет состоит из сообщений. pkg -- это сокращение для package в случаях, когда идентификатор мог получиться совем уж длинным.

ок, я это и предполагал

тогда надо всегда писать pkg вместо package; и вообще иметь список из скажем стандартных 10-12 сокращений (типа os, db, pkg) который каждый разраб должен знать наизусть и всегда использовать, иначе код пухнет (стандартных хотя бы для твоего кода)

з.ы. а если ты вдруг с этим внезапно не согласен, то хотя бы try_send_or_resend_packages и прочие столь длинные сокращать, раз уж ты сократил select_out_pkg_for_sending

имя комментирует...

я бы еще предложил множественное число pkges, но это на твое усмотрение

и опять на усмотрение i_pkg вместо in_pkg и o_pkg вместо out_pkg

имя комментирует...

> Слой работы с БД должен получить конкретную дату/время.

и каким же образом ACE_Time_Value неконкретное?

как раз наоброт, это ACE_Date_Time неконкретное -- это и не время вовсе (т.к. у него нет даже оператора минус), а некий объект-для-форматирования-и-печати, у которого можно прочитать поля час и минута

в пользу моей точки зрения -- я вижу перевод из ACE_Time_Value в ACE_Date_Time (т.е. в форматированное), но не наоборот

еще ACE_Date_Time не валидируется конструктором, когда создается

я полагаю, что был бы это не с++, так класса ACE_Date_Time вообще бы не было, а были бы методы get_day(), ... у ACE_Time_Value; но поскольку на с++ пишут параноики на тему эффективности, то предполагается, что они сначала переведут настоящее время в ACE_Date_Time, а затем начнут читать у него поля (и скажу -- лучше бы поручить эту оптимизацию компилятору, и не отвлекать пользователя лишним классом)

eao197 комментирует...

@имя:

и каким же образом ACE_Time_Value неконкретное?

Его еще в локальное время нужно преобразовывать. Или в не локальное :)

Вообще, все имеет исторические корни. Структуры данных, связанные со слоем БД, изначально хранили дату-время в ACE_Date_Time (поскольку так было проще для основных операций). Поэтому и дальше пошло использоваться соглашение о передаче в слой БД дат и времен в виде ACE_Date_Time.

eao197 комментирует...

@имя:

>а если ты вдруг с этим внезапно не согласен, то хотя бы try_send_or_resend_packages и прочие столь длинные сокращать, раз уж ты сократил select_out_pkg_for_sending

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

Рефакторить же, чтобы привести к общему виду... Ну можно, наверное.

имя комментирует...

> select_out_packages_for_sending это виртуальный метод интерфейса, который скрывает от агента детали работы с БД (в том числе и тип конкретной БД).

а моя "хрень" это в простейшем случае ровно то же самое, но переписанное в стиле "именованные параметры"; с++ их напрямую не поддерживает, но можно заюзать chained methods; в не-простейшем случае, который по-моему весьма вероятен, там будет еще профит

"именованные параметры" -- это нужно, поскольку там уже 2 параметра, и оба время; а еще туда вполне возможно потребуется передавать параметр логирования

#define RUN_AND_LOG(x) run_and_log(cfg.diagnostic_logging.x)

db::out_msg_info_list msges =
    db->select_out_pkges
        .from_time(current_time - cfg.notifies.out_pkg_throughput.resend_timeout)
        .up_to_time(current_time)
        .no_more_than(max_pkges_to_send)
        .RUN_AND_LOG(rescan_db_for_resending);

если переписать cfg на пространствах имен или на наследовании, то макрос можно будет убрать (заменить на языковые средства с++)

заметь, мы убрали дублирование -- логгинг был с повторением rescan_db_for_resending, сейчас он написан только один раз

а что если нам все же потребуется именно 2 логирования, причем еще и с разными параметрами конфига? тогда надо сделать LOG_AND_RUN2 куда влепить 2 параметра логирования (либо variadic macro, если msvc их поддерживает)

з.ы. а еще весьма вероятно, что .up_to_time(current_time) можно просто выбросить, сделав по умолчанию там именно текущиее время, что еще сократит код

имя комментирует...

локальное время must die, об этом уже говорили у тебя в блоге

что означает локальное время 2:30:00, если в 3:00:00 был перевод часов на час назад? оно означает 2 (два) момента

имя комментирует...

> Рефакторить же, чтобы привести к общему виду... Ну можно, наверное.

и вот так, по двум-трем процентикам мелких улучшений у тебя и набежит суммарное сокращение кода на 50%

eao197 комментирует...

@имя:

При всем желании сказать, что вот это:

>#define RUN_AND_LOG(x) run_and_log(cfg.diagnostic_logging.x)

db::out_msg_info_list msges =
db->select_out_pkges
.from_time(current_time - cfg.notifies.out_pkg_throughput.resend_timeout)
.up_to_time(current_time)
.no_more_than(max_pkges_to_send)
.RUN_AND_LOG(rescan_db_for_resending);


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

имя комментирует...

http://codepad.org/cRBDgZOv вот как я это представляю себе полностью

При всем желании сказать, что вот это проще в восприятии и в сопровождении, чем мой изначальный код, не могу. В особенности, если добавиться второй макрос.

может это потому, что для тебя непривычны "chained methods вместо именованных параметров", поскольку ты их сравнительно мало используешь на практике?

з.ы. они вполне приличны и просты, не то что зверские метапрограммы на шаблонах

з.ы.ы. имхо от *отдельных однострочных* макросов тоже не надо шарахаться

eao197 комментирует...

@имя:

>http://codepad.org/cRBDgZOv вот как я это представляю себе полностью

Это не полностью. Где код тех самых chaining methods? А так же документация к ним.

Так же этот код не является эквивалентом моему. Если бы логирование было допустимо на уровне слоя БД, оно бы там и было. Но слой БД не связан с логированием -- это не его дело.

Ну и #undef RUN_AND_LOG не помешал бы.

В итоге выигрыша я не вижу в упор.

имя комментирует...

а еще подозрительно выглядит такое вот сочетание:

load_batcher.check_for_new_period( current_time );
const uint max_pkges_to_send = load_batcher.remaining_load();

что будет, если кто-то забудет вызвать check_for_new_period ?

ну допустим у тебя там предусмотрена защита от забывчивых (хотя это отдельная работа); почему бы не сделать просто

const uint max_pkges_to_send = load_batcher.remaining_load(current_time);

eao197 комментирует...

@имя:

Ну и маленькая заноза. Вместо красивого

current_time - cfg.notifies.out_pkg_throughput.resend_timeout

Придется записать:

current_time - ACE_Time_Value(cfg.notifies.out_pkg_throughput.resend_timeout, 0)

А если resend_timeout задается в минутах или в миллисекундах, или вообще в виде вещественного числа (или структуры), то будет еще красочнее.

eao197 комментирует...

@имя:

>что будет, если кто-то забудет вызвать check_for_new_period ?

У него будет неправильно дозироваться нагрузка.

Там изначально задумано так, обращения к remaining_load() и increment_current_load() привязывались только к тому такту, на котором был вызван check_for_new_period. При этом, в принципе, после обращения к check_for_new_period() возможны многократные обращения к remaining_load/increment_current_load.

имя комментирует...

Ну и #undef RUN_AND_LOG не помешал бы.

нет, я предполагаю что это макрос уровня части проекта, без undef

он рассчитывает на разумное соглашение "конфиг у нас всегда называется cfg, и отдел логгинга там всегда называется diagnostic_logging"; если соглашение есть, то макрос сокращает код и убирает дублирование "log <—> diagnostic_logging"; если соглашения нет, то компилятор ругнется; по-моем, никому не взбредет в голову назвать парой cfg.diagnostic_logging что-то совсем не относящееся к конфигу и логингу

в принципе, его можно и выкинуть -- строчка растянется на 20 символов до

.run_and_log( cfg.diagnostic_logging.rescan_db_for_resending )

имя комментирует...

> Это не полностью. Где код тех самых chaining methods? А так же документация к ним.

ровно там же, где код и документация к select_out_packages_for_sending

дополнительно туда перенесен логгинг, но я пока не слышал от тебя обоснований того, что логировать функцию доступа к базе внезапно надо в месте ее использования (а не определения), да и еще 2 раза (до вызова и после вызова)

имя комментирует...

Придется записать:

current_time - ACE_Time_Value(cfg.notifies.out_pkg_throughput.resend_timeout, 0)

А если resend_timeout задается в минутах или в миллисекундах, или вообще в виде вещественного числа (или структуры), то будет еще красочнее.


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

если у тебя время это ACE_Time_Value, значит будет ACE_Time_Value

з.ы. никто не мешает иметь второй конфиг, где все элементы это строки, но его нужно никому не показывать, кроме валидатора

имя комментирует...

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

eao197 комментирует...

@имя:

>ровно там же, где код и документация к select_out_packages_for_sending

Нет, нет его там. Прототип одного метода с документацией на него -- это одно дело.

У тебя же не один, а несколько методов. Каждый из них должен иметь и свой прототип, и какой-то объект с атрибутами, куда сохраняются параметры.

Вот и где вся эта кухня? Какой у нее объем?

eao197 комментирует...

@имя:

>resend_timeout это время?

resend_timeout -- это размер временного отрезка в своих собственных попугаях.

eao197 комментирует...

@имя:

>я пока не слышал от тебя обоснований того, что логировать функцию доступа к базе внезапно надо в месте ее использования (а не определения), да и еще 2 раза (до вызова и после вызова)

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

Есть другой слой абстракции -- доступ к БД. На этом слое нет понятия логирования, как и нет понятия агента, от имени которого пишутся в лог сообщения.

Посему, поскольку факт обращения к БД (с конкретным параметром, который должен быть в логе) должен быть залогирован, то это делает агент перед обращением к БД. А после обращения к БД логирует факт завершения, опять же с новым параметром.

имя комментирует...

> Вот и где вся эта кухня? Какой у нее объем?

ее объем будет больше, т.к. она решает бОльшие задачи -- передачу именованных параметров -- и это оправдано

как она будет выглядеть в минимальном переписывании -- щас накатаю

имя комментирует...

Есть другой слой абстракции -- доступ к БД. На этом слое нет понятия логирования, как и нет понятия агента, от имени которого пишутся в лог сообщения.

и кто мешает во время вызова доступа к БД передать туда лишний дополнительный параметр -- имя агента (кстати, не это ли делает таинственный .finish(*this)?) ?

___________________

что же касается дизайна именованных параметров, то он зависит от того, наличие каких параметров для тебя обязательно; а еще там придется записывать те куски кода, которые тут у тебя не приведены

eao197 комментирует...

@имя:

и кто мешает во время вызова доступа к БД передать туда лишний дополнительный параметр -- имя агента

Это смешение слоев абстракции. Нижний слой не должен знать деталей более высокого слоя (или соседнего по своему уровню слоя).

(кстати, не это ли делает таинственный .finish(*this)?) ?

so_log_msg_same_ns -- это макрос, который раскрывается в цепочку chaining methods (и objects так же). Последний в этой цепочке метод finish как раз получает ссылку на агента, инициирующего логирование.

имя комментирует...

resend_timeout -- это размер временного отрезка в своих собственных попугаях.

да хоть в планковских единицах времени, не важно -- после чтения конфига там должен быть объект стандартного класса времени

исключения из этого правила -- если там у тебя хитрозамороченная система вообще без абсолютных единиц времени, во что я (с ходу) не верю -- т.е. хотя бы после чтения всего конфига тебе уже известны абсолютные значения

eao197 комментирует...

@имя:

>ее объем будет больше, т.к. она решает бОльшие задачи -- передачу именованных параметров -- и это оправдано

А вот нафиг это все? Нет в С++ именованных параметров. Поэтому городить огород чтобы передавать пару-тройку параметров в несколько методов -- это не нужное усложнение. Которому место в универсальной библиотеке общего назначения (вроде определения параметров при создания сокета), а не в конкретном прикладном проекте.

eao197 комментирует...

@имя:

да хоть в планковских единицах времени, не важно -- после чтения конфига там должен быть объект стандартного класса времени

Во-первых, не факт, что должен быть. Прочитали из конфига в попугаях, значит и лежать может дальше в попугаях. Вдруг это будет проще и удобнее.

Во-вторых, нет в С++ пока стандартного класса для хранения time_value и time_duration. В ACE свои классы, в POCO свои, в Boost-е свои. У нас совместно используется и ACE и POCO. И не исключено, что со временем еще что-то прицепим. Попугаи, они интероперабельны оказываются :)

имя комментирует...

Это смешение слоев абстракции. Нижний слой не должен знать деталей более высокого слоя (или соседнего по своему уровню слоя).

so_log_msg_same_ns -- это макрос, который раскрывается в цепочку chaining methods (и objects так же). Последний в этой цепочке метод finish как раз получает ссылку на агента, инициирующего логирование.


вот и у тебя есть это якобы "смешение", которое неизбежно и не плохо; логирование вообще такая вещь, которая не относится строго к одному уровню, и ее приходится протягивать по уровням (соблюдая опреденную аккуратность, конечно)

ага, макрос маленькими буквами!? и ты молчал как партизан все это время, при этом критикуя мой макрос, сделанный, можно сказать, на замену твоего

вдобавок, оказывается, у тебя сделано почти то же, что предлагаю я, разве что имеется дублирование (до и после вызова), которого у меня нет

имя комментирует...

это не нужное усложнение. Которому место в универсальной библиотеке общего назначения (вроде определения параметров при создания сокета), а не в конкретном прикладном проекте.

у тебя именно тот случай, который похож на сокет -- интерфейс к sql-ю (либо как вариант твои собственные Query, похожие sql)

там как раз именованные параметры фактически обязательны из-за кучи опций

попытка вместо них сделать select_out_pkg_for_sending очень похожа на запутывание кода; инлайн этой функции (тут вспомним, что я инлайнер) в виде не то чтобы sql-я, а своей тонюсенькой оберточки вокруг sql-я будет как раз тем, что надо

имя комментирует...

Прочитали из конфига в попугаях, значит и лежать может дальше в попугаях.

это называется бардак

Во-вторых, нет в С++ пока стандартного класса для хранения time_value и time_duration.

и значит ты вправе придумать свой класс, совместимый со всеми используемыми тобой классами (и пусть он мерит в попугаях -- *тогда* у меня нет претезий к ним) или взять чужой класс, но внутренний стандарт должен быть

имя комментирует...

как вариант не-бардака может быть:

1. стандартный-лично-для-тебя-класс-времени

2. его наследник, меряющий время в попугаях

3. возможно еще наследники, сделанные для интероперабельности с остальными классами типа ACE_Time_Value

... ну или какая-то иная схема не обязательно с наследованием, но обязательно с полиморфизмом и *незаметной* конвертацией времени в разные классы

eao197 комментирует...

@имя:

>вот и у тебя есть это якобы "смешение", которое неизбежно и не плохо; логирование вообще такая вещь, которая не относится строго к одному уровню, и ее приходится протягивать по уровням (соблюдая опреденную аккуратность, конечно)

Какое смешение? Его нет. Есть слой логирования для SObjectizer агентов. Этим слоем пользуется прикладой агент. Снабжая его нужными параметрами. Точно так же он пользуется слоем БД.

Но вот слой БД к логированию от имени SObjectizer-агентов не имеет никакого отношения. И не должен в принципе.

То, что ты позволяешь себе переместить логирование внутрь слоя БД -- это ты решаешь какую-то другую задачу. С другими условиями.

>критикуя мой макрос, сделанный, можно сказать, на замену твоего

Твой макрос я не критиковал. Мне не нравится, что прикладной программист для выполнения прикладных действий должен по месту вводить свои частные макросы. Тогда как макросы so_log_msg_8* -- это части нашей библиотеки логирования, которые прикладной программист просто использует. Так же, как assert() или errno (который вообще не пойми что).

имя комментирует...

Которому место в универсальной библиотеке общего назначения (вроде определения параметров при создания сокета), а не в конкретном прикладном проекте.

тонюсенькой обертке вокруг sql-я место в прикладном проекте по той причине, что ты, как мне кажется, постоянно селектишь пакеты по времени

мне вообще интересно -- а ты вообще по *другим* признакам их селектишь? и даже если да, то много ли этих признаков?

причем, судя по тому что ты написал, ты уже сделал мини-обертку для абстрагирования от БД, так что добавить парочку классов и десяток методов туда будет и не слишком затратно

eao197 комментирует...

@имя:

>попытка вместо них сделать select_out_pkg_for_sending очень похожа на запутывание кода; инлайн этой функции (тут вспомним, что я инлайнер) в виде не то чтобы sql-я, а своей тонюсенькой оберточки вокруг sql-я будет как раз тем, что надо

Еще раз -- кто сказал, что это тонюсенькая обертка? Даже в простейшей реализации "в лоб" это несколько запросов + транзакция. А когда это дело будет тюнится под особенности конкретных версий конкретных типов СУБД это вообще может выродится в несколько файлов с кодом, оптимизированным для разных баз. Поэтому-то и был выделен слой, к которому прикладной уровень обращается в приказном порядке -- "сделай вот это", а как это будет сделано, какой кровью и с какими подробностями... Об этом прикладной слой и знать не должен без лишней на то необходимости.

имя комментирует...

Но вот слой БД к логированию от имени SObjectizer-агентов не имеет никакого отношения. И не должен в принципе.

То, что ты позволяешь себе переместить логирование внутрь слоя БД -- это ты решаешь какую-то другую задачу. С другими условиями.


я ориентируюсь на это:

Логируется факт начала выборки и факт завершения выборки.

поскольку функция select_out_packages_for_sending точно знает оба этих факта, и не знает только то, от имени кого (или как) надо произвести логирование, ей надо это логирование перепоручить, передав ссылку на логирующий аппарат

тем самым уберется дублирование кода и возможные ошибки

это я уже 2-й раз говорю, только немного четче

имя комментирует...

Еще раз -- кто сказал, что это тонюсенькая обертка?

(насчет толщины) ну уж ведь не гибернейт у тебя там, а?

а если это не тонюсенькая обертка, то мои доводы насчет необходимости поддержки именованных параметров только усиливаются

имя комментирует...

в дополнение к фактам:

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

и функция select_out_packages_for_sending знает эти количества, причем *лучше* значет, чем агент; т.е. в случае вызвова логирования со стороны агента эти количества приходится в лог передавать в явном виде

eao197 комментирует...

@имя:

>это я уже 2-й раз говорю, только немного четче

Да хоть в 25-й. Не должен слой БД заниматься логированием. Вообще.

Мой код был написан с учетом этого ограничения. Не было бы его, код был бы другим.

имя комментирует...

Не должен слой БД заниматься логированием. Вообще.

ок, допустим у нас есть dll-ка архиватор, и он не должен сам показывать гуевое окошко с процентом выполнения (это смешение слоев), но должен как-то рапортовать о проценте выполнения задачи

решение -- архиватор вызывает callback

точно так же и здесь из около-БД-шного кода можно вызывать callback, который будет логировать от имени соотв. агента

так устраивает?

eao197 комментирует...

@имя

>точно так же и здесь из около-БД-шного кода можно вызывать callback, который будет логировать от имени соотв. агента

Не нужен здесь никакой callback. И БД вообще не в курсе, нужно ли что-то логировать, в какой мере и при каких условиях.

БД либо возвращает результат, либо бросает исключение.

Это агент знает, что вот сейчас он вызовет слой БД. Сей факт при определенных условиях (о которых знает агент) должен быть залогирован. А вот агент получил результат от слоя БД. И опять-таки, только при условиях, о которых знает агент, сей факт логируется.

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

Miroslav комментирует...

я заметил что не получил тот ответ только сейчас лежа с гриппом на больничном. Сел и порубил крупные куски для примера и сунул в http://pastie.org/3720201
я понимаю что вкусовщина но мало ли :)

eao197 комментирует...

@Miroslav:

Во-первых, выздоравливай!

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

1. Макрос XX_LOGMSG следовало бы определить прямо внутри метода try_send_or_resend_packages. А в конце метода сделать для него undef. Поскольку в других методах класса so_log_msg_same_ns используются с другими параметрами и другим количеством аргументов. Т.е. там XX_LOGMSG должен иметь другой вид.

2. Лично я не люблю typedef-ы, если введенное им имя используется всего один раз. За исключением случаев когда под typedef прячется какой-то хитрый указатель (например, указатель на метод).

3. Переменную need_rescan_db следовало бы обозвать need_log_rescan_db.

Это то, за что я бы автора кода поругал :)

Miroslav комментирует...

с деталями понятно. то есть разницу в чтении не видно?

eao197 комментирует...

@Miroslav:

На мой взгляд, по сути переделок не было, основные черты кода остались неизменныи. Стало чуть многословнее. Читать код мне это ничуть не мешало. Но писать столько лично мне было бы в лом.

Miroslav комментирует...

окей попробую пояснить.
1)апи лога засоряет локальный контекст именами которые не нужны там. Это одно из немногих мест где я допускаю макросы - весь мусор надо убрать и си увы иначе не позволяет.
2)многократное чтение конфигурации. вы правда хотите чтобы однажды те 2 ифа сработали по разному?
3)авторы апи не позаботились об краткой записи имени смартпойнтера. Тайпдефу или даже дефайну место там.
4)6 лишних пробелов слева при ограничении в 80 дают жуткие лесенки в 3 ряда. Обратите внимание что 2! ряда это предел после которого это уже неприемлемо.
5) сложные многострочные if/... без скобок.
вот навскидку

eao197 комментирует...

@Miroslav:

1)апи лога засоряет локальный контекст именами которые не нужны там.

Пряча их использование мы увеличиваем количество сущностей, которыми должен оперировать программист. При этом этот конкретный API используется только здесь и сейчас. В других методах он другой.

авторы апи не позаботились об краткой записи имени смартпойнтера.

Не вижу в этом смысла, если a) в имени смартпоинтера обязательно нужно отразить тот факт, что это auto_ptr, b) это имя используется считанное количество раз.

4)6 лишних пробелов слева при ограничении в 80 дают жуткие лесенки в 3 ряда.

Я сам придерживаюсь ограничения в 80 символов. И лесенка в 3 ряда -- это вовсе не жуть.

Да и if-ы вовсе не сложные. Простое условие, один вызов под if-ом.