[Kde-finance-apps] 1st Revision -Alkimia dbus Service

[Fernando Vilas]

On Wednesday, June 23, 2010 12:58:34 Mukesh Gupta wrote:
> Hello Thomas,Alvaro,Fernando and all,
> I have made the changes to my first commit as per the reviews.
> https://svn.kde.org/home/kde/trunk/playground/office/alkimia/alkdbus/AlkRec
> <goog_1583234049> ord
> 
> The changes made are as follows:
> 
> 
> 1)The database connection is now established in the constructor itself  and
> disconnected in destructor .Before it was created at each function call.
> 2)Binding variables are used in place of normal sql queries. SqlTableModel
> is also replaced with the QSqlQuery with binding variables.
> 3)All function calls are made using call by reference.Earlier it was call
> by value.Const correctness also implemented.
> 4)Enumerated data types are used for status and transaction variables.
> 5)Getter and Setter functions are used for private members.
> 6)Database connection object db is made static .
> 7)Code indented using astyle and kmymoney guidelines
> 8)Documentation prepared using Doxygen.
> 9)Licence provided in all the files.
> 10)Database name made dynamic for the class methods.
> 
> Thomas,Alvaro, Fernando please check if  i missed out something ?
> Please point out anything you feel is worth changing.
> 
> Alvaro , sorry i made so many changes all at once.I got your mail after i
> had already made these changes. Will keep that in mind next time.
> 
> Regards,
> Mukesh

Mukesh,

	First off, this code is vastly improved from the previous versions. The 
documentation and consistent indentation make it far more readable. Data 
encapsulation, as given by the private member variables and accessor/mutator 
(getter/setter) functions, has been implemented. I like the switch to bind 
variables. I was hoping to get more utility from the QSqlTableModel, but that 
is ultimately your decision as a developer, and I do not have enough 
experience with that object to offer better input. All this was good work, I 
know you had a lot to work on from all the emails that came in.

	Now for more feedback, though less than last time :-)

Cristian already mentioned the variable naming conventions regarding 
capitalization. I will add that include guards are generally all caps.

Include directives are generally put inside the include guards.

If the function is expanded in the header file, it normally needs an "inline" 
keyword (this may be different for Qt and KDE). That said, unless you are 
trying to develop a header-only library, normally (not always) the interface 
and implementation are in different files.

Good attempt on the database connection, but there is still more to go. Class 
level static variables are not as good as a static function with a static 
variable within it. Scott Meyers explains this in "Effective C++" or possibly 
the sequel "More Effective C++". These are 2 of the best books I have ever read 
on C++ programming.

The object still connects to the database on each instantiation, which is 
better than each function, but I still think there is room for improvement. 
For instance, disconnecting in the dtor may not behave as expected if the 
objects are put into a container.

The connect() function now allows for a name to be passed, but the calls to 
connect() in the ctors still have the name hard coded.

As Alvaro mentioned, even though you did not have time to get to it in this 
round, replacing the printf() calls with a test driver will really help you.

This is a little different request, but do you have a high-level design for the 
system? That is, what objects use/call which others, and why. How does the 
system of objects as a whole look? A text description of a few 
sentences/paragraphs would really help, but if UML is your thing, a diagram is 
nice as well. Whatever works for you is fine, but it will really help us steer 
your work if we know where you are headed within your piece of the work.

That is all that I saw for this round...  once again, nice job implementing 
the changes so quickly.

-- 
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/20100623/8bd10b74/attachment.sig