5
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ I’m not sure whether this question might actually be a better fit for SO. It can be rephrased as: “Is having a sibling class as member dangerous?” or something like that, which sounds SO-ish to me. \$\endgroup\$ – Jonas Schäfer Jun 24 '14 at 7:54
  • \$\begingroup\$ Sorry, I had the same trepidation. However, my code was a lot more comprehensive at first, but I decided an SCCCE version would get the idea across without convoluting the structure. \$\endgroup\$ – Jhomas Tefferson Jun 24 '14 at 18:49
  • \$\begingroup\$ In the text you talk about vector<Moon*> and in the code you have vector<Moon>. Which one is correct? How is the ownership relation defined (does the Planet own its moons or only reference them an there is a universe that owns all CelestialObjs?) \$\endgroup\$ – Nobody moving away from SE Jun 27 '14 at 10:13
4
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This is glorious! Thank you! I will definitely take this into consideration and rename/reorganize some structures, because I will be adding larger objects later in the project life cycle. Thank you for the informative and thorough response! \$\endgroup\$ – Jhomas Tefferson Jun 25 '14 at 7:41
  • \$\begingroup\$ @JhomasTefferson: I've been thinking about this one issue: binary systems... I can't work out an easy way to implement those in your current class hierarchy... If you don't want to change too much, you might end up with dreaded multiple inheritance (class BinaryStar : public Star, public Galaxy or something), so you can have more than 1 star, and add a solar system. But you're bound to run into the diamond problem. I know: it's a rare thing, but I'd like to know how you'd tackle this \$\endgroup\$ – Elias Van Ootegem Jun 25 '14 at 7:50
  • \$\begingroup\$ Oh sorry, I must have missed this. When I read your comment I was actually grinning. Good find. I wouldn't know how to implement that just yet. To avoid diamond inheritance, I'd have to declare the inheritance as ` class XXXX : virtual public YYYY`. I'm still relatively new to C++, so I'm learning as I go along. \$\endgroup\$ – Jhomas Tefferson Jun 26 '14 at 21:29
  • \$\begingroup\$ Also, as far as the naming goes, what term can be used to commonly describe Planets and Moons? As you've said, CelestialObj is too generic, as is CelestialBody. Whatever term I think of that can be applied to Planets/Moons/Asteroids and their derivatives, can also be applied to galaxies as a whole (AstronomicalObj/Body, etc). It'll definitely help clear up my structure. Also sorry for this being the furthest thing from programming related, lol. To further this, class OrbittingCelestialObj : public CelestialObj also applies to galaxies/solar systems, as they all orbit around a center of mass \$\endgroup\$ – Jhomas Tefferson Jun 26 '14 at 21:37
  • \$\begingroup\$ @JhomasTefferson: lol, you're relatively new to C++, but you really have chosen to pour a very complex thing into a strict hierarchical structure :). The fact that basing everything on a very generic CelestialObj is no problem. I meant to point out that your implementation of aCelestialObj was too limiting, and couldn't accomodate things like galaxies and nebula's. Perhaps you could consider changing the approach angle \$\endgroup\$ – Elias Van Ootegem Jun 27 '14 at 9:46
3
\$\begingroup\$

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

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.