Cons of declaring derived class type member in another derived class (both of which share the same base class)

Question

I'd like some input on the way I've structured some things in a side-project I'm working on.

I declare an abstract class, CelestialObj, which will contain the common elements between Planets, and Moons (both of which I'd like to be derived from CelestialObj, obviously).

Now, a Planet must keep track of its Moon objects. Therefore, each of my Planet objects will contain a vector<Moon*> stack. I was wondering if this is bad practice or can cause me trouble later down the line? I can't really find much information regarding this issue on various forums/google searches. Here's a SCCCE version of the code:

class CelestialObj {
protected:
    int size;
    //virtual function definitions here, imaging they're in Moon/Planet too
};

class Moon : public CelestialObj {
public:
    int distFromPlanet;
    Moon() : distFromPlanet(10), size(1000) { }
};

class Planet : public CelestialObj {
public:
    vector<Moon> moons;
    Planet() : size(2000) { Moon mun; moons.push_back(mun); }
};

The actual code is much more organized, with members declared protected/private where appropriate, but this conveys the intended logic.

Is there anything that stands out as dangerous? Or that should be avoided? Minor things such as size being const, and public/private/protected declarations should not be discussed, as they're fine in my actual code. It's just the organization I'm kind of worried about. Is there an alternate organization method anyone recommends? If so, why?

Answer 1

The code itself
I have little to say about the code itself, other than to address your concerns with the vector<Moon> in your Planet class. If you have a C++11 compiler, you might want to add a move constructor and/or provide methods that exploit the move semantics as much as possible. Something like:

class Planet : public CelestialObj
{
    public:
        Planet & add_moons  (vector<Moon>&& vMoon){};
        Planet & add_moon  (Moon&& moon){};//copy push_back
};

The rest of this answer is more about the limitations of your model, and some little niggles I have with a couple of things you mentioned.

At first glance, I don't see any glaring omissions or down-right dangerous code. However, I am a bit of a space-geek at times, and I find the name of your base class a tad risky/counter intuitive.
To me a CelestialObj could be anything ranging from a Planet, a Moon, some Asteroid, Meteorite Planetoids and DwarfPlanets, all the way through to a Star, Nebula, BlackHole, and even things like Galaxy or a Constellation could be considered a celestial object.

Because I don't know what you are going to use these classes for, and not knowing how detailed you want to go, I'm just going to point out a couple of things that you might want to keep in the back of your head.
It's easier to change your approach in the earlier stages, than it will be further down the line.

Of course, a Galaxy is but a CelestialObj that contains a vector<CelestialObj>, mainly Star instances. Each Star can contain a similar vector, representing solar systems and so on.
A Planetoid could be implemented as a specific type of Asteroid, just like a DwarfPlanet is a variation on the Planet class. Great, all this can be made to fit your current hierarchy perfectly, but I would change one thing:

Currently, your Planet class has a vector<Moon> property. Given that there can be objects in orbit around moons, planets can have other objects in its orbit (ie Saturn's rings etc), and that, well, basically: anything can revolve around anything, I'd move the vector up to the CelestialObj class, and make it more generic (vector<CelestialObj> satellites;).

This should allow you to fit more into your current model, but the moment you add bigger objects, or you want to create more complex systems, you'll find it simply can't cope with the added complexities.

When you add nebula's and black holes to the mix, then things become quite tricky, of course, as things like luminescense aren't simple properties.
Forgive my geeky-ness here, but computing the brightness and luminosity of stars is fairly simple, but a galaxy, well: the sum of all celestial bodies isn't quite enough. There's reflection to take into consideration, and black holes trap light in its orbit, aswell as bend the light or even absorb it.

Note:
Now I know you didn't want to discuss minor things, but if ever you extend the CelestialObj class to create a Star, you'll find that a const int size and const int mass is as pointless as a broken pencil. Either way, in terms of celestial bodies: I'd consider it acceptable to use a bigger datatype than a mere int: I'd use a double.
To fix this, you might choose to create a class like this:

class Star : public CelestialObj
{};
class DyingStar : public Star
{
    public://assume double size;
        DyingStar (Star &dead_star_shining, long ttl){};
        Star &pass_time ( long time ){};//heat up planets, swallow closest, alter size_factor
        double getSize( void ) { return size * size_factor; };
    protected:
        double size_factor;
}

Other things that I'm not even going to touch on include neutron stars, wormholes, binary star systems, the list is endless...

Again, this isn't as much a critique of your actual code. This "review" is meant more as a sort of "consider this, how would you fit it into your current model?" type of deal.

In response to comments:
You probably realized that you've chosen a hideously complex thing to try and represent in a relatively simple hierarchical structure. I would, however advise you to not try and use the CelestialObj base class as the only base-class, to extend on to represent well, the entire universe basically.
If you did that, then the only suitable class name would be class BigBang, which, by definition, would have to use multiple inheritance, inheriting both from the Quantum, Matter, DarkMatter, Space and tons of other classes, and would have to implement the Chaos interface. Depending on who you ask, you'd probably have to use the God template, too. Save yourself the trouble, and just use a set of base classes:

• CelestialObj is fine, and can be split up into 2 main groups: ReflectingObj (ie: bodies that don't emit light: planets, moons, asteroids) and FusionObj (suns in various stages, including dying suns, neutron stars, pulsars and... The core of a black hole. Everything that is either the remains of a star or that consumes light/celestial bodies).

• Cluster: A base class that is either vector-like or doubly linked-list like you can feed CelestialObj instances to, to form SolarSystems, Galaxy's and the actual BlackHole class (because black holes can have light in orbit around it. Light has dual properties (energy and matter, so if you want: create a separate class for this tricky little bugger)
  Special cases of Clusters include, for example, Constellations, as they can be a group of stars, combined with galaxies (ie the constellation Orion). The members of a constellation might be too far apart for any of its members to influence the other, so this cluster doesn't have to provide methods for member-interaction, but it should be able to group instaces of CelestialObj alongside Cluster isntances.

Answer 2

Recursive type definitions are common. The thing to remember is whether or not your model whatever you're modelling. In you case ask yourself

• whether a planet is a celestial object
• whether a moon is a celestial object
• whether a planet can have moons

of course the answer is yes to all three and that proves your model is sensical from that respect. Last question would then be "Does my chosen platform allow me to express this model" and the answer to that is also yes.

In a more generalised form. When you consider inheritance ask your self whether there's a "is-a" relationship between objects of the derived and base classes. If the answer is no don't use inheritance. If the answer is yes it's a candidate for inheritance (doesn't mean you should always use inheritance though). For members as yourself whether the containing model has a "has-a" relationship to the contained model in the real world. If not don't included it in the containing model. If yes consider composition a candidate

Answer 3

There is no dangerous issue with that code. It would be even okay if you had a direct Moon member, instead of having a vector of Moons (this is worth noting, because with a vector (just as with a pointer), the actual Moon object is not in the same memory region as the Planet object).

There might be some performance issues (and/or surprises) though, as you’re copying Moons when you’re pushing them into the vector. You might want to switch to std::unique_ptr if that behaviour is not desired.

Otherwise, it looks intuitive and okay to me.