[Kde-finance-apps] 1st Revision -Alkimia dbus Service
fvilas at iname.com
Thu Jun 24 01:15:05 CEST 2010
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.
> <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.
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.
fvilas at iname.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
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
More information about the Kde-finance-apps