Просьба о code review - Qt

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

S>Уважаемые, если у кого есть время, желание итд, пожалуйста просмотрите проект домашней бухгалтерии https://github.com/Sheridan/HAcc
S>В логику можно не вникать, мне хочется знать что там может быть не так, неправильно, или некрасиво.

Про логику
1. Зачем синглетонов напихал?
2. Здесь <HAcc / trunk / src / model> чего такое???

file ccontractors.cpp April 21, 2011 Создание и редактирование валюты [Sheridan]

Как получается, что контрагенты создают и редактируют валюты?
3. Здесь <HAcc / trunk / src / model / ccurrencyes.h>

//! Синглон валют
/*!
Управляет контрагентвми. Непосредственно управляет таблицами
- currencyes
@see CCurrency
*/
class CCurrencyes :

Кто чем управляет?

Дальше извини, тяжело смотреть код с невнятной логикой.

checkSpetialPurposeTag -> checkSpecialPurposeTag (и прочие Spetial)
CSingleTone -> CSingletone
CTransactionsPools -> CTransactionPools, transactions_pool -> transaction_pool

Ни во что не вникал, Qt не знаю

еще бросаются в глаза неэффективности типа

QAction * CBases::constructAction(EActionsTypes atype, const hacc::TDBID &id, QObject *reciever, const char * method)
{
    if(m_actions[atype][id] == NULL)
    {
        QAction *gAction = generateAction(atype, reciever, method);
        gAction->setEnabled(HACC_DB->isOpen());
        connect(HACC_DB, SIGNAL(stateChange(bool)), gAction, SLOT(setEnabled(bool)));
        m_actions[atype][id] = gAction;

    }
    return m_actions[atype][id];
}

выражение m_actions[atype][id] повторяется дважды-трижды (в зависимости от ифа), а ведь это каждый раз двойной поиск по мапам, т.е. операция совсем небыстрая в общем случае.

Плюс непонятно, что будет, если generateAction() вернет NULL, если такое может быть, конечно.

1) Не понравилась конвертация string->money,amount. Надо использовать QLocale::decimalPoint, ну и плюс хранение денег просто в double без отдельного класса, это та еще головная боль при поиске копеек.
2) Много макросов.
3) Не внятное владение указателями на объект в CFileDialogs

QFileDialog *CFileDialogs::fileDialog(...)
{
    QFileDialog *dlg = new QFileDialog(HACC_WINDOW);// у dlg как я понимаю есть родитель
}

SFileDialogResult CFileDialogs::getOpenFileName(const QString &title, const QString &extentions)
{
    QFileDialog *dlg = fileDialog(title, extentions);// здесь создали объект и сделали ему родителя
    dlg->setFileMode(QFileDialog::ExistingFile);
    return execFileDialog(dlg, title, extentions);// здесь на прямую удалили dlg
}

Мне все равно как форматируются исходники и по какому принципу называются классы, но такое встречаю первый раз

, ненадолго впал в ступор

                CDatabase::~CDatabase() { close();}
const QString & CDatabase::dbFileName() { return m_fileName; }
           bool CDatabase::isOpen    () { return m_db.isOpen(); }

4) Заходя в src/model думал будут наследники QAbstractModel.

5) При первом запуске программа не смогла поместится на экран, создавая доки используй tabifyDockWidget.
6) В модальные диалоги передавай parent, чтобы не было лишних движений в (как называется то место где все программы появляются после запуска?) Ну и убери знак (?) в диалогах.

Если уж используете синглтон, то закрывайте конструктор копирования и соответствующий оператор:

private:
  MySingletone(const MySingletone&);
  MySingletone& operator=(const MySingletone&);

Кроме того в программе присутствуют бесконечные delete, даже для диалогов. В Qt же есть отличный механизм удаления объектов, унаследованных от QObject.
Вот, например:

WAbstractScrollArea::~WAbstractScrollArea()
{
    delete m_wArea; // Это лишнее...
}

void WAbstractScrollArea::buildSelfUi()
{
    setVerticalScrollBarPolicy  (Qt::ScrollBarAsNeeded );
    setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff);

    m_wArea = new QWidget(this);  // ...потому что m_wArea - наследник QObject
    m_wArea->setLayout(m_lArea);
    setViewport(m_wArea);
}

Если не нужно наследоваться от QObject, есть ещё QScopedPointer, QSharedPointer.

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

S>Кроме того в программе присутствуют бесконечные delete, даже для диалогов. В Qt же есть отличный механизм удаления объектов, унаследованных от QObject.
Привык самостоятельно следить за удалением созданного. Отвыкать нет никакого желания, да и опасно, есть шанс в один прекрасный момент выстрелить себе в ногу.

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

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

S>>Уважаемые, если у кого есть время, желание итд, пожалуйста просмотрите проект домашней бухгалтерии https://github.com/Sheridan/HAcc
S>>В логику можно не вникать, мне хочется знать что там может быть не так, неправильно, или некрасиво.

VVP>Про логику
VVP>1. Зачем синглетонов напихал?
Надо было в одном все городить?

VVP>2. Здесь <HAcc / trunk / src / model> чего такое???
VVP>

file ccontractors.cpp April 21, 2011 Создание и редактирование валюты [Sheridan]

Как получается, что контрагенты создают и редактируют валюты?
В коммит попало какоето изменение в указанном файле

VVP>3. Здесь <HAcc / trunk / src / model / ccurrencyes.h>
VVP>

//! Синглон валют
VVP>/*!
VVP>Управляет контрагентвми. Непосредственно управляет таблицами
VVP>- currencyes
VVP>@see CCurrency
VVP>*/
VVP>class CCurrencyes :

Кто чем управляет?
Неудачный копипаст

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

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

S>Привык самостоятельно следить за удалением созданного. Отвыкать нет никакого желания, да и опасно, есть шанс в один прекрасный момент выстрелить себе в ногу.

Как раз самостоятельное удаление — это и есть стрельба себе по ногам. Потому что утечки будут всякий раз при возникновении необработанного исключения.

За освобождением ресурсов должны следить RAII-обёртки.

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

S>>Уважаемые, если у кого есть время, желание итд, пожалуйста просмотрите проект домашней бухгалтерии https://github.com/Sheridan/HAcc
S>>В логику можно не вникать, мне хочется знать что там может быть не так, неправильно, или некрасиво.
S>>Спасибо заранее

Зачем столько вспомогательных GUI классов? Чем стандартные не угодили? Писал бы на qml я бы еще понял, там с этим не всегда хорошо.

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

J>Плюс непонятно, что будет, если generateAction() вернет NULL, если такое может быть, конечно.
Это обрабатывается несколько позже, в итоге возвращается new QAction("Somewhere bug", 0);

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

VVP>

file ccontractors.cpp April 21, 2011 Создание и редактирование валюты [Sheridan]

Как получается, что контрагенты создают и редактируют валюты?
VVP>3. Здесь <HAcc / trunk / src / model / ccurrencyes.h>
Fixed.

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

S>Если уж используете синглтон, то закрывайте конструктор копирования и соответствующий оператор:

S>

S>private:
S>  MySingletone(const MySingletone&);
S>  MySingletone& operator=(const MySingletone&);
S>

Fixed.

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

J>checkSpetialPurposeTag -> checkSpecialPurposeTag (и прочие Spetial)
J>CSingleTone -> CSingletone
J>CTransactionsPools -> CTransactionPools, transactions_pool -> transaction_pool
Fixed.

J>выражение m_actions[atype][id] повторяется дважды-трижды (в зависимости от ифа), а ведь это каждый раз двойной поиск по мапам, т.е. операция совсем небыстрая в общем случае.
По другому практически никак. Смысл в том, что есть действие не существует, оно создается и возвращается. Если существует, то просто возвращается. Типа синглтонов.

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

I>Зачем столько вспомогательных GUI классов? Чем стандартные не угодили? Писал бы на qml я бы еще понял, там с этим не всегда хорошо.
Всмысле? Я беру стандартные и компоную из них нужные мне. Например кнопка с выпадающим списком-табличкой и индикацией выбранного на кнопке.

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

I>1) Не понравилась конвертация string->money,amount. Надо использовать QLocale::decimalPoint,
Смысл запятую в точку преобразовать и поудалять все кроме чисел и точки. Зачем лишний вызов?

I>ну и плюс хранение денег просто в double без отдельного класса, это та еще головная боль при поиске копеек.
мысль...

I>2) Много макросов.
Ничего не поделаешь, мне очень удобно с макросами.

I>3) Не внятное владение указателями на объект в CFileDialogs
Согласен, невнятное. Но работает правильно

I>Мне все равно как форматируются исходники и по какому принципу называются классы, но такое встречаю первый раз

, ненадолго впал в ступор
I>

I>                CDatabase::~CDatabase() { close();}
I>const QString & CDatabase::dbFileName() { return m_fileName; }
I>           bool CDatabase::isOpen    () { return m_db.isOpen(); }
I>

Смысл — совпадающие части выровнять. Изза этого сразу видно различие между строками.

I>5) При первом запуске программа не смогла поместится на экран,
о0 15"?

I>создавая доки используй tabifyDockWidget.
А, понял. Возможно. Может mdi по умолчанию?

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

J>>выражение m_actions[atype][id] повторяется дважды-трижды (в зависимости от ифа), а ведь это каждый раз двойной поиск по мапам, т.е. операция совсем небыстрая в общем случае.
S>По другому практически никак. Смысл в том, что есть действие не существует, оно создается и возвращается. Если существует, то просто возвращается. Типа синглтонов.

не, это ты не понял.
тебя операторы с толку сбивают, похоже.
давай я перепишу на вызовах функций:

QAction * CBases::constructAction(EActionsTypes atype, const hacc::TDBID &id, QObject *reciever, const char * method)
{
    if(m_actions.run_long_search(atype).run_long_search(id) == NULL)
    {
        QAction *gAction = generateAction(atype, reciever, method);
        gAction->setEnabled(HACC_DB->isOpen());
        connect(HACC_DB, SIGNAL(stateChange(bool)), gAction, SLOT(setEnabled(bool)));
        m_actions.run_long_search(atype).run_long_search(id) = gAction;

    }
    return m_actions.run_long_search(atype).run_long_search(id);
}

теперь проблема видна?

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

J>теперь проблема видна?

Я это понимал и раньше. Терперь давай тебе объясню:

QAction * CBases::constructAction(EActionsTypes atype, const hacc::TDBID &id, QObject *reciever, const char * method)
{
    if(В массиве есть элемент?)
    {
        QAction *gAction = generateAction(atype, reciever, method);
        gAction->setEnabled(HACC_DB->isOpen());
        connect(HACC_DB, SIGNAL(stateChange(bool)), gAction, SLOT(setEnabled(bool)));
        Нету? ок, создаем и добавляем. = gAction;

    }
    return Теперь есть в любом случае, возвращаем;
}

Имхо любая попытка оптимизации тут приведет лишь к лишним вызовам.
Через отдельную переменную? Ее ндо инициализировать элементом массива, а затем обратно положить в массив если NULL был.
Сделать проверку не на NULL а на contains(id) — в итоге три проверки вместо одной:

!(
m_actions.contains(atype) &&
m_actions[atype].contains(id) &&
m_actions[atype][id] == NULL
)

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

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

C>Как раз самостоятельное удаление — это и есть стрельба себе по ногам. Потому что утечки будут всякий раз при возникновении необработанного исключения.
Необработанное исключение должно подразумевать окошко об ошибке и аварийное завершение программы. Этого у меня тоже, конечно, нет. Но с этой точки зрения утечки — фигня.

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

Если я правильно понимаю, в финальном мапе у тебя лежат указатели, и вызов m_actions[atype][id] имеет тип QAction*& (ссылка на указатель).
Поэтому надо ее просто закэшировать и дальше пользоваться этой ссылкой, и тогда у тебя будет всего один поиск по мапам:

QAction * CBases::constructAction(EActionsTypes atype, const hacc::TDBID &id, QObject *reciever, const char * method)
{
    QAction*& ret = m_actions[atype][id]; // поиск
    if(ret == NULL)
    {
        QAction *gAction = generateAction(atype, reciever, method);
        ...
        ret = gAction; // поиска нет

    }
    return ret; // поиска нет
}

да, чтоб два раза не вставать:
reciever -> receiver

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

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

I>>1) Не понравилась конвертация string->money,amount. Надо использовать QLocale::decimalPoint,
S>Смысл запятую в точку преобразовать и поудалять все кроме чисел и точки. Зачем лишний вызов?
Замени разделитель в своей локали на другой, и посмотри как будет работать программа, собственно вариантов 2:
а) Используешь свою локаль устанавливая ее перед чтением записью данных
b) Используешь QLocale::decimalPoint

I>>5) При первом запуске программа не смогла поместится на экран,
S>о0 15"?
Кхм, 2 монитора, 1-ый 24'', второй 19'', при первом запуске программа по ширине расползлась на 2 монитора. При закрытии программы не было сохранения позиций QWidget.saveGeometry()

I>>создавая доки используй tabifyDockWidget.
S>А, понял. Возможно. Может mdi по умолчанию?
Сгруппируй элементы так чтобы пользователь запуская программу мог сразу работать, а не перетаскивать элементы под свое виденье.

	От:	Sheridan
	Дата:	27.01.12 05:41
	Оценка:

От:	VVP	67524421
Дата:	27.01.12 05:58
Оценка:	10 (1)

От:	jazzer	Skype: enerjazzer
Дата:	27.01.12 06:15
Оценка:	10 (1)

	От:	Igore
	Дата:	27.01.12 06:55
	Оценка:	10 (1)

	От:	sheep2k
	Дата:	27.01.12 08:21
	Оценка:	10 (1)