Брутально и бессердечно о программировании и проектировании
ГлавнаяФорумАртПаттерныАнтипаттерныТест-драйвЗаметкиКнигорецензииСправочная

26 августа 2007, Vitamin

Показать комментарии
Решил выслать вам немного кода на рецензию.
1) Примитив- простенькая обертка для управления объектами (примитивный велосипед, но писал давно, когда шаблоны изучал
0
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
template<class T, void (T::*_ctor)(), void (T::*_dtor)()>
class AutoEnv
{
        AutoEnv(const AutoEnv&);
        AutoEnv& operator=(const AutoEnv&);
public:
        AutoEnv(T& rc) : rc_(rc)
        {
                (rc_.*_ctor)();
        }
        ~AutoEnv()
        {
                (rc_.*_dtor)();
        }
private:
        T& rc_;
};
Как использовать, думаю, рассказывать не надо :) Что бы доработал- обернуть вызов в деструкторе в try...catch
Для того, чтобы запретить копирование, лучше использовать private наследование от boost::noncopyable. Если у вашего класса появятся друзья, то они смогут копировать объекты вашего класса. В случае с boost::noncopyable это будет невозможно.

Отлавливать исключения в деструкторе не нужно; на это существуют две явные причины. Во-первых, если T::_dtor сгенерирует исключение, то это будут проблемы исключительно T::_dtor; ваш класс тут совершенно не при чем. Во-вторых, ваш класс не знает, как именно следует обработать исключение. Способ обработки исключения может зависеть не только от типа исключения и места его возникновения, но и от места его перехвата. Ваш класс не знает, с кем именно он работает, с каким-то глобальным синглтоном, или с какой-то локальной мелочевкой. Если вы будете «глушить» исключения в деструкторе вашего класса, то вы лишите вашу программу возможности адекватно на них реагировать. Можно, конечно же, делать переброску, но только зачем? Все равно исключения должны отлавливаться кем-то снаружи.
Ответственность это конечно хорошо, только не нужно решать те проблемы, которые вас совершенно не касаются. Тем более если это приводит к неправильной работе программы.

Помимо всего перечисленного, отлов исключений в деструкторе вашего класса в некоторых случаях может нарушать транзакционность внешнего кода.
2) Маленький финт ушами на тему настроек в программе. Хранение строковых и целочисленных опций с буферизацией и перезагрузкой (внешней). Потокобезопасная.
0
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
//options interface with bufferization and timechecking
template<const std::string& name>
std::string getString(const char* defval = "") const
{
static Mutex mutex;
static std::string value;
static time_t lasttime = 0;
       {
              Locker locker(mutex);
              if (lasttime != settings_.timestamp())
              {
                 lasttime = settings_.timestamp();
                 value = settings_.getString(name, defval);
              }
       }
       return value;
}

//same for integer
template<const std::string& name>
int getInteger(int defval = 0) const
{
..
}
Благородные цели, но, к сожалению, плохая реализация.

Исходя из того что функции константны, можно сделать вывод, что settings_ либо глобален, либо объявлен с модификатором mutable, либо нечто подобное происходит внутри settings_::getXXX. В любом их этих случаев это плохо. Свойство mutable, естественно, имеет право на существование, но использовать его следует лишь в особо специфических ситуациях, коей ваша не является. На счет того, что глобальные переменные это плохо, надеюсь, возражений нет.

Функция не является «потокобезопасной». Зачем вы внесли Locker во внутренний скоуп? Когда будет создаваться копия объекта value, мутекс уже будет свободен, а это значит что в момент копирования value инструкцией return value; в эту самую value кто-то другой может присваивать новое значение. А стандартная библиотека не является «потокобезопасной».

Идем дальше. Ваш settings_ это ошибка, которая встречается в очень многих программах. Я даже думаю выделить и описать ее как отдельный антипаттерн. Конфиги придуманы только потому, что системным администраторам так удобнее конфигурировать программу, имея лишь текстовый редактор. Делать конфиг элементом дизайна архитектуры — грубейшая ошибка. Конфиг это огромная куча, в которую свалили настройки всех мастей. Старая пословица гласит: «не кладите все яйца в одну корзину».

К сожалению, C++ не позволяет реализовать универсальный автоматический механизм кеширования, прозрачный как для функциональности, так и для ее пользователя. Существует три основных способа организации кеширования: кеширование внутри функциональности (как это сделано у вас), кеширование на уровне вызова (пользователю приходится вызывать функции определенным образом) и создание прокладки-адаптера между пользователем и функциональностью. Адаптер имеет один единственный недостаток — При изменении интерфейса функциональности, придется менять интерфейс адаптера. Эту проблему можно частично автоматизировать, но, к сожалению, только частично. Тем не менее, я рекомендую использовать именно адаптер. При правильном подходе к проектированию, функциональности, нуждающейся в кешировании, будет не так много, если она будет вообще. Не более чем несколько классов. Да, при изменении интерфейса скорее всего вам придется поправить адаптер. Но зато кеширование останется полностью невидимым как для самой функциональности, так и для ее пользователей.
3) Loggers.h - моя попытка сочетать статический и динамический полиморфизм. Задача- журналировать разные системные события с разным уровнем детализации и минимальными затратами (ибо обычно параметры, передающиеся в журнал должны форматироваться в строку). Ограничения- нельзя использовать сторонние библиотеки (только STL).
Пример использования:
-собственно, функция журналирования,доступная всем компонентам (поуказателю):
0
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
void Core::logging(const log::LogMessage& msg) const
{
static int donelines[2] = {};

        if ((msg.category() == MSG_NOTICE && msg.loglevel() >
getInteger<OptionVerbose>(VERBOSE_MIN)) ||
            (msg.category() == MSG_WARNING && msg.loglevel() >
getInteger<OptionWarninglevel>(VERBOSE_MIN))) return;

        FILE* logfile = msg.category() == MSG_NOTICE ?
        logFile_.asFile() :
        errFile_.asFile();
        int& doneline = donelines[msg.category() == MSG_NOTICE];
        struct tm thisTime;
        time_t thistim = time(NULL);
        localtime_r(&thistim, &thisTime);
        fprintf
        (
            logfile,
            "\n%02u-%02u-%02u %02i:%02i:%02i>%.*s",
            thisTime.tm_mday,
            thisTime.tm_mon + 1,
            thisTime.tm_year + 1900,
            thisTime.tm_hour,
            thisTime.tm_min,
            thisTime.tm_sec,
            msg.loglevel() * 2 - 1,
            "                                 "
        );
        msg.put(logfile);
        if (++doneline >= getInteger<OptionFlushlines>(100))
        {
                fflush(logfile);
                doneline = 0;
        }
}
Я немножко подрихтовал исходник, чтобы он более-менее влезал по ширине.

К сожалению, все еще хуже, чем с кешированием.

Судя по всему, все, что касается работы с ядром, вы запихиваете в класс Core. Это плохо, поскольку это неизбежно ведет к раздуванию класса, а именно — к антипаттерну «Blob». Core — так должно называться пространство имен, но никак не единственный класс.

Почему функция константна? Ведь запись в лог это изменение состояния объекта. А я скажу вам, почему. Потому что для работы с файлами вы используете «сишную» функциональность. Во-первых, вы обманываете пользователя с константностью. Во-вторых, сишный код имеет море недостатков. Работа с простыми указателями либо не безопасна с точки зрения исключений, либо нарушает инкапсуляцию. Либо и то, и другое. Форматирование в printf-стиле не проверяет типы на этапе компиляции и не контролирует вылезание за выделенную область в стеке. В C++ нельзя программировать на Plain C!

Использование результата булевой операции в качестве индекса массива это дурной тон. Это говорит о том, что вы не смогли решить задачу изящно. Это тоже ошибка. Например в C# запрещены арифметические операции над типом bool. Надеюсь, что когда-нибудь их запретят и в C++.

На счет конфигов я вам уже все сказал.

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

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

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

Посмотрите как сделан лог в библиотеке boost.
4) Template.h Несколько простеньких оберток для файлов (FileDesc, FDDesc).
Классы реализации шаблонов. Применялись для заполнения полей текстовых страниц:
StaticTemplate - поля инстанциируются значениями на момент задания полей
DynamicTemplate - то же самое, но на момент создания шаблона
StaticMultipleTemplate - аналогично первому, но поддерживает группы полей.
FileKeeper - хранитель содержимого файла с автоматическим контролем изменения содержимого (да, не кроссплатформенный....). Пример использования (выдернуто из контекста, посему может в лоб не компиляться):
0
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
FileKeeper<std::string> keeper;
static const std::string Field1("[f1]");
static const std::string Field2("[f2]");

       StaticTemplate<std::string> templ;
       templ.add(Field1, field1_value);
       templ.add(Field2, field2_value);
       cout << templ.pass(keeper.get(template_filename));
                
...
class Strings
{
public:
      static const std::string begGroup;
      static const std::string endGroup;
      ...
};

const std::string Strings::begGroup("{begin}");
const std::string Strings::endGroup("{end}");
...

std::vector<std::string> valueslist;
static const std::string FieldVal("[val]");

   // fill valueslist
   valueslist.push_back("1");
   valueslist.push_back("2");
   ...
   valueslist.push_back("8");
   StaticMultipleTemplate<std::string, std::vector> templ;
   //single fields
   templ.add(Field1, "field1");
   templ.add(Field2, "field2");
   //multiple fields
   templ.add(FieldVal, valueslist);
   ...
   //pass
   cout << templ.pass<Strings::begGroup,
   Strings::endGroup>("[f1]:{begin}[val],{end}:[f2]");

   out:
   field1:1,2,3,4,5,6,7,8,:field2
Код писался для себя, но позже был применен в одном рабочем проекте (внутреннего использования в компании). Но поскольку никаких ограничений по использованию кода нет (в разумных пределах конечно), использую наработки и в других своих личных проектах.
Vitamin/CAIG/2001
Ваш код, к сожалению, содержит очень много ошибок. На столько много, что я даже не стану их расписывать. Для начала проработайте те ошибки, о которых я рассказал вам выше.

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

Оглавление
Статистика
© 2007—2009 Inside C++ Коммерческие услугиКонтактная информация

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