[Kstars-devel] Review Request: My entire GSoC work till now. What's Interesting works for planets, bright stars and constellations now.

[Rafal Kulaga]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105481/#review15701
-----------------------------------------------------------


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.

- Rafal Kulaga


On July 8, 2012, 5:55 p.m., Samikshan Bairagya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105481/
> -----------------------------------------------------------
> 
> (Updated July 8, 2012, 5:55 p.m.)
> 
> 
> Review request for KStars, Rafal Kulaga and Akarsh Simha.
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   kstars/CMakeLists.txt 1113333 
>   kstars/data/CMakeLists.txt 0dc591b 
>   kstars/data/Interesting.dat PRE-CREATION 
>   kstars/data/PlanetFacts.dat PRE-CREATION 
>   kstars/data/initWIList.dat PRE-CREATION 
>   kstars/indi/indielement.h 93be2ba 
>   kstars/kstars.h 959f042 
>   kstars/kstarsactions.cpp e2f8fc5 
>   kstars/kstarsinit.cpp bbc70fe 
>   kstars/kstarsui-indi.rc d2bbdb0 
>   kstars/kstarsui-win.rc 20a2fa0 
>   kstars/kstarsui.rc 1f28415 
>   kstars/tools/WhatsInteresting/Base.qml PRE-CREATION 
>   kstars/tools/WhatsInteresting/CMakeLists.txt PRE-CREATION 
>   kstars/tools/WhatsInteresting/WIView.h PRE-CREATION 
>   kstars/tools/WhatsInteresting/WIView.cpp PRE-CREATION 
>   kstars/tools/WhatsInteresting/modelmanager.h PRE-CREATION 
>   kstars/tools/WhatsInteresting/modelmanager.cpp PRE-CREATION 
>   kstars/tools/WhatsInteresting/skyobjitem.h PRE-CREATION 
>   kstars/tools/WhatsInteresting/skyobjitem.cpp PRE-CREATION 
>   kstars/tools/WhatsInteresting/skyobjlistmodel.h PRE-CREATION 
>   kstars/tools/WhatsInteresting/skyobjlistmodel.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105481/diff/
> 
> 
> Testing
> -------
> 
> Testing done for planets, constellations, bright stars. Details-view not yet ready for DSOs.
> 
> 
> Thanks,
> 
> Samikshan Bairagya
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kstars-devel/attachments/20120712/9ad5ef27/attachment.html>