13

Здравствуйте. Я пишу небольшую программку для работы с заметками. Сейчас реализован минимальный функционал: заметку можно создать, отредактировать и удалить. Для GUI у меня Qt, база - SQLite. В общем, всё работает и делает то, что я хочу. Но опыт программирования у меня стремится к нулю, поэтому подозреваю, что есть много недочетов, которые я не могу исправить, потому что не знаю, куда смотреть. Хотелось бы, чтобы вы указали на все ошибки/проблемы/недостатки, которые бросаются в глаза. Интересует всё от логики разделения на классы до возможных утечек памяти.

Сейчас есть по сути 12 классов:

  • 4 окна: MattyNotesMainWindow, MattySettingsDialog, addNoteDialog, MattyMessageBox
  • класс для работы с БД DbManager
  • класс для составления SQL-запросов QueryConstructor
  • собственно класс-заметка MattyNote
  • класс-конструктор для визуального отображения заметки MattyGroupBox
  • класс, который занимается сортировкой и показом заметок NoteHolder
  • класс для управления css MattyStyleSheetEditor
  • класс для хранения строчек и символов Constants
  • часики MattyClocks

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

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

bool DbManager::connect(const QString & path)
{
    MattyNotesDb = QSqlDatabase::addDatabase("QSQLITE");
    MattyNotesDb.setDatabaseName(path);

    if(QFile::exists(path))
    {
        if (!MattyNotesDb.open())
        {
            showIsNotOpenedError();
            MattyNotesDb.close();

            return false;
        }
        else
        {
            PathToDb = MattyNotesDb.databaseName();
        }

        return true;
    }
    else
    {
        return false;
    }
}

bool DbManager::editNote(MattyNote & Note, int NoteId)
{
    if (connected())
    {
        QueryConstructor Edit;
        Edit.setTableName(QStringLiteral("Notes"));

        Edit.addWhereFieldValue(QStringLiteral("NoteId"), QString::number(NoteId));

        QMap<QString, QString> NoteTemp;
        NoteTemp["NoteTitle"] = "\'" + Note.getTitle() + "\'";
        NoteTemp["NoteType"] = "\'" + Note.getType() + "\'";
        NoteTemp["NoteText"] = "\'" + Note.getText() + "\'";
        NoteTemp["EventTime"] = "\'" + Note.getEventTime() + "\'";
        NoteTemp["EventDate"] = "\'" + Note.getEventDate() + "\'";
        NoteTemp["CrTime"] = "\'" + Note.getCrTime() + "\'";
        NoteTemp["CrDate"] = "\'" + Note.getCrDate() + "\'";
        NoteTemp["TypeId"] = QString::number(Note.getTypeId());

        Edit.setWhatToSetFieldValue(NoteTemp); //

        QSqlQuery editNoteQuery;
        return editNoteQuery.exec(Edit.constructUpdateQuery());
    }
    else
    {
        return false;
    }
}

QVector<MattyNoteRow> DbManager::showNotes()
{
    if (connected())
    {
        QVector<MattyNoteRow> VectorOfNoteRows;

        QueryConstructor SelectAllNotes;
        SelectAllNotes.setTableName(QStringLiteral("Notes"));
        SelectAllNotes.setOrderByClause("NoteId", Descending);

        QSqlQuery getAllNotesQuery(MattyNotesDb);

        if( getAllNotesQuery.exec(SelectAllNotes.constructSelectQuery()))
        {
            while (getAllNotesQuery.next())
            {
                MattyNoteRow Row;

                Row.NoteId=getAllNotesQuery.value("NoteId").toInt();
                Row.NoteTitle=getAllNotesQuery.value("NoteTitle").toString();
                Row.NoteType=getAllNotesQuery.value("NoteType").toString();
                Row.NoteText=getAllNotesQuery.value("NoteText").toString();
                Row.EventTime=getAllNotesQuery.value("EventTime").toString();
                Row.EventDate=getAllNotesQuery.value("EventDate").toString();
                Row.CrTime=getAllNotesQuery.value("CrTime").toString();
                Row.CrDate=getAllNotesQuery.value("CrDate").toString();
                Row.TypeId=getAllNotesQuery.value("TypeId").toInt();

                VectorOfNoteRows.push_back(Row);
            }
        }
        else
        {
            QMessageBox::critical(NULL, QObject::tr("Error"), getAllNotesQuery.lastError().text());
        }

        return VectorOfNoteRows;
    }
    else
    {
        return QVector<MattyNoteRow>();
    }
}

Отправка заметок на форму:

    void NoteHolder::publishNotes(QWidget* ParentWidget)
    {
        erasePublishedNotes(ParentWidget);

        getAllNotes();

        QVector<class MattyNote>::iterator NoteNumber;
        int i;
        for (NoteNumber = ListOfAllNotes.begin(), i=0; NoteNumber < ListOfAllNotes.end();NoteNumber++, i++)
        {

            MattyGroupBox* MyGroupBox = new MattyGroupBox(*NoteNumber, ParentWidget);

            ParentWidget->layout()->addWidget(MyGroupBox);
        }
    }

void NoteHolder::erasePublishedNotes(QWidget* ParentWidget)
{
    MattyGroupBox* MgbTemp;
    while ((MgbTemp = ParentWidget->findChild<MattyGroupBox*>()) != 0)
    {
        delete MgbTemp;
    }

    QGroupBox* GbTemp;
    while ((GbTemp = ParentWidget->findChild<QGroupBox*>()) != 0)
    {
        delete GbTemp;
    }
}

void NoteHolder::getAllNotes()
{
    TotalNoteCount = 0;

    if (!ListOfAllNotes.isEmpty())
        ListOfAllNotes.clear();

    QVector<struct MattyNoteRow> ListOfRows = DbManager::showNotes();

    for (int i = 0; i < ListOfRows.length();i++)
    {
        MattyNote TempNote(ListOfRows[i]);
        ListOfAllNotes.append(TempNote);
        TotalNoteCount++;
    }
}

Весь исходный код можно увидеть тут: GitHub

Разделение на классы, их методы, вызовы, зависимости и т.д. тут: Doxygen

16
  • 1
    (Поскольку плюсы почти не знаю, накидаю ревью про остальное). Документация — очень важная часть программы. От меня плюс за то, что воспользовались Doxygen и опубликовали доки на GitHub Pages. Однако сам код почти без комментариев.
    – Nick Volynkin
    29 дек 2016 в 5:57
  • 1
    По git: хорошо что вы версионируете код и что два разработчика используют разные ветки. Можно улучшить: 1) комментарии коммитов часто неинформативные, при работе в большой команде они обязательно должны раскрывать суть изменений 2) настраивать user.name и user.email для конкретного проекта лучше не в .gitconfig, а через git config --local user.name=....
    – Nick Volynkin
    29 дек 2016 в 6:29
  • 1
    Ага, сам уже начал догадываться. Там заметно, что гитхаб не распознаёт авторство коммитов с рабочей учётки. Нужно зайти в Settings → Emails и добавить второй адрес почты.
    – Nick Volynkin
    29 дек 2016 в 6:36
  • 1
    @AlexKrass все это можно в ответе писать) 29 дек 2016 в 21:02
  • 1
    @AlexKrass, Вы точно не хотите вынести это все в ответ? Даже если вам награда не доедет, то есть еще модная секретная шляпа "this is fine": meta.stackexchange.com/questions/288271/… 30 дек 2016 в 16:57

2 ответа 2

2
+250

Определитесь с хранением подключаемых файлов

Все подключаемые файлы должны находится в заголовочных файлах *.h, у Вас же половина находится в *.cpp. Приведите приложение к единому виду, вынесите все лишние подключения в заголовки.

Универсальные пути

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

Config.h

class Config
{
    public:
        static QString GetValue(QString);
    private:
        Config();
};

Config.cpp

Config::Config() { }

QString Config::GetValue(QString key)
{
    QString configPath = QCoreApplication::applicationDirPath() + "/config.ini";
    QSettings settings(configPath,  QSettings::IniFormat);

    return settings.value(key).toString();
}

Теперь можно вычистить кучу лишнего из DbManager, так же читайте документацию, оттуда многое можно вынести. Теперь не нужно передавать строки, они будут сами читаться из файла config.ini или же путь будет устанавливаться по умолчанию в директорию с исполняемым файлом. Так же вы не закрываете соединение с БД, поэтому я думаю можно воспользоваться простым DbManager::MattyNotesDb.isOpen() в DbManager::connected().

QString DbManager::StoredPathDB = "";
QSqlDatabase DbManager::MattyNotesDb;

QString DbManager::GetPathDB()
{
    if(DbManager::StoredPathDB.isNull() || DbManager::StoredPathDB.isEmpty())
    {
        QString cPath = Config::GetValue("PathToDb");
        if(cPath.isNull() || cPath.isEmpty())
        {
            cPath = QCoreApplication::applicationDirPath() + "/MattyNotes.sqlite";
        }

        DbManager::StoredPathDB = cPath;
    }

    return StoredPathDB;
}

bool DbManager::connected()
{
    return DbManager::MattyNotesDb.isOpen();
}

bool DbManager::connect()
{
    MattyNotesDb = QSqlDatabase::addDatabase("QSQLITE");
    MattyNotesDb.setDatabaseName(DbManager::GetPathDB());

    if (!MattyNotesDb.open())
    {
        showIsNotOpenedError();
        MattyNotesDb.close();

        return false;
    }

    return true;
}

Теперь можно приложение скинуть на флешку и оно должно работать везде.

Принцип единой ответственности

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

MattyNoteRow(QSqlQuery query)
{
        NoteId=query.value("NoteId").toInt();
        NoteTitle=query.value("NoteTitle").toString();
        NoteType=query.value("NoteType").toString();
        NoteText=query.value("NoteText").toString();
        EventTime=query.value("EventTime").toString();
        EventDate=query.value("EventDate").toString();
        CrTime=query.value("CrTime").toString();
        CrDate=query.value("CrDate").toString();
        TypeId=query.value("TypeId").toInt();
}

Теперь запрос всех заметок будет выглядеть лучше:

while (getNotesQuery.next())
{
    VectorOfNotes.push_back(MattyNoteRow(getNotesQuery));
}

Еще немного о БД

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

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

Так же стоит для сущностей создать отдельный класс. Как-то так это должно выглядеть в конечном итоге.

DbManager.cpp

QSqlDatabase DbManager::GetInstance()
{
    QSqlDatabase db = QSqlDatabase::addDatabase("QSQLITE");
    db.setDatabaseName(DbManager::GetPathDB());

    return db;
}

MattyNoteProvider.cpp

MattyNoteRow MattyNoteProvider::ShowNote(int id)
{
    QSqlDatabase db = DbManager::GetInstance();

    if (db.open())
    {
        QString sql = "SELECT * FROM Notes WHERE NoteID = :noteID ORDER BY ASC";

        QSqlQuery query;
        query.prepare(sql);
        query.bindValue(":noteID", id);
        query.exec();

        ...

        db.close();
        return row;
    }
}
3
  • А подобные MattyNoteProvider классы стоит создавать для каждой таблицы?
    – Matty
    3 янв 2017 в 22:02
  • @Matty желательно разграничивать сущности, но вообще не обязательно. Если вы считаете выделять класс для таблицы излишним, то не делайте этого. Возможно, потом Вам захочется собрать сущности вместе, если их будет много и можно создать класс для хранения и обращаться через него. Еще можно в проекте можно под это дело выделить отдельную папку.
    – Alex Krass
    3 янв 2017 в 23:12
  • Такая концепция называется ORM (Object relationship mapping). Каждой сущности в базе данных сопоставляют соответствующий класс.
    – maestro
    4 янв 2017 в 16:39
2

Давайте я попробую ответить, ведь однажды мы уже познакомились с Matty...

enum OrderType { Straight, Descending };

Сортировка по возрастанию называется Ascending.

public:
    MattySettingsDialog(QWidget * parent = 0);
    ~MattySettingsDialog();

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

enum Type { MessageBoxInformation = 0, MessageBoxWarning = 1, MessageBoxQuestion = 2 };

Type - слишком склонное к конфликтам имя.

У вас используется Doxygen с кучей диаграмм без каких-либо текстовых примечаний, в то время как такие диаграммы редко бывают полезны (имхо). Сам проект Qt очень хорошо документирован, посмотрите как там всё устроено.

Присоединюсь также к мнению коллег о том, что не стоит самостоятельно генерировать запросы. Я заметил это ещё когда мы с вами искали пропавшие таблицы. Такое решение не только сложно сопровождать, но оно также склонно к SQL инъекциям. Использование предварительно подготовленного параметрического запроса полностью исключает инъекцию. В дальнейшем, когда вы будете работать с сетевыми базами данных, это будет важно учитывать. На практике очень редко бывает ситуация, когда структура запроса должна динамически меняться, для остальных случаев prepare - более чем достаточно. Не забывайте, что база данных должна быть открыта при вызове метода prepare, иначе поведение не определено. Я однажды споткнулся об эти грабли.

Рекомендую все свои классы наследовать от QObject. В противном случае вы не сможете использовать в них сигналы и слоты. Например, наиболее вероятный кандидат на добавление сигнала - класс DbManager, сигналом может быть "изменение состояния базы данных" (установление / потеря соединения).

В дальнейшем нужно подумать о способе установки программы. Как я понял, она у вас кроссплатформенная. На Linux исполняемые библиотеки Qt - пререквизит, поэтому внеся соответствующую информацию в конфигурационный файл пакета, вы решите все вопросы, связанные с наличием библиотек на компьютере. В случае с Windows вам потребуется вручную собирать все dll-файлы и заносить их в установочный пакет. Эти файлы могут лежать примерно по такому пути: "c:\Qt\5.6.1\5.6\Src\MinGW". Нужно определить, какие именно файлы нужны вашей программе и при установке поместить их рядом с программой. Было время, я с помощью qmake подбирал все необходимые файлы, складывал их в отдельный каталог и генерировал скрипт для Inno Setup. Если желаете сделать что-то похожее, воспользуйтесь переменной INSTALLS для складывания всех файлов в общий каталог, а также функцией write_file для записи iss-скрипта.

6
  • Вот знала же, что истина где-то рядом) Спасибо за Accending. На счет Type тоже логично, надо будет заменить. QObject у меня присутствует везде, где есть коннекты. Или вы рекомендуете просто везде его добавить? Да, я уже использую именно Inno Setup, там на гитхабе где-то даже скриптик лежит. Проверяла, на других компах устанавливается и работает. Правда, только виндовскую версию проверяла. Для Qt есть очень удобная программка, собирающая все необходимое в одну папку.
    – Matty
    2 янв 2017 в 17:52
  • Честно говоря, у меня проблемы с пониманием логики виртуальных и абстрактных вещей. В моем случае надо к деструктору приписать virtual? На счет комментариев в документации думала, но не разобралась, как именно их туда вносить. Обычные комментарии, которые находятся рядом с, допустим, методом, Doxygen игнорирует. Хм.. раз мне все советуют отказаться от такого способа составления запросов, то попробую его убрать.
    – Matty
    2 янв 2017 в 17:56
  • Точнее оно Ascending, но все равно спасибо, потому что оно у меня вообще из головы вылетело)
    – Matty
    2 янв 2017 в 18:00
  • Да, нужно приписать virtual. Комментарии - не обычные, а специальным образом отформатированные. Сейчас поправлю Ascending.
    – maestro
    2 янв 2017 в 18:10
  • А каким именно образом должны быть написаны комментарии?
    – Matty
    2 янв 2017 в 19:32

Ваш ответ

Нажимая на кнопку «Отправить ответ», вы соглашаетесь с нашими пользовательским соглашением, политикой конфиденциальности и политикой о куки

Всё ещё ищете ответ? Посмотрите другие вопросы с метками или задайте свой вопрос.