No subject
Thu Jul 1 01:41:10 CEST 2010
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?
> - 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?
>
> - 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 ..."
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? 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.
>
> Regards,
> Alvaro
> _______________________________________________
> Kde-finance-apps mailing list
> Kde-finance-apps at kde.org
> https://mail.kde.org/mailman/listinfo/kde-finance-apps
>
--000feaee1006ecc822048a4b2d85
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Hi Alvaro,<br>Thanks so much for taking the time to review and write out al=
l of these insightful comments! Now the challenge of fixing them :) I notic=
ed too that you were accepted to KDE e.V., congratulations!<br><br><div cla=
ss=3D"gmail_quote">
On Tue, Jun 29, 2010 at 10:46 PM, Alvaro Soliverez <span dir=3D"ltr"><<a=
href=3D"mailto:asoliverez at gmail.com" target=3D"_blank">asoliverez at gmail.co=
m</a>></span> wrote:<br><blockquote class=3D"gmail_quote" style=3D"margi=
n: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-le=
ft: 1ex;">
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=
></blockquote><div>=A0</div><div>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 contain=
ed in their own method, or a separate class, or the UI frontend even? Is th=
ere any kind of best-practice with regards to unit tests?<br>
=A0</div><blockquote class=3D"gmail_quote" style=3D"margin: 0pt 0pt 0pt 0.8=
ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
- When initializing a class, QString should be initialized on the same<br>
call and not on the body.<br>
<br>
CompanyInfoPlugin::CompanyInfoPlugin()<br>
{<br>
=A0m_pluginName =3D "CompanyInfoPlugin";<br>
<br>
should be<br>
<br>
CompanyInfoPlugin::CompanyInfoPlugin():<br>
=A0m_pluginName("CompanyInfoPlugin")<br>
{<br>
<br>
That is more efficient, because it saves a QString instance.<br></blockquot=
e><div>OK that's easy enough. Can the same pre-body initialization be d=
one for things like QStringList/QHash too?<br></div><blockquote class=3D"gm=
ail_quote" style=3D"margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(2=
04, 204, 204); padding-left: 1ex;">
<br>
- Avoid using "select *" on queries. That's prone to errors i=
n the<br>
future and less efficient as tables start growing<br></blockquote><div>OK, =
tried to fix those. <br></div><blockquote class=3D"gmail_quote" style=3D"ma=
rgin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding=
-left: 1ex;">
- You should use queries with bind values as opposed to create the<br>
whole query string each time.=A0</blockquote><div>I'm pretty confused b=
y what can be a bound variable, and how to use them when the number of colu=
mns/values isn't known ahead of time. For example, populating a QString=
with some columns and then saying this doesn't work:<br>
query.prepare("select :columns from ... ")<br>query.bindValue(:co=
lumns, mySelectColumnsQString);<br>query.exec();<br>qDebug() << query=
.lastQuery() =3D=3D "select ? from ..."<br><br>I'll keep play=
ing around with it though, there must be a way...<br>
</div><blockquote class=3D"gmail_quote" style=3D"margin: 0pt 0pt 0pt 0.8ex;=
border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
query.exec( "SELECT * FROM symbolinfo WHERE symbol=3D'" + sym=
bolCAPS + '\'' );<br>
<br>
should be similar to this example in<br>
<a href=3D"http://doc.trolltech.com/4.6/qsqlquery.html#details" target=3D"_=
blank">http://doc.trolltech.com/4.6/qsqlquery.html#details</a><br>
<br>
QSqlQuery query;<br>
=A0 =A0 query.prepare("INSERT INTO person (id, forename, surname) &qu=
ot;<br>
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "VALUES (:id, :forename, :surname=
)");<br>
=A0 =A0 query.bindValue(0, 1001);<br>
=A0 =A0 query.bindValue(1, "Bart");<br>
=A0 =A0 query.bindValue(2, "Simpson");<br>
=A0 =A0 query.exec();<br>
<br>
That's much more efficient, and much more secure if the queries are<br>
open to user intervention.=A0<br></blockquote><blockquote class=3D"gmail_qu=
ote" style=3D"margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 20=
4, 204); padding-left: 1ex;">
<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<b=
r>
with QDebug (unless it is commented out, of course)<br></blockquote><div>Go=
od to know. And definitely does suggest I need unit tests - most of my &quo=
t;unit testing" is done manually, verifying results on the debug outpu=
t... definitely time to change that work flow :)<br>
</div><blockquote class=3D"gmail_quote" style=3D"margin: 0pt 0pt 0pt 0.8ex;=
border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<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>
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const QStringList &symb=
ols,<br>
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const QStringList &name=
s,<br>
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const QStringList &exch=
anges,<br>
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const QStringList &type=
s );<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></blockq=
uote><div>OK. When you say instance, is it best to create a DTO using new o=
r to pass-by-reference? I've always been kind of confused about underst=
anding when non-pointer objects get automatically deleted from memory. In t=
his 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 c=
an fix that one :/<br>
</div><blockquote class=3D"gmail_quote" style=3D"margin: 0pt 0pt 0pt 0.8ex;=
border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
- I've noticed you added some krazy excludes. Is there a reason for tha=
t?<br>
=A0Krazy excludes should be added only as last-instance, when it is<br>
certain there is a false positive.<br>
<br>
* =A0Copyright (c) 2010 Brian Cappello =A0 =A0 =A0 =A0 =A0 =A0//krazy:exclu=
de=3Dcopyright<br>
=A0* =A0briancappello [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></blockquote><div>OK, that was dumb, wil=
l remove those. <br></div><blockquote class=3D"gmail_quote" style=3D"margin=
: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-lef=
t: 1ex;">
<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 d=
ays.<br>
<br>
Regards,<br>
Alvaro<br>
_______________________________________________<br>
Kde-finance-apps mailing list<br>
<a href=3D"mailto:Kde-finance-apps at kde.org" target=3D"_blank">Kde-finance-a=
pps at kde.org</a><br>
<a href=3D"https://mail.kde.org/mailman/listinfo/kde-finance-apps" target=
=3D"_blank">https://mail.kde.org/mailman/listinfo/kde-finance-apps</a><br>
</blockquote></div><br>
--000feaee1006ecc822048a4b2d85--
More information about the Kde-finance-apps
mailing list