Покритикуйте код
От: CEMb  
Дата: 05.02.15 09:18
Оценка: +1
Почитал соседнюю ветку, и вспомнилось...

Принцип создания функции инициализации:

BOOL CApp::InitCommon()
{
    try
    {
        if(!m_zero.InitCommon())
            throw 0;
        if(!m_one.InitCommon())
            throw 1;
        if(!m_two.InitCommon())
            throw 2;
        //...
    }
    catch(int iExc)
    {
        switch(iExc)
        {
        //...
        case 3:
            m_two.UninitCommon();
        case 2:
            m_one.UninitCommon();
        case 1:
            m_zero.UninitCommon();
        }
        return FALSE;
    }
    return TRUE;
}

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

чтоб понятнее, в чём суть, лучше так:

BOOL CApp::Init()
{
    try
    {
        m_hSome0 = InitSome0()
        if (!m_hSome0)
            throw 0;
        m_hSome1 = InitSome1()
        if (!m_hSome1)
            throw 1;
        m_hSome2 = InitSome2()
        if (!m_hSome2)
            throw 2;
        //...
    }
    catch(int iExc)
    {
        switch(iExc)
        {
        //...
        case 3:
            Release(hSome2);
        case 2:
            Release(hSome1);
        case 1:
            Release(hSome0);
        }
        return FALSE;
    }
    return TRUE;
}


Часто в примерах встречал код, который инициализировал некоторые члены класса один за другим, в случае ошибки делал деинициализацию уже готовых. В результате к хвосту функции число вызова деинициализаторов было велико. Ну и повторы кода на каждом условии.
Как вариант, люди использовали некую булеву переменную для понимания, что инициализации идут хорошо, каждая новая инициализация обкладывалась условием на эту переменную. В конце эта переменная проверялась, если false, то проверялись опять же все хендлы на валидность, и делалась деинициализация.
Мне кажется, мой код проще и нагляднее, быстро расширяется, можно не бояться забыть, что что-то пропустил.
Жду: критику, доработку, идеи
Re: Покритикуйте код
От: Mr.Delphist  
Дата: 05.02.15 09:26
Оценка: 25 (5) +8
Здравствуйте, CEMb, Вы писали:

CEM>Часто в примерах встречал код, который инициализировал некоторые члены класса один за другим, в случае ошибки делал деинициализацию уже готовых. В результате к хвосту функции число вызова деинициализаторов было велико. Ну и повторы кода на каждом условии.


RAII

CEM>Как вариант, люди использовали некую булеву переменную для понимания, что инициализации идут хорошо, каждая новая инициализация обкладывалась условием на эту переменную. В конце эта переменная проверялась, если false, то проверялись опять же все хендлы на валидность, и делалась деинициализация.


RAII

CEM>Мне кажется, мой код проще и нагляднее, быстро расширяется, можно не бояться забыть, что что-то пропустил.


RAII

CEM>Жду: критику, доработку, идеи


Основная идея — пусть деинициализацию делает компилятор, автоматически. Благо сейчас уже не надо втягивать себе буст для этого — необходимые решения уже в Стандартной библиотеке. Что-то пошло не так? Срабатывает вываливание из скоупа и соответствующий авто-вызов деструктора. А рукопашный код плох тем, что стопудово что-то забудешь. Да и код замусоривается в портянку. В общем, не устану повторять — RAII.
Re: Покритикуйте код
От: Evgeny.Panasyuk Россия  
Дата: 05.02.15 09:55
Оценка: 5 (2)
Здравствуйте, CEMb, Вы писали:

CEM>Принцип создания функции инициализации:


Если нормальный RAII, без двухфазной инициализации (или что там у тебя), с нормальными RAII полями, ну ни как не получается (почему?), то можно:
BOOL CApp::InitCommon()
{
    return initialize(m_zero, m_one, m_two);
}
Где initialize это variadic шаблон функции, который прячет внутри всю эту логику.

Либо извратится через RAII обвёртки + ScopeGuard:
BOOL CApp::InitCommon()
{
    try
    {
        auto a = init(m_zero);
        auto b = init(m_one);
        auto c = init(m_two);
        // ...
        commit(a, b, c);
    }
    catch(init_failure)
    {
        return false;
    }
    return true;
}
init возвращает ScopeGuard, который при отсутствии commit'а сделает uninit в деструкторе. А с использованием stack_unwinding commit можно вообще убрать.

Либо просто goto:
BOOL CApp::InitCommon()
{
    if(!m_zero.InitCommon())
        goto ERROR_zero;
    if(!m_one.InitCommon())
        goto ERROR_one;
    if(!m_two.InitCommon())
        goto ERROR_two;
    
    //...
    return true;

    ERROR_two:
        m_two.UninitCommon();
    ERROR_one:
        m_one.UninitCommon();
    ERROR_zero:
        m_zero.UninitCommon();

    return false;
}
Уж точно проще и лаконичней твоего варианта
Re: Покритикуйте код
От: denisko http://sdeniskos.blogspot.com/
Дата: 05.02.15 10:00
Оценка:
Здравствуйте, CEMb, Вы писали:

CEM>Почитал соседнюю ветку, и вспомнилось...


CEM>Принцип создания функции инициализации:


Лучше сделай [аналог] boost::optional + RAII.



CEM>Часто в примерах встречал код, который инициализировал некоторые члены класса один за другим, в случае ошибки делал деинициализацию уже готовых. В результате к хвосту функции число вызова деинициализаторов было велико. Ну и повторы кода на каждом условии.

RAII спасет отца.


CEM>Как вариант, люди использовали некую булеву переменную для понимания, что инициализации идут хорошо, каждая новая инициализация обкладывалась условием на эту переменную.

boost::optional или свой аналог.
<Подпись удалена модератором>
Re: Покритикуйте код
От: Kernan Ниоткуда https://rsdn.ru/forum/flame.politics/
Дата: 05.02.15 10:10
Оценка: +3
Здравствуйте, CEMb, Вы писали:

CEM>Почитал соседнюю ветку, и вспомнилось...

Тут у тебя концептуальная проблема. Ты используешь исключения не совсем по назначению в качестве управляющих конструкций.
CEM>Мне кажется, мой код проще и нагляднее, быстро расширяется, можно не бояться забыть, что что-то пропустил.
RAII ещё проще и нагляднее.
Sic luceat lux!
Re[2]: Покритикуйте код
От: andyp  
Дата: 05.02.15 11:03
Оценка:
Здравствуйте, Evgeny.Panasyuk, Вы писали:

EP>[/ccode]init возвращает ScopeGuard, который при отсутствии commit'а сделает uninit в деструкторе. А с использованием stack_unwinding commit можно вообще убрать.


Евгений, можешь рассказать, чего я глобально лишаюсь, используя только standalone версию библиотеки + лямбды в качестве action? Тяжеловато продраться через бустовский чудеса с препроцессором.
Re[2]: Покритикуйте код
От: CEMb  
Дата: 05.02.15 12:02
Оценка: -1
Здравствуйте, Kernan, Вы писали:

K>Тут у тебя концептуальная проблема. Ты используешь исключения не совсем по назначению в качестве управляющих конструкций.


Почему? Произошла ошибка, выкинул исключение, обработка дальше.
Re[2]: Покритикуйте код
От: CEMb  
Дата: 05.02.15 12:08
Оценка:
Здравствуйте, Mr.Delphist, Вы писали:

MD>RAII


Ага, это да, хорошо.
Но если в моём случае вызова статических методов для инициализации статических данных классов? Каждый раз при создании экземпляра класса проверять, было ли загружено статическое, как-то нехорошо?

Переобобщу вопрос: как сделать загрузку статических данных классов перед стартом основной программы. Подходит ли мой метод?
Re: Покритикуйте код
От: Кодт Россия  
Дата: 05.02.15 13:07
Оценка: 4 (1)
Здравствуйте, CEMb, Вы писали:

CEM>Принцип создания функции инициализации:

CEM>
CEM>            throw 0;
CEM>            throw 1;
CEM>            throw 2;
CEM>


То, что в плюсах можно кидать любые типы, ещё не значит, что можно кидать любые типы!

CEM>
CEM>        switch(iExc)
CEM>        {
CEM>        //...
CEM>        case 3:
CEM>            Release(hSome2);
CEM>        case 2:
CEM>            Release(hSome1);
CEM>        case 1:
CEM>            Release(hSome0);
CEM>        }
CEM>


Свитч с объединением веток в стиле Устройства Дуффа — ошибкоопасный стиль программирования.
И вообще: а если у тебя вылетит другое исключение, что будешь делать, как отобразишь его на точку отката?

Пары Init-Deinit навевают идею RAII. Возможно, что переделка приведёт к изрядному переписыванию, и даже возможно, что к архитектурным изменениям. Но они будут того стоить, ибо отразят суть.

Например, собрать все поля CApp, устанавливаемые в процедуре Init(), в одну структуру.
Перекуём баги на фичи!
Re: Покритикуйте код
От: __kot2  
Дата: 05.02.15 13:22
Оценка:
Здравствуйте, CEMb, Вы писали:
CEM>Жду: критику, доработку, идеи
жуткая хрень, конечно
нормальный код:

CEM>
class CApp:
    Zero zero
    One one
    Two two
CEM>CApp::CApp(){}


если хочется прямо ловить исключения при инициализации внутренних классов и подменять на другой тип, то есть одна резкоиспользуемая штука вроде:
class CApp:
Zero zero
One one
Two two
CEM>CApp::CApp() try{ zero(), one(), two()) catch (){} {}
то есть блок try-catch в списоке инициализации
за точность синтаксиса не уверен
Отредактировано 05.02.2015 13:26 __kot2 . Предыдущая версия .
Re: Покритикуйте код
От: chaotic-good  
Дата: 05.02.15 13:49
Оценка:
Здравствуйте, CEMb, Вы писали:

CEM>Почитал соседнюю ветку, и вспомнилось...


CEM>Принцип создания функции инициализации:


CEM>
CEM>BOOL CApp::InitCommon()
CEM>{
CEM>    try
CEM>    {
CEM>        if(!m_zero.InitCommon())
CEM>            throw 0;
CEM>        if(!m_one.InitCommon())
CEM>            throw 1;
CEM>        if(!m_two.InitCommon())
CEM>            throw 2;
CEM>        //...
CEM>    }
CEM>    catch(int iExc)
CEM>    {
CEM>        switch(iExc)
CEM>        {
CEM>        //...
CEM>        case 3:
CEM>            m_two.UninitCommon();
CEM>        case 2:
CEM>            m_one.UninitCommon();
CEM>        case 1:
CEM>            m_zero.UninitCommon();
CEM>        }
CEM>        return FALSE;
CEM>    }
CEM>    return TRUE;
CEM>}
CEM>

CEM>у меня конкретно в коде были вызовы статических методов классов для инициализации общих данных, (через C:: ).

Можно вот так вот сделать (развлечения ради, понятно что тут нужен RAII).

int opcnt = 0;
if (m_zero.InitCommon()) {
   opcnt++;
   if (m_one.InitCommon()) {
       opcnt++;
       if (m_two.InitCommon()) {
           opcnt++;
           goto OK;
       }
   }
}

switch(opcnt) {
case 3:
    m_two.UninitCommon();
case 2:
    m_one.UninitCommon();
case 1:
    m_zero.UninitCommon();
}

return FALSE;

OK:

return TRUE;

Во имя Сатаны!

CEM>Жду: критику, доработку, идеи


RAII нужно использовать или scope exit какой-нибудь как тут уже писали.
Re[3]: Покритикуйте код
От: Mr.Delphist  
Дата: 05.02.15 16:05
Оценка:
Здравствуйте, CEMb, Вы писали:

CEM>Здравствуйте, Mr.Delphist, Вы писали:


MD>>RAII


CEM>Ага, это да, хорошо.

CEM>Но если в моём случае вызова статических методов для инициализации статических данных классов? Каждый раз при создании экземпляра класса проверять, было ли загружено статическое, как-то нехорошо?

А что меняет слово static?
Re[3]: Покритикуйте код
От: Evgeny.Panasyuk Россия  
Дата: 05.02.15 18:15
Оценка: 16 (2)
Здравствуйте, andyp, Вы писали:

EP>>[/ccode]init возвращает ScopeGuard, который при отсутствии commit'а сделает uninit в деструкторе. А с использованием stack_unwinding commit можно вообще убрать.

A>Евгений, можешь рассказать, чего я глобально лишаюсь, используя только standalone версию библиотеки + лямбды в качестве action? Тяжеловато продраться через бустовский чудеса с препроцессором.

Boost.ScopeExit писался в первую очередь для pre-C++11 кода. Например там эмулируются локальные функции, в то время как в C++11 уже есть готовые лямбды. Ничем не уступающая реализация ScopeExit, и даже превосходящая, в C++11 займёт всего два десятка строк.

Библиотека stack_unwinding даёт возможность реализовать не только scope(exit) из языка D, но и scope(failure) и scope(success), так как предоставляет unwinding_indicator (позволяющий узнать завершился ли scope исключением или нет).

Вот полная реализация scope(exit)/scope(failure)/scope(success) в одном файле.

Также, на базе кода из stack_unwinding, аналогичная реализация есть в библиотеке Facebook.Folly. У Александреску даже есть выступление на эту тему:
http://vimeo.com/channels/ndc2014/97329153
http://rsdn.ru/forum/cpp/5637880.flat
Автор: Skorodum
Дата: 06.06.14
Отредактировано 05.02.2015 18:18 Evgeny.Panasyuk . Предыдущая версия .
Re[4]: Покритикуйте код
От: andyp  
Дата: 05.02.15 18:36
Оценка:
Здравствуйте, Evgeny.Panasyuk, Вы писали:

EP>Библиотека stack_unwinding даёт возможность реализовать не только scope(exit) из языка D, но и scope(failure) и scope(success), так как предоставляет unwinding_indicator (позволяющий узнать завершился ли scope исключением или нет).


Это я понял. У тебя на гитхабе в библиотеке два фолдера — один standalone, второй boost. В standalone есть scope_exit, scope_failure, scope_success, которые отпределяются с помощью unwinding_indicator. С этим все понятно. Но есть еще фолдер boost, там определяются всякие хитрые макросы. Я собственно про них и спрашивал — что я теряю не используя их.

EP>Вот полная реализация scope(exit)/scope(failure)/scope(success) в одном файле.


Спасибо.

EP>Также, на базе кода из stack_unwinding, аналогичная реализация есть в библиотеке Facebook.Folly. У Александреску даже есть выступление на эту тему:

EP>http://vimeo.com/channels/ndc2014/97329153
EP>http://rsdn.ru/forum/cpp/5637880.flat
Автор: Skorodum
Дата: 06.06.14


Спасибо
Re[5]: Покритикуйте код
От: Evgeny.Panasyuk Россия  
Дата: 05.02.15 18:48
Оценка:
Здравствуйте, andyp, Вы писали:

A>Это я понял. У тебя на гитхабе в библиотеке два фолдера — один standalone, второй boost. В standalone есть scope_exit, scope_failure, scope_success, которые отпределяются с помощью unwinding_indicator. С этим все понятно. Но есть еще фолдер boost, там определяются всякие хитрые макросы. Я собственно про них и спрашивал — что я теряю не используя их.


Там реализуются полные аналоги макросов BOOST_SCOPE_EXIT*: BOOST_SCOPE_FAILURE и BOOST_SCOPE_SUCCESS. Они работают и на C++98.
Я думал попытаться включить их в Boost.ScopeExit, поэтому сделал proof-of-concept (особо не заморачиваясь с выносом общих частей из макросов, просто сделав копию boost/scope_exit.hpp + небольшие правки). Но текущий мейнтейнер Boost.ScopeExit практически никак не реагировал на дискуссию и это дело зависло.

В общем, если у тебя C++11 или выше, то эти макросы и не нужны.
Отредактировано 05.02.2015 18:49 Evgeny.Panasyuk . Предыдущая версия .
Re[6]: Покритикуйте код
От: andyp  
Дата: 05.02.15 18:50
Оценка:
Здравствуйте, Evgeny.Panasyuk, Вы писали:

EP>Там реализуются полные аналоги макросов BOOST_SCOPE_EXIT*: BOOST_SCOPE_FAILURE и BOOST_SCOPE_SUCCESS. Они работают и на C++98.

EP>Я думал попытаться включить их в Boost.ScopeExit, поэтому сделал proof-of-concept (особо не заморачиваясь с выносом общих частей из макросов, просто сделав копию boost/scope_exit.hpp + небольшие правки). Но текущий мейнтейнер Boost.ScopeExit практически никак не реагировал на дискуссию и это дело зависло.

Понял, спасибо.
Re[3]: Покритикуйте код
От: Kernan Ниоткуда https://rsdn.ru/forum/flame.politics/
Дата: 06.02.15 06:30
Оценка:
Здравствуйте, CEMb, Вы писали:

CEM>Здравствуйте, Kernan, Вы писали:


K>>Тут у тебя концептуальная проблема. Ты используешь исключения не совсем по назначению в качестве управляющих конструкций.


CEM>Почему?

Потому что ты мыслишь как С-шник и нарушаешь инкапсуляцию. Фактически ты переписал логику с goto через try-throw-catch.
CEM>Произошла ошибка, выкинул исключение, обработка дальше.
Выкинул, но не там. Проблемы начались когда ты придумал метод InitiCommon вместо использования конструктора. Пример, ты же не заглядываешь каждый раз в цилиндр своей машины когда едешь чтобы проверить есть ли пропуски впрыска или нет. У тебя этим занимается сам двигатель который сообщает что у него есть пропуски в конкретном месте.
Sic luceat lux!
Re[4]: Покритикуйте код
От: CEMb  
Дата: 06.02.15 12:47
Оценка:
Здравствуйте, Mr.Delphist, Вы писали:

CEM>>Но если в моём случае вызова статических методов для инициализации статических данных классов? Каждый раз при создании экземпляра класса проверять, было ли загружено статическое, как-то нехорошо?


MD>А что меняет слово static?


я делаю инициализацию объекта в конструкторе. Но для статика явного конструктора нет.
Re[2]: Покритикуйте код
От: CEMb  
Дата: 06.02.15 12:51
Оценка:
Здравствуйте, Кодт, Вы писали:

К>То, что в плюсах можно кидать любые типы, ещё не значит, что можно кидать любые типы!


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

К>Свитч с объединением веток в стиле Устройства Дуффа — ошибкоопасный стиль программирования.

К>И вообще: а если у тебя вылетит другое исключение, что будешь делать, как отобразишь его на точку отката?

Это да, в этом месте будет "потеряна" информация о этапах инициализации.

К>Пары Init-Deinit навевают идею RAII. Возможно, что переделка приведёт к изрядному переписыванию, и даже возможно, что к архитектурным изменениям. Но они будут того стоить, ибо отразят суть.


Ну я обычно так и делаю, но тут static
Re[4]: Покритикуйте код
От: CEMb  
Дата: 06.02.15 13:00
Оценка:
Здравствуйте, Kernan, Вы писали:

K>>>Тут у тебя концептуальная проблема. Ты используешь исключения не совсем по назначению в качестве управляющих конструкций.


CEM>>Почему?

K>Потому что ты мыслишь как С-шник и нарушаешь инкапсуляцию. Фактически ты переписал логику с goto через try-throw-catch.

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

CEM>>Произошла ошибка, выкинул исключение, обработка дальше.

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

Тут проблема в статических членах классов. У меня есть несколько классов в которых есть статические вектора. Их всех надо заполнить перед работой программы (из файлов). Классы никак между собой и с программой явно не связаны, т.е. не зависят от других классов и самой программы.

И отдельный вопрос про нормальность выкидывания исключения в мной приведённом коде Что там не так?
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.