<br><br><div class="gmail_quote">On Thu, Jul 1, 2010 at 8:35 AM, Alvaro Soliverez <span dir="ltr"><<a href="mailto:asoliverez@gmail.com">asoliverez@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
Hello Brian,<br>
see my comments below.<br>
<div class="im"><br>
On Thu, Jul 1, 2010 at 12:36 AM, Brian Cappello <<a href="mailto:briancappello@gmail.com">briancappello@gmail.com</a>> wrote:<br>
> Hi Alvaro,<br>
> Thanks so much for taking the time to review and write out all of these<br>
> insightful comments! Now the challenge of fixing them :) I noticed too that<br>
> you were accepted to KDE e.V., congratulations!<br>
><br>
> On Tue, Jun 29, 2010 at 10:46 PM, Alvaro Soliverez <<a href="mailto:asoliverez@gmail.com">asoliverez@gmail.com</a>><br>
> wrote:<br>
>><br>
>> Hello Brian,<br>
>> here is a first review of the code:<br>
>><br>
>> - For strings that will be displayed to the user, please use i18n, so<br>
>> the strings can be translated<br>
>> - Create a folder to drop all CMake modules there<br>
>> - Add unit tests, at least for all public methods in the backend classes<br>
><br>
><br>
> From reading Wikipedia I gather unit tests are a collection of calls to<br>
> methods that verify if expected outcomes are met? How is it best to<br>
> integrate these into my code? Should they be contained in their own method,<br>
> or a separate class, or the UI frontend even? Is there any kind of<br>
> best-practice with regards to unit tests?<br>
><br>
</div>Here is a link to QTest, the recommended unit testing framework for Qt.<br>
Unit testing is more easily done for backend classes (ui classes can<br>
be tested, it's just more cumbersome). With cmake, you can run cmake<br>
test, and it will execute all tests and inform the result.<br>
<br>
For every backend class <class>, you should have a class <class>test.<br>
It should have a test method for each public method of <class>. Try to<br>
test for those corner cases in the class that might be hard to find if<br>
not looking exactly at it.<br>
<br>
Also, look for "test-drive development", which relies on creating the<br>
unit tests first, and the main code later.<br></blockquote><div><br> I must have missed the link, were you referring to the perl based QTest at <a href="http://qtest.qbilt.org">qtest.qbilt.org</a> 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?<br>
</div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im"><br>
<br>
>><br>
>> - When initializing a class, QString should be initialized on the same<br>
>> call and not on the body.<br>
>><br>
>> CompanyInfoPlugin::CompanyInfoPlugin()<br>
>> {<br>
>> m_pluginName = "CompanyInfoPlugin";<br>
>><br>
>> should be<br>
>><br>
>> CompanyInfoPlugin::CompanyInfoPlugin():<br>
>> m_pluginName("CompanyInfoPlugin")<br>
>> {<br>
>><br>
>> That is more efficient, because it saves a QString instance.<br>
><br>
> OK that's easy enough. Can the same pre-body initialization be done for<br>
> things like QStringList/QHash too?<br>
<br>
</div>No, in that case you have to initialize that in the body. Create a<br>
separate method for those initialization, so you leave the body code<br>
as clean as possible.<br>
<div class="im"><br>
>><br>
>> - Avoid using "select *" on queries. That's prone to errors in the<br>
>> future and less efficient as tables start growing<br>
><br>
> OK, tried to fix those.<br>
>><br>
>> - You should use queries with bind values as opposed to create the<br>
>> whole query string each time.<br>
><br>
> I'm pretty confused by what can be a bound variable, and how to use them<br>
> when the number of columns/values isn't known ahead of time. For example,<br>
> populating a QString with some columns and then saying this doesn't work:<br>
> query.prepare("select :columns from ... ")<br>
> query.bindValue(:columns, mySelectColumnsQString);<br>
> query.exec();<br>
> qDebug() << query.lastQuery() == "select ? from ..."<br>
><br>
<br>
</div>Why do you not know which columns you are retrieving?<br></blockquote><div><br>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. <br>
<br>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...<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
In some cases, if you are retrieving 3-4-5 columns depending on the<br>
case, it's better to just retrieve all 5 columns, and use the data you<br>
need that to keep building the query on-the-fly.<br>
<br>
Unless you are retrieving big blobs, the performance degradation from<br>
retrieving an extra column is minimal, and you gain on simplicity and<br>
maintainability.<br>
<div><div></div><div class="h5"><br>
<br>
> I'll keep playing around with it though, there must be a way...<br>
>><br>
>> query.exec( "SELECT * FROM symbolinfo WHERE symbol='" + symbolCAPS + '\''<br>
>> );<br>
>><br>
>> should be similar to this example in<br>
>> <a href="http://doc.trolltech.com/4.6/qsqlquery.html#details" target="_blank">http://doc.trolltech.com/4.6/qsqlquery.html#details</a><br>
>><br>
>> QSqlQuery query;<br>
>> query.prepare("INSERT INTO person (id, forename, surname) "<br>
>> "VALUES (:id, :forename, :surname)");<br>
>> query.bindValue(0, 1001);<br>
>> query.bindValue(1, "Bart");<br>
>> query.bindValue(2, "Simpson");<br>
>> query.exec();<br>
>><br>
>> That's much more efficient, and much more secure if the queries are<br>
>> open to user intervention.<br>
>><br>
>> - Get rid of QDebug statements on the commited code. That suggests<br>
>> that unit tests are extremely necessary. As a rule, don't commit code<br>
>> with QDebug (unless it is commented out, of course)<br>
><br>
> Good to know. And definitely does suggest I need unit tests - most of my<br>
> "unit testing" is done manually, verifying results on the debug output...<br>
> definitely time to change that work flow :)<br>
>><br>
>> - I see that a number of classes have an interesting amount of<br>
>> parameters which all seem to be related.<br>
>> For example:<br>
>> searchResults( const QString &searchTerm,<br>
>> const QStringList &symbols,<br>
>> const QStringList &names,<br>
>> const QStringList &exchanges,<br>
>> const QStringList &types );<br>
>><br>
>> You should create a class to contain these values and just pass an<br>
>> instance of it. The patter is called Data Transfer Object (DTO)<br>
><br>
> OK. When you say instance, is it best to create a DTO using new or to<br>
> pass-by-reference?<br>
<br>
</div></div>When possible, try to pass by reference, like you are doing here.<br>
<br>
Just fill in the object with the same data you were passing, and pass<br>
only one object by reference.<br>
You could use a QList<QList>, for example, if you have trouble sending<br>
custom classes across DBus.<br>
<div class="im"><br>
I've always been kind of confused about understanding<br>
> when non-pointer objects get automatically deleted from memory. In this<br>
> specific example, that signal ends up getting emitted across DBus, so, first<br>
> I need to figure how to pass custom types through adaptors before I can fix<br>
> that one :/<br>
>><br>
>> - I've noticed you added some krazy excludes. Is there a reason for that?<br>
>> Krazy excludes should be added only as last-instance, when it is<br>
>> certain there is a false positive.<br>
>><br>
>> * Copyright (c) 2010 Brian Cappello //krazy:exclude=copyright<br>
>> * briancappello [at] gmail [dot] com<br>
>><br>
>> In this case, Krazy could not find the email, probably. Using [at] and<br>
>> [dot] won't stop the email harvesters, so use the proper form and<br>
>> relay on the gmail spam filter.<br>
><br>
> OK, that was dumb, will remove those.<br>
>><br>
>><br>
>> And, to emphasize, test, test, and test. :)<br>
>><br>
>> I still have to review the design. I'll get to it during the next few<br>
>> days.<br>
>><br>
</div><div><div></div><div class="h5">_______________________________________________<br>
Kde-finance-apps mailing list<br>
<a href="mailto:Kde-finance-apps@kde.org">Kde-finance-apps@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/kde-finance-apps" target="_blank">https://mail.kde.org/mailman/listinfo/kde-finance-apps</a><br>
</div></div></blockquote></div><br>