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

[Thomas Baumgart]

Hi all,

on Thursday 24 June 2010 01:15:05 Fernando Vilas wrote:


> 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/AlkR
> >ec <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.

Agreed, this looks much nicer.

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

I have a few comments also:

* in alkdbus there's now an Alkrecord and AlkRecord subdirectory. In case of 
renaming, your should have simply used the "svn move" to rename the directory.  
This would have kept the history. Now you only can remove the 'ancient' 
directory.

* setters/getters so far OK, but you should not return a const object but 
rather a const reference to it. It's much faster:

   const QString getSomething(void) const 

creates a copy of the QString member object, whereas

   const QString& getSomething(void) const

simply returns a reference to the QString member object which you cannot alter 
(constness).

* line width: Using a line width way beyond 100 chars too many times is not a 
good idea. I often use vi in a 80 char console to read source code and it 
really hurts my eyes (well, I don't always have access to an IDE). This is 
esp. a problem for comments.

* regarding the DB connection you could try to take a look into the singleton 
pattern. That might be of help here.

* it might have been mention before.  "MarkAsRead" should be called 
"markAsRead". Same applies for other methods.

* this one is an optimization issue and heavily depends on the amount of data 
you move around:

   QList <AlkData> queryallTransactions()

is slower than

   queryallTransactions(QList <AlkData>& )

but of course it is used differently. So


   QList<AlkData> myList;
   myList = queryallTransactions();

becomes

   QList<AlkData> myList;
   queryallTransactions(myList);


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

Yes!

> 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.
> 

-- 

Regards

Thomas Baumgart

GPG-FP: E55E D592 F45F 116B 8429   4F99 9C59 DB40 B75D D3BA
-------------------------------------------------------------
Linux, because rebooting is for adding new hardware ...
-------------------------------------------------------------
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 225 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/kde-finance-apps/attachments/20100624/e8f70d25/attachment.sig