HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
samethebothtypedeclaringmemberbaseanothersharewhich

Problem

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 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 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?

Solution

The code itself

I have little to say about the code itself, other than to address your concerns with the vector 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&& 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, 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 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 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/

Code Snippets

class Planet : public CelestialObj
{
    public:
        Planet & add_moons  (vector<Moon>&& vMoon){};
        Planet & add_moon  (Moon&& moon){};//copy push_back
};
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;
}

Context

StackExchange Code Review Q#55082, answer score: 4

Revisions (0)

No revisions yet.