Code-review program for working with notes

Question

Hello. I am writing a small program for working with notes. The minimum functionality is now implemented: a note can be created, edited and deleted. For GUI I have Qt, the base is SQLite. In general, everything works and does what I want. But my programming experience tends to zero, so I suspect that there are many flaws that I cannot fix, because I don’t know where to look. I would like you to point out all the errors / problems / flaws that are evident. Interested in everything from the logic of class division to possible memory leaks.

Now there are essentially 12 classes:

4 windows: MattyNotesMainWindow , MattySettingsDialog , addNoteDialog , MattyMessageBox
class for working with DB DbManager
class for compiling SQL queries QueryConstructor
proper class note MattyNote
constructor class for visual display of the MattyGroupBox note
class that deals with sorting and displaying notes NoteHolder
class to manage css MattyStyleSheetEditor
class to store lines and characters Constants
watch MattyClocks

I understand that it is unlikely that anyone will read all the code, so maybe you can look at the classes and their methods and say that it’s too much, or look at any one class and point out the flaws in it.

For example, the functions of connecting to the database, editing an existing note, and extracting all notes from the database:

 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>(); } }

Sending notes to the form:

  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++; } }

All source code can be seen here: GitHub

Division into classes, their methods, challenges, dependencies, etc. here: Doxygen

(Since I almost don’t know the pros, I’m posting a review about the rest)
From me, plus for taking advantage of Doxygen and publishing the docks on GitHub Pages.
By git: it's good that you are versioning the code and that the two developers use different branches.
You can improve: 1) commit comments are often not informative, when working in a large team, they must disclose the essence of changes 2) it is better not to configure user.name and user.email for a particular project in .gitconfig , but through git config --local user.name=...
There it is noticeable that the githab does not recognize the authorship of commits from the working account.
You need to go to Settings → Emails and add a second email address.
@AlexKrass, you definitely do not want to endure all this in response?
Even if the reward does not reach you, then there is still a fashionable secret hat "this is fine": meta.stackexchange.com/questions/288271/…

Alex Krass Alex Krass 13.9k one 18 46 · Accepted Answer · 2017-01-03T15:59:42

Decide on storing the attached files.

All include files must be in the header files *.h , half of you are in *.cpp . Bring the application to a single form, make all the extra connections in the headers.

Universal paths

Now you have most of the paths and settings in the application, this is very bad, because every time you have to recompile and the program loses its versatility. It is much easier for the user to use more versatile means in the form of configuration files.

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(); }

Now you can clean up a lot of excess from DbManager , just read the documentation, from there a lot can be taken out. Now you do not need to transfer the lines, they will be read from the config.ini file themselves, or the path will be installed by default in the directory with the executable file. You also do not close the connection to the database, so I think you can use the simple DbManager::MattyNotesDb.isOpen() in 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; }

Now you can throw the application on a USB flash drive and it should work everywhere.

Principle of shared responsibility

Each class must be responsible for its goal, it should not have several functions. Do not forget to make the formation of entities inside them, without smearing them on the code of other classes.

 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(); }

Now the request for all notes will look better:

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

A little more about the database

In general, it's good to use one DbManager::MattyNotesDb for everything and static methods are not the best way out. The problems will begin when the application suddenly acquires multithreading or some other entity. Therefore, this practice is better not to use. Do not be afraid to write non-flexible methods in the model. model design is very different from application design. Dynamic formation is left in case of emergency.

This is due to the fact that it is quite difficult to recreate the query and execute it on the database in this form or catch errors. Also, several people often work on the project and explicit requests make communication much easier. Sometimes it is enough to look at the request and find the error or places where changes are required. Dynamic shaping deprives you of this and you have to shovel all the code and recreate requests by code. Therefore, they often make hard requests, but they are more efficient.

It is also worth creating a separate class for entities. Somehow it should look like in the end.

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; } }

Do MattyNoteProvider need to create classes like MattyNoteProvider for each table?
@Matty it is desirable to distinguish between entities, but not necessarily at all.
If you consider allocating a class for a table as redundant, then do not do it.
Perhaps, then you will want to bring the entities together, if there are many of them and you can create a class for storage and access through it.
Each entity in the database is assigned a corresponding class.

Answer 2 · 2017-01-02T15:53:12

Let me try to answer, because once we have already met Matty ...

 enum OrderType { Straight, Descending };

Sort ascending is called Ascending .

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

Destructors in inherited classes must be virtual, otherwise the order of destructors call will be violated. The same in MattyNotesMainWindow and many other classes.

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

Type is a name that is too conflict-prone.

You use Doxygen with a bunch of diagrams without any text notes, while such diagrams are rarely useful (IMHO). The Qt project itself is very well documented, see how everything works out there.

I will also join the opinion of colleagues that you should not independently generate queries. I noticed this even when we were looking for the missing tables. Such a solution is not only difficult to maintain, but it is also prone to SQL injections. The use of a previously prepared parametric query completely eliminates the injection. In the future, when you will work with network databases, it will be important to consider. In practice, very rarely there is a situation when the structure of the request must be dynamically changed, for other cases, prepare is more than enough. Do not forget that the database must be open when calling the prepare method, otherwise the behavior is not defined. I once stumbled on this rake.

I recommend that all of my classes inherit from QObject. Otherwise, you cannot use signals and slots in them. For example, the most likely candidate for adding a signal is the DbManager class, the signal may be a “database state change” (connection establishment / loss).

In the future, you need to think about how to install the program. As I understand it, you have it cross-platform. On Linux, the Qt executable library is a prerequisite, so by entering the relevant information in the package configuration file, you will resolve all issues related to the presence of libraries on the computer. In the case of Windows, you will need to manually collect all the dll-files and enter them into the installation package. These files may lie approximately along the following path: "c: \ Qt \ 5.6.1 \ 5.6 \ Src \ MinGW". It is necessary to determine exactly which files your program needs and when installing them place them next to the program. There was a time, I used qmake to select all the necessary files, put them into a separate directory and generate a script for Inno Setup. If you want to do something similar, use the INSTALLS variable to add all files to a common directory, as well as the write_file function to write a iss-script.

Here I knew that the truth is somewhere nearby) Thanks for Accending .
On account of the Type also logical, it will be necessary to replace.
QObject at me is present everywhere where there are connections.
Yes, I already use Inno Setup, there is even a script on a githaba somewhere.
For Qt, there is a very handy program that collects everything you need into one folder.
Honestly, I have problems understanding the logic of virtual and abstract things.
In my case it is necessary to assign virtual to the destructor?
At the expense of comments in the documentation I thought, but I did not figure out exactly how to put them there.
Regular comments that are close to, say, a method that Doxygen ignores.
Hmm .. since I’m all advised to abandon this method of making requests, then I will try to remove it.
More precisely, it is Ascending , but thanks anyway, because it all

Code-review program for working with notes

2 answers 2

More articles: