<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/105481/">http://git.reviewboard.kde.org/r/105481/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've got a few comments, mainly about minor issues. So:
1) Remove stray whitespaces in your code - they're marked with red colour in diff view on the reviewboard.
2) Use consistent naming pattern for source files and folders - all lower case characters (WIView.h/cpp, WhatsInteresting folder).
3) Use consistent code formatting patterns. There are numerous examples when you use different 'styles' even in the same source file.
4) Add Doxygen-style comments at least in all your header files. They don't need to be extra-elaborate, but you definitely shouldn't leave anything uncommented.
5) Your variable/function naming conventions are good, but try to avoid in-descriptive names like ModelManager *m. Maybe you'd like to use prefixes for member variables? That's not required but I find it quite helpful when done in correct way.
6) Try to use 'const' keyword a little bit more. KStars code is definitely mostly const-incorrect, but try to make it a little bit better in new classes.
7) Use references instead of passing parameters as a value. In case of some Qt classes it doesn't make a big difference, because of implicit data-sharing, but it's generally a good thing to do.
Now to particular things in your code:
- Hard-coded URL in file wiview.cpp [diff line 1941]: baseView->setSource(QUrl("/home/sam/kstars/kstars/tools/WhatsInteresting/Base.qml"));
- Memory leaks! Make sure that you know when you need to delete objects created on a heap manually and when QObject hierarchy will take care of this. Find and remove them yourself, I will check it in future. You can also check for memleaks with Valgrind.
- I feel that code of WIView class is a little bit too obfuscated. Poke me on IRC and we'll talk about it.
- Why do you do something like this:
+ switch (o->type())
+ {
+ case 0:
+ sotype = SkyObject::STAR;
+ break;
+ case 3:
+ sotype = SkyObject::OPEN_CLUSTER;
+ break;
+ case 6:
+ sotype = SkyObject::PLANETARY_NEBULA;
+ break;
+ case 8:
+ sotype = SkyObject::GALAXY;
+ break;
+ case 11:
+ sotype = SkyObject::CONSTELLATION;
+ break;
+ default:
+ break;
+ }
When SkyObject::type() returns an enum?
- Use enums instead of QString comparisons. It's faster. It's more elegant. It works with switch-case statements.
- Use + operator instead of QString::append() when joining long strings. It looks nicer.
- Again, const-ify things that don't change, like QString cardinals[] in SkyObjItem::setPosition(). Another thing is that you can use QStringList instead of table of strings.
I have many other comments. Poke me on the IRC to get them.</pre>
<br />
<p>- Rafal</p>
<br />
<p>On July 8th, 2012, 5:55 p.m., Samikshan Bairagya wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KStars, Rafal Kulaga and Akarsh Simha.</div>
<div>By Samikshan Bairagya.</div>
<p style="color: grey;"><i>Updated July 8, 2012, 5:55 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">QML user interface for testing purpose ready. This shows planets, bright stars, constellations and deep-sky object. Description not yet available for bright stars and constellations. The details-view is not ready for DSOs. Also the list of DSOs displayed is not the filtered or interesting list.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Testing done for planets, constellations, bright stars. Details-view not yet ready for DSOs.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kstars/CMakeLists.txt <span style="color: grey">(1113333)</span></li>
<li>kstars/data/CMakeLists.txt <span style="color: grey">(0dc591b)</span></li>
<li>kstars/data/Interesting.dat <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/data/PlanetFacts.dat <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/data/initWIList.dat <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/indi/indielement.h <span style="color: grey">(93be2ba)</span></li>
<li>kstars/kstars.h <span style="color: grey">(959f042)</span></li>
<li>kstars/kstarsactions.cpp <span style="color: grey">(e2f8fc5)</span></li>
<li>kstars/kstarsinit.cpp <span style="color: grey">(bbc70fe)</span></li>
<li>kstars/kstarsui-indi.rc <span style="color: grey">(d2bbdb0)</span></li>
<li>kstars/kstarsui-win.rc <span style="color: grey">(20a2fa0)</span></li>
<li>kstars/kstarsui.rc <span style="color: grey">(1f28415)</span></li>
<li>kstars/tools/WhatsInteresting/Base.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/tools/WhatsInteresting/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/tools/WhatsInteresting/WIView.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/tools/WhatsInteresting/WIView.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/tools/WhatsInteresting/modelmanager.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/tools/WhatsInteresting/modelmanager.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/tools/WhatsInteresting/skyobjitem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/tools/WhatsInteresting/skyobjitem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/tools/WhatsInteresting/skyobjlistmodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kstars/tools/WhatsInteresting/skyobjlistmodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/105481/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>