Написал несколько недель назад фрагмент кода. Отладил. Пошел писать другой код. Через какое-то время пришлось вернутся к нему еще раз. И тут наступили тормоза – не могу увидеть, где делается самое важное действие. Нет его в коде и все. Хотя программа работает и я точно знаю, это действие я писал и что ничего не менял.
Коротко: нужно определить, сколько готовых к отсылке пакетов сообщений можно поднять из БД, вычитать их из БД и отослать. Попутно логируя выполняемые действия.
Все это я в коде видел за исключением самой отсылки пакетов :)
Вот сей фрагмент в первозданном виде:
| 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 комментариев:
Помоему очень много буков. :)
Не, когда идентификаторы говорят сами за себя - это хорошо...
Но для большей чистоты кода, ИМХО, надо стараться кратко доносить смысл каждого идентификатора. То есть прилагать усилия к тому, чтобы идентификаторы были короче, а текст был бы прозрачнее.
А то смотришь на код, кода нету, один текст. :)
о! кажись мне удалось победить блогстоп и запостить код с нормальной индентацией!
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 более заметен чем воскл. знак
дальше я пофантазирую на тему "а как это бы выглядело в хорошем языке"
@Андрей Валяев
Это та подробность и та длина имен, к которой я пришел с опытом. Если писать еще многословнее, отслеживать смысл в дебрях слов становится сложнее. Если же стремиться к лаконичности, то приходится заниматься расшифровкой криптограмм :)
@имя
По поводу 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 по одной штучке, либо туда доходит всего одно число (т.е. их количество)
это можно рассматривать и как плюс, и как минус
@имя:
это могло бы выглядеть так
Могло бы. Но зачем?
И как тебе удалось победить блогспот?
есть такой символ в юникоде 00A0, и вот похоже браузеры его отображают как пробел, но пробелом не считают :-)
А я пришел например к тому, что фигурные скобки надо ставить всегда... так однозначнее и предсказуемее...
А то с 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
не?
Вот это: m_cfg.m_notifies.m_out_pkg_throughput.m_resend_timeout и m_cfg.m_diagnostic_logging.m_rescan_db_for_resending два раза - ну очень длинные имена. :)
@Андрей Валяев
Да, имена длинные. Но если их заменить на методы с короткими именами, которые возвращают нужные значения, то в проекте появятся два вспомогательных метода, которые используются всего в одном месте. Но которые затем будут болтаться в коде и требовать к себе внимания.
Сокращение в одном приведет к увеличению в другом :)
Если бы такие значения требовались в разных местах агента, тогда их добавление было бы оправдано.
@имя:
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 это работает
@имя
AFAIK, работа RVO находится в сильной зависимости от целой кучи факторов, включая фазу Луны. Поэтому если есть сомнения, то я предпочитаю полагаться на собственные руки.
прошу прощения но первый листинг это какойто фарш с макаронами. Крупными такими чтобы подавиться и умереть.
@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, и наследовать от него, а не делать его полем -- тогда опять все бы вытаскивалось на один уровень выше
@имя:
Вопросы отлично показывают стоимоть категоричных оценок чужому коду :) Что, где, как, почему и зачем неизвестно, но код -- отстой ;)
>1. в чем разница между message, package и pkg?
Пакет состоит из сообщений. pkg -- это сокращение для package в случаях, когда идентификатор мог получиться совем уж длинным.
>2. что конкретно логируется таким вот интересным образом (кажется, что логируется время запроса, но еще что)?
Логируется факт начала выборки и факт завершения выборки. В лог добавляется необходимая для анализа этих операций информация -- количество пакетов, которые можно поднять, количество прочитанных сообещний.
>3. select_out_packages_for_sending эта такая обертка чисто для sql-запроса?
Это виртуальный метод интерфейса, который скрывает от агента детали работы с БД (в том числе и тип конкретной БД).
@имя:
а конвертацию в ACE_Date_Time должна производить та функция, которой потребовались например дни месяца, часы, минуты, т.е. скажем sql-запрос
Не катит. Разделение ответственности. Слой работы с БД должен получить конкретную дату/время. Как она получается -- не его забота. И дополнительные преобразования из ACE_Time_Value или еще чего-либо (скажем, из std::tm) ему не нужны.
@имя:
>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, а затем начнут читать у него поля (и скажу -- лучше бы поручить эту оптимизацию компилятору, и не отвлекать пользователя лишним классом)
@имя:
и каким же образом ACE_Time_Value неконкретное?
Его еще в локальное время нужно преобразовывать. Или в не локальное :)
Вообще, все имеет исторические корни. Структуры данных, связанные со слоем БД, изначально хранили дату-время в ACE_Date_Time (поскольку так было проще для основных операций). Поэтому и дальше пошло использоваться соглашение о передаче в слой БД дат и времен в виде ACE_Date_Time.
@имя:
>а если ты вдруг с этим внезапно не согласен, то хотя бы 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%
@имя:
При всем желании сказать, что вот это:
>#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 вместо именованных параметров", поскольку ты их сравнительно мало используешь на практике?
з.ы. они вполне приличны и просты, не то что зверские метапрограммы на шаблонах
з.ы.ы. имхо от *отдельных однострочных* макросов тоже не надо шарахаться
@имя:
>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);
@имя:
Ну и маленькая заноза. Вместо красивого
current_time - cfg.notifies.out_pkg_throughput.resend_timeout
Придется записать:
current_time - ACE_Time_Value(cfg.notifies.out_pkg_throughput.resend_timeout, 0)
А если resend_timeout задается в минутах или в миллисекундах, или вообще в виде вещественного числа (или структуры), то будет еще красочнее.
@имя:
>что будет, если кто-то забудет вызвать 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
з.ы. никто не мешает иметь второй конфиг, где все элементы это строки, но его нужно никому не показывать, кроме валидатора
весьма интересный вопрос для обсуждения, если у тебя исторически сложилось несколько *разных* классов для времени; я тут могу попробовать предложить решение с минимальным переписыванием, и услышать критку
@имя:
>ровно там же, где код и документация к select_out_packages_for_sending
Нет, нет его там. Прототип одного метода с документацией на него -- это одно дело.
У тебя же не один, а несколько методов. Каждый из них должен иметь и свой прототип, и какой-то объект с атрибутами, куда сохраняются параметры.
Вот и где вся эта кухня? Какой у нее объем?
@имя:
>resend_timeout это время?
resend_timeout -- это размер временного отрезка в своих собственных попугаях.
@имя:
>я пока не слышал от тебя обоснований того, что логировать функцию доступа к базе внезапно надо в месте ее использования (а не определения), да и еще 2 раза (до вызова и после вызова)
Почему же внезапно. Есть слой абстракции -- агенты, на этом слое существует такое понятие, как логирование. При этом каждое сообщение в лог привязывается к агенту, из которого оно было залогированно.
Есть другой слой абстракции -- доступ к БД. На этом слое нет понятия логирования, как и нет понятия агента, от имени которого пишутся в лог сообщения.
Посему, поскольку факт обращения к БД (с конкретным параметром, который должен быть в логе) должен быть залогирован, то это делает агент перед обращением к БД. А после обращения к БД логирует факт завершения, опять же с новым параметром.
> Вот и где вся эта кухня? Какой у нее объем?
ее объем будет больше, т.к. она решает бОльшие задачи -- передачу именованных параметров -- и это оправдано
как она будет выглядеть в минимальном переписывании -- щас накатаю
Есть другой слой абстракции -- доступ к БД. На этом слое нет понятия логирования, как и нет понятия агента, от имени которого пишутся в лог сообщения.
и кто мешает во время вызова доступа к БД передать туда лишний дополнительный параметр -- имя агента (кстати, не это ли делает таинственный .finish(*this)?) ?
___________________
что же касается дизайна именованных параметров, то он зависит от того, наличие каких параметров для тебя обязательно; а еще там придется записывать те куски кода, которые тут у тебя не приведены
@имя:
и кто мешает во время вызова доступа к БД передать туда лишний дополнительный параметр -- имя агента
Это смешение слоев абстракции. Нижний слой не должен знать деталей более высокого слоя (или соседнего по своему уровню слоя).
(кстати, не это ли делает таинственный .finish(*this)?) ?
so_log_msg_same_ns -- это макрос, который раскрывается в цепочку chaining methods (и objects так же). Последний в этой цепочке метод finish как раз получает ссылку на агента, инициирующего логирование.
resend_timeout -- это размер временного отрезка в своих собственных попугаях.
да хоть в планковских единицах времени, не важно -- после чтения конфига там должен быть объект стандартного класса времени
исключения из этого правила -- если там у тебя хитрозамороченная система вообще без абсолютных единиц времени, во что я (с ходу) не верю -- т.е. хотя бы после чтения всего конфига тебе уже известны абсолютные значения
@имя:
>ее объем будет больше, т.к. она решает бОльшие задачи -- передачу именованных параметров -- и это оправдано
А вот нафиг это все? Нет в С++ именованных параметров. Поэтому городить огород чтобы передавать пару-тройку параметров в несколько методов -- это не нужное усложнение. Которому место в универсальной библиотеке общего назначения (вроде определения параметров при создания сокета), а не в конкретном прикладном проекте.
@имя:
да хоть в планковских единицах времени, не важно -- после чтения конфига там должен быть объект стандартного класса времени
Во-первых, не факт, что должен быть. Прочитали из конфига в попугаях, значит и лежать может дальше в попугаях. Вдруг это будет проще и удобнее.
Во-вторых, нет в С++ пока стандартного класса для хранения 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
... ну или какая-то иная схема не обязательно с наследованием, но обязательно с полиморфизмом и *незаметной* конвертацией времени в разные классы
@имя:
>вот и у тебя есть это якобы "смешение", которое неизбежно и не плохо; логирование вообще такая вещь, которая не относится строго к одному уровню, и ее приходится протягивать по уровням (соблюдая опреденную аккуратность, конечно)
Какое смешение? Его нет. Есть слой логирования для SObjectizer агентов. Этим слоем пользуется прикладой агент. Снабжая его нужными параметрами. Точно так же он пользуется слоем БД.
Но вот слой БД к логированию от имени SObjectizer-агентов не имеет никакого отношения. И не должен в принципе.
То, что ты позволяешь себе переместить логирование внутрь слоя БД -- это ты решаешь какую-то другую задачу. С другими условиями.
>критикуя мой макрос, сделанный, можно сказать, на замену твоего
Твой макрос я не критиковал. Мне не нравится, что прикладной программист для выполнения прикладных действий должен по месту вводить свои частные макросы. Тогда как макросы so_log_msg_8* -- это части нашей библиотеки логирования, которые прикладной программист просто использует. Так же, как assert() или errno (который вообще не пойми что).
Которому место в универсальной библиотеке общего назначения (вроде определения параметров при создания сокета), а не в конкретном прикладном проекте.
тонюсенькой обертке вокруг sql-я место в прикладном проекте по той причине, что ты, как мне кажется, постоянно селектишь пакеты по времени
мне вообще интересно -- а ты вообще по *другим* признакам их селектишь? и даже если да, то много ли этих признаков?
причем, судя по тому что ты написал, ты уже сделал мини-обертку для абстрагирования от БД, так что добавить парочку классов и десяток методов туда будет и не слишком затратно
@имя:
>попытка вместо них сделать select_out_pkg_for_sending очень похожа на запутывание кода; инлайн этой функции (тут вспомним, что я инлайнер) в виде не то чтобы sql-я, а своей тонюсенькой оберточки вокруг sql-я будет как раз тем, что надо
Еще раз -- кто сказал, что это тонюсенькая обертка? Даже в простейшей реализации "в лоб" это несколько запросов + транзакция. А когда это дело будет тюнится под особенности конкретных версий конкретных типов СУБД это вообще может выродится в несколько файлов с кодом, оптимизированным для разных баз. Поэтому-то и был выделен слой, к которому прикладной уровень обращается в приказном порядке -- "сделай вот это", а как это будет сделано, какой кровью и с какими подробностями... Об этом прикладной слой и знать не должен без лишней на то необходимости.
Но вот слой БД к логированию от имени SObjectizer-агентов не имеет никакого отношения. И не должен в принципе.
То, что ты позволяешь себе переместить логирование внутрь слоя БД -- это ты решаешь какую-то другую задачу. С другими условиями.
я ориентируюсь на это:
Логируется факт начала выборки и факт завершения выборки.
поскольку функция select_out_packages_for_sending точно знает оба этих факта, и не знает только то, от имени кого (или как) надо произвести логирование, ей надо это логирование перепоручить, передав ссылку на логирующий аппарат
тем самым уберется дублирование кода и возможные ошибки
это я уже 2-й раз говорю, только немного четче
Еще раз -- кто сказал, что это тонюсенькая обертка?
(насчет толщины) ну уж ведь не гибернейт у тебя там, а?
а если это не тонюсенькая обертка, то мои доводы насчет необходимости поддержки именованных параметров только усиливаются
в дополнение к фактам:
В лог добавляется необходимая для анализа этих операций информация -- количество пакетов, которые можно поднять, количество прочитанных сообещний.
и функция select_out_packages_for_sending знает эти количества, причем *лучше* значет, чем агент; т.е. в случае вызвова логирования со стороны агента эти количества приходится в лог передавать в явном виде
@имя:
>это я уже 2-й раз говорю, только немного четче
Да хоть в 25-й. Не должен слой БД заниматься логированием. Вообще.
Мой код был написан с учетом этого ограничения. Не было бы его, код был бы другим.
Не должен слой БД заниматься логированием. Вообще.
ок, допустим у нас есть dll-ка архиватор, и он не должен сам показывать гуевое окошко с процентом выполнения (это смешение слоев), но должен как-то рапортовать о проценте выполнения задачи
решение -- архиватор вызывает callback
точно так же и здесь из около-БД-шного кода можно вызывать callback, который будет логировать от имени соотв. агента
так устраивает?
@имя
>точно так же и здесь из около-БД-шного кода можно вызывать callback, который будет логировать от имени соотв. агента
Не нужен здесь никакой callback. И БД вообще не в курсе, нужно ли что-то логировать, в какой мере и при каких условиях.
БД либо возвращает результат, либо бросает исключение.
Это агент знает, что вот сейчас он вызовет слой БД. Сей факт при определенных условиях (о которых знает агент) должен быть залогирован. А вот агент получил результат от слоя БД. И опять-таки, только при условиях, о которых знает агент, сей факт логируется.
Все это отражено в коде, который, как я вижу, воспринимается вполне полно (в плане логики, а не деталей). Настолько полно, что есть предложения по его улучшению. Значит код не так уж и плох. Если бы он был не понятен, то его бы не улучшали, а выясняли что и как он делает. Т.е. цель -- написать нормальный, читабельный прикладной код -- была достигнута.
я заметил что не получил тот ответ только сейчас лежа с гриппом на больничном. Сел и порубил крупные куски для примера и сунул в http://pastie.org/3720201
я понимаю что вкусовщина но мало ли :)
@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:
На мой взгляд, по сути переделок не было, основные черты кода остались неизменныи. Стало чуть многословнее. Читать код мне это ничуть не мешало. Но писать столько лично мне было бы в лом.
окей попробую пояснить.
1)апи лога засоряет локальный контекст именами которые не нужны там. Это одно из немногих мест где я допускаю макросы - весь мусор надо убрать и си увы иначе не позволяет.
2)многократное чтение конфигурации. вы правда хотите чтобы однажды те 2 ифа сработали по разному?
3)авторы апи не позаботились об краткой записи имени смартпойнтера. Тайпдефу или даже дефайну место там.
4)6 лишних пробелов слева при ограничении в 80 дают жуткие лесенки в 3 ряда. Обратите внимание что 2! ряда это предел после которого это уже неприемлемо.
5) сложные многострочные if/... без скобок.
вот навскидку
@Miroslav:
1)апи лога засоряет локальный контекст именами которые не нужны там.
Пряча их использование мы увеличиваем количество сущностей, которыми должен оперировать программист. При этом этот конкретный API используется только здесь и сейчас. В других методах он другой.
авторы апи не позаботились об краткой записи имени смартпойнтера.
Не вижу в этом смысла, если a) в имени смартпоинтера обязательно нужно отразить тот факт, что это auto_ptr, b) это имя используется считанное количество раз.
4)6 лишних пробелов слева при ограничении в 80 дают жуткие лесенки в 3 ряда.
Я сам придерживаюсь ограничения в 80 символов. И лесенка в 3 ряда -- это вовсе не жуть.
Да и if-ы вовсе не сложные. Простое условие, один вызов под if-ом.
Отправить комментарий