Code-review программы для работы с заметками

Здравствуйте. Я пишу небольшую программку для работы с заметками. Сейчас реализован минимальный функционал: заметку можно создать, отредактировать и удалить. Для 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 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 DbManager::showNotes() { if (connected()) { QVector 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(); } }
Отправка заметок на форму:
void NoteHolder::publishNotes(QWidget* ParentWidget) { erasePublishedNotes(ParentWidget);
getAllNotes();
QVector::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()) != 0) { delete MgbTemp; }
QGroupBox* GbTemp; while ((GbTemp = ParentWidget->findChild()) != 0) { delete GbTemp; } }
void NoteHolder::getAllNotes() { TotalNoteCount = 0;
if (!ListOfAllNotes.isEmpty()) ListOfAllNotes.clear();
QVector ListOfRows = DbManager::showNotes();
for (int i = 0; i < ListOfRows.length();i++) { MattyNote TempNote(ListOfRows[i]); ListOfAllNotes.append(TempNote); TotalNoteCount++; } }
Весь исходный код можно увидеть тут: GitHub
Разделение на классы, их методы, вызовы, зависимости и т.д. тут: Doxygen

Ответ

Определитесь с хранением подключаемых файлов
Все подключаемые файлы должны находится в заголовочных файлах *.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; } }