[Kstars-devel] Refactor of the moons handling classes

[Khudyakov Alexey]

On Четверг 06 августа 2009 08:21:23 Médéric Boquien wrote:
> Hello,
>
> I think the situation with the classes handling the Jupiter and Saturn
> classes is not satisfying.
> * the jupitermoonscomponent.* and saturnmoonscomponent.* files are
> basically copies of each other with only small details that are different,
> * same for jupitermoons.* and saturnmoons.* except for the method
> calculating the position of the satellites which are of course
> planet-specific. In addition to the code duplication, which is nefarious it
> makes adding the moons of other planets cumbersome.
> As patches are better than just whining i have attached a patch which
> should improve the situation. Here is what it does mainly:
> * it removes JupiterMoonsComponent and the SaturnMoonsComponent classes and
> replaces them with a PlanetMoonsComponent class,
> * it factors the sharable code of JupiterMoons and SaturnMoons into a new
> base class PlanetMoons. Only the code specific to the calculation of the
> location of the satellites is left into the subclasses.
> As i am not very good at C++ i would appreciate if someone could have a
> look before i commit it.
>
Nice work. 

1. Destructor of PlanetMoons doesn't free TrailObjects. Call qDeleteAll(Moon) 
should be done in PlanetMoon rather than in JupiterMoons or SaturnMoons.

2. Copy ctor and assignment operator should be made private in order to 
prohibit copying (and subsequent double free of trailObjects). It's common C++ 
idiom. 
Code snippet:
    PlanetMoons(const PlanetMoons&);
    PlanetMoons& operator = (const PlanetMoons&);

3. nmoons should be accessor method of PLanetMoons not a data member in 
PlanetComponents. Number of moons is known by PlanetMoons and there is no need 
in data duplication. 

And few more comments of more cosmetic matter 

* In PlanetMoons::moonNamed break is not required since it's unreachable.

* In would be nice to add virtual annotation to destructor and findPosition. 
It's just to be explicit about it. IMHO it makes easier to understand code.