[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-core-devel
Subject: Re: New repo in kdereview: kalk
From: Ivan =?utf-8?B?xIx1a2nEhw==?= <ivan.cukic () kde ! org>
Date: 2021-02-21 13:25:30
Message-ID: 5665381.o3KbAiDoV7 () drako
[Download RAW message or body]
Hi all,
Cool idea for the project. I went through the C++ part of the code, and have a
few suggestions.
> > Please reconsider your decision.
> https://blog.acolyer.org/2020/10/02/toward-an-api-for-the-real-numbers/
+1 for the plea of not reinventing numerics however fun it is to play with
flex/yacc.
For the actual code review:
historymanager.cpp:
Item 1:
if (file.exists()) {
file.open(QIODevice::ReadOnly);
This should be:
if (file.open(QIODevice::ReadOnly);
The fact that a file exists does not mean open will succeed.
Item 2:
This:
for (const auto &record : qAsConst(array)) {
historyList.append(record.toString());
}
should be prefixed with:
historyList.reserve(array.count());
Item 3:
There is no need to call file.close() as its destructor will close it.
Item 4:
Instead of auto array and qAsConst(array), just declare array to be const:
const auto array = ...
or
const auto& array = ...
Item 5:
this->save() can be only save(). It is not common to write this-> in C++
unless there is a strong reason to do so in a particular case.
Item 6:
QList<QString> historyList; -> QList<QString> m_historyList;
inputmanager.cpp:
Item 1:
Great to see a correct C++ singleton :)
Item 2:
You can use std::deque instead of std::stack. You'll get .clear() operation.
It will be more efficient than = {};
unitmodel.cpp:
Item 1:
Similar to above, when you know the number of elements a collection will have,
call reserve(size) before you start appending (this is for m_units).
BTW, these for loops can be replaced with std::transform and
std::back_inserter, but I don't want to go overload the review. ;)
Cheers,
Ivan
--
dr Ivan Čukić
ivan@cukic.co, https://cukic.co/
gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232 07AE 01C6 CE2B FF04 1C12
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic