[Kde-finance-apps] Review of alkquotes

[Alvaro Soliverez]

Hello Brian,
see my comments below.

On Thu, Jul 1, 2010 at 12:36 AM, Brian Cappello <briancappello at gmail.com> wrote:
> Hi Alvaro,
> Thanks so much for taking the time to review and write out all of these
> insightful comments! Now the challenge of fixing them :) I noticed too that
> you were accepted to KDE e.V., congratulations!
>
> On Tue, Jun 29, 2010 at 10:46 PM, Alvaro Soliverez <asoliverez at gmail.com>
> wrote:
>>
>> Hello Brian,
>> here is a first review of the code:
>>
>> - For strings that will be displayed to the user, please use i18n, so
>> the strings can be translated
>> - Create a folder to drop all CMake modules there
>> - Add unit tests, at least for all public methods in the backend classes
>
>
> From reading Wikipedia I gather unit tests are a collection of calls to
> methods that verify if expected outcomes are met? How is it best to
> integrate these into my code? Should they be contained in their own method,
> or a separate class, or the UI frontend even? Is there any kind of
> best-practice with regards to unit tests?
>
Here is a link to QTest, the recommended unit testing framework for Qt.
Unit testing is more easily done for backend classes (ui classes can
be tested, it's just more cumbersome). With cmake, you can run cmake
test, and it will execute all tests and inform the result.

For every backend class <class>, you should have a class <class>test.
It should have a test method for each public method of <class>. Try to
test for those corner cases in the class that might be hard to find if
not looking exactly at it.

Also, look for "test-drive development", which relies on creating the
unit tests first, and the main code later.


>>
>> - When initializing a class, QString should be initialized on the same
>> call and not on the body.
>>
>> CompanyInfoPlugin::CompanyInfoPlugin()
>> {
>>  m_pluginName = "CompanyInfoPlugin";
>>
>> should be
>>
>> CompanyInfoPlugin::CompanyInfoPlugin():
>>  m_pluginName("CompanyInfoPlugin")
>> {
>>
>> That is more efficient, because it saves a QString instance.
>
> OK that's easy enough. Can the same pre-body initialization be done for
> things like QStringList/QHash too?

No, in that case you have to initialize that in the body. Create a
separate method for those initialization, so you leave the body code
as clean as possible.

>>
>> - Avoid using "select *" on queries. That's prone to errors in the
>> future and less efficient as tables start growing
>
> OK, tried to fix those.
>>
>> - You should use queries with bind values as opposed to create the
>> whole query string each time.
>
> I'm pretty confused by what can be a bound variable, and how to use them
> when the number of columns/values isn't known ahead of time. For example,
> populating a QString with some columns and then saying this doesn't work:
> query.prepare("select :columns from ... ")
> query.bindValue(:columns, mySelectColumnsQString);
> query.exec();
> qDebug() << query.lastQuery() == "select ? from ..."
>

Why do you not know which columns you are retrieving?
In some cases, if you are retrieving 3-4-5 columns depending on the
case, it's better to just retrieve all 5 columns, and use the data you
need that to keep building the query on-the-fly.

Unless you are retrieving big blobs, the performance degradation from
retrieving an extra column is minimal, and you gain on simplicity and
maintainability.


> I'll keep playing around with it though, there must be a way...
>>
>> query.exec( "SELECT * FROM symbolinfo WHERE symbol='" + symbolCAPS + '\''
>> );
>>
>> should be similar to this example in
>> http://doc.trolltech.com/4.6/qsqlquery.html#details
>>
>> QSqlQuery query;
>>     query.prepare("INSERT INTO person (id, forename, surname) "
>>                   "VALUES (:id, :forename, :surname)");
>>     query.bindValue(0, 1001);
>>     query.bindValue(1, "Bart");
>>     query.bindValue(2, "Simpson");
>>     query.exec();
>>
>> That's much more efficient, and much more secure if the queries are
>> open to user intervention.
>>
>> - Get rid of QDebug statements on the commited code. That suggests
>> that unit tests are extremely necessary. As a rule, don't commit code
>> with QDebug (unless it is commented out, of course)
>
> Good to know. And definitely does suggest I need unit tests - most of my
> "unit testing" is done manually, verifying results on the debug output...
> definitely time to change that work flow :)
>>
>> - I see that a number of classes have an interesting amount of
>> parameters which all seem to be related.
>> For example:
>> searchResults( const QString &searchTerm,
>>                        const QStringList &symbols,
>>                        const QStringList &names,
>>                        const QStringList &exchanges,
>>                        const QStringList &types );
>>
>> You should create a class to contain these values and just pass an
>> instance of it. The patter is called Data Transfer Object (DTO)
>
> OK. When you say instance, is it best to create a DTO using new or to
> pass-by-reference?

When possible, try to pass by reference, like you are doing here.

Just fill in the object with the same data you were passing, and pass
only one object by reference.
You could use a QList<QList>, for example, if you have trouble sending
custom classes across DBus.

 I've always been kind of confused about understanding
> when non-pointer objects get automatically deleted from memory. In this
> specific example, that signal ends up getting emitted across DBus, so, first
> I need to figure how to pass custom types through adaptors before I can fix
> that one :/
>>
>> - I've noticed you added some krazy excludes. Is there a reason for that?
>>  Krazy excludes should be added only as last-instance, when it is
>> certain there is a false positive.
>>
>> *  Copyright (c) 2010 Brian Cappello            //krazy:exclude=copyright
>>  *  briancappello [at] gmail [dot] com
>>
>> In this case, Krazy could not find the email, probably. Using [at] and
>> [dot] won't stop the email harvesters, so use the proper form and
>> relay on the gmail spam filter.
>
> OK, that was dumb, will remove those.
>>
>>
>> And, to emphasize, test, test, and test. :)
>>
>> I still have to review the design. I'll get to it during the next few
>> days.
>>