[Kde-finance-apps] Alkimia Database Class -First Commit

[Fernando Vilas]

On Monday, June 21, 2010 13:45:09 Thomas Baumgart wrote:
> Hi all,
> 
> on Monday 21 June 2010 18:53:07 Mukesh Gupta wrote:
> > Hello Friends,
> > 
> > My first commit to  for alkimia is here
> > https://svn.kde.org/home/kde/trunk/playground/office/alkimia/alkdbus/Alkr
> > ec ord/
> > 
> > It consists of alkrecord class which will handle query and register
> > functions for the dbus server.Will add more functionality soon.The file
> > 
> > :ALK_DBASE: file is the sqlie database which is initially populated with
> > 
[snip lots of useful feedback]
> 
> 
> So much for now.  I know, Rome wasn't built in one day, but I want to give
> as much feedback as possible.

I'll chime in on this one as well.

The database name should not be fixed programatically. To get things working, 
it is fine, but in the longer term, consider passing the name.

Thomas mentioned passing things by const-ref. To go a step further, look into 
const correctness http://www.parashift.com/c++-faq-lite/const-correctness.html 
It will save you headaches in the long run.

Thomas also mentioned naming consistence. In KDE, class names are generally 
mixed case, so along with s/ALKRECORD/AlkRecord/, there is the matter of 
s/record/Record/. However "Record" is a very generic name, so it may need 
revision.

record::show() seems like it will be used differently later, but consider that 
QString::operator+() is slower than QString::arg(), so for your complex 
string, the latter may help.

record::amount is an int, but I see the placeholder for an AlkValue. I like 
this. It looks like you are making an effort to get it working, and refine it 
later, and you even have the path forward already documented. Good work.

Your main.cpp looks like a good starting point to get things working. You may 
also want to port it to QTest. Reading 5 transactions when printed is one 
thing, but even 20 gets unwieldy. As a test driver, it will only complain when 
something is wrong, and you can debug from there.

Use the initializer lists in your ctors instead of assignments in the body. 
Otherwise, the variables get default-constructed, then assign-copied. It saves 
steps.

There is code duplication between the AlkRecord dtor and 
AlkRecord::disconnect. One should call the other. This is far easier to fix 
early than later.

Use bind variables in your SQL statements. It will improve readability and a 
few other things also.

In C++ for loops, it is generally preferred to use ++i to i++.

Since transactiontype seems to be an enumerated type, use an enum. That is an 
area where C++ is somewhat less typesafe than the rest of the language, but at 
least the compiler has a prayer of getting it right.

Connecting and disconnecting on every transaction is *slow*. Consider another 
way.

Consider more documentation. Most developers really dislike doing it, until 
they inherit some strange code, wonder who wrote it, and see their own name on 
it. Even then, they still do not like it, but at least they do not complain as 
much about having to do it.

Finally, as Thomas said, we are pointing out as many things as we see to 
improve the code. We all learned how to code from someone, sometime, telling 
us the same thing(s) you are getting now. Overall, the best thing you could 
have done is commit the code and ask for comments. Nice work.

-- 
Thanks,
Fernando Vilas
fvilas at iname.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/kde-finance-apps/attachments/20100621/200cd820/attachment.sig