[Kde-finance-apps] Review of alkquotes

[Brian Cappello]

On Thu, Jul 1, 2010 at 8:35 AM, Alvaro Soliverez <asoliverez at gmail.com>wrote:

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

 I must have missed the link, were you referring to the perl based QTest at
qtest.qbilt.org or Qt's QTestLib? Anyway, I've been doing some reading on
test-driven development so I'll definitely work on adjusting my work flow to
design the class methods around the test requirements... might need to
rethink how some things are currently done, though I really need to get my
feet wet with unit testing before I can know for sure. I'm assuming I should
take a tangent from developing more features and functionality to make sure
what's there is properly testable first? Seems like it should be higher
priority, even though I'm beginning to slip from the schedule I laid out for
myself... thoughts?

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

Sorry, guess I wasn't too clear with what I meant. That "select ? from ..."
is Qt's way of saying it didn't understand my bind variable, I think. (I had
initialized mySelectColumnsQString to
symbol,name,date,time,open,high,low,close,volume but the last-executed query
only came back with the "?".) The way I have the db-storage/retrieval
methods set up now is to take a generic list of keys (which are also the
column names) as input. Currently there is only support for a few
keys/columns implemented, but, to add more simply requires modifying
backend.cpp's master column list and statsmanager.cpp's m_yahooKeys look-up
hash; the rest of the (current) code is meant to be key/column-generic. So,
at least the way things are currently structured, the column names are known
but the specific columns are flexible.

For a predefined query, such as looking up the last price quote which will
always require the same columns, I could add bind variables the same way
that Qt doc you linked suggests. But, I'm confused with how to do bind
variables once support for more keys has been added and the specific
keys/columns requested by clients becomes unpredictable. Which leads me to
your comment about just retrieving all the columns: since QSqlQuery doesn't
support accessing results by column name (at least not directly), in order
to return just the requested keys/columns, the "index" of the column in the
database would need to be looked up by doing something like query.value(
db_columns.indexOf( "keyName" ) ).toString(), for each requested key, where
db_columns is the list of all the column names used to initially create the
table. At least, that's the only way I know how to do it, but that's
obviously not saying much. I guess I'm just a little lost with what's the
best way to integrate bind variables into the way I've currently structured
my code or if I should try to group related keys as predefined queries and
let clients deal with selecting what they need or something else entirely...

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.
> >>
> _______________________________________________
> Kde-finance-apps mailing list
> Kde-finance-apps at kde.org
> https://mail.kde.org/mailman/listinfo/kde-finance-apps
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-finance-apps/attachments/20100701/a1137277/attachment.htm