Advice on vessel code structure

[tblaxland]

OK, I'm continuing to plug away at my Ananke code and I would like some advice on structuring my code. As I've added in beacons, multiple meshes and so on I've found that the member functions of the Ananke class are getting a little bloated. To try and keep the code readable and maintainable, it is my thought to move some parts of this code into other classes, like this:

class BEACON_MANAGER {
//...
};

class MESH_MANAGER {
//...
};

class Ananke: public VESSEL2 {
//...
    void SomeFunction();
    SOME_TYPE SomeVariable;

    BEACON_MANAGER *BeaconManager;
    MESH_MANAGER *MeshManager;
//...
}

The Ananke constructor (or clbkPostCreation?) would create a new instance of BEACON_MANAGER/MESH_MANAGER for each instance of Ananke. The destructor could remove those instances when a vessel is deleted.

First question, is this a reasonable way to approach the problem? If not, any other suggestions?

I'll proceed on the assumption that I am on the right track. What is a good way for the members of BEACON_MANAGER/MESH_MANAGER to get access to the members of Ananke (eg, SomeFunction/SomeVariable)? Do I need to make those members public and then pass an Ananke pointer to the BEACON_MANAGER/MESH_MANAGER constructors? If so, how would I declare such a pointer? I can't do this because class Ananke would not be defined at the point of the declaration (see comment):

class BEACON_MANAGER {
    BEACON_MANAGER(Ananke *hAnanke) { //invalid because class Ananke not defined yet?
        //...
    };
//...
};

class Ananke: public VESSEL2 {
public:
    Ananke (OBJHANDLE hVessel, int flightmodel)
        : VESSEL2 (hVessel, flightmodel);
//...
private:
    BEACON_MANAGER *BeaconManager;
//...
}

Ananke::Ananke (OBJHANDLE hVessel, int flightmodel) {
    BeaconManager = new BEACON_MANAGER;
}

Final question, I have some global "const"s that define various properties of the vessel (eg, masses and size of various components). Would these be better or worse as "static const" members of the Ananke class?

Any advice/opinions are appreciated. Apologies for the long winded post - my last attempt at restructuring AttitudeMFD so I could implement some new features ended up making the code even harder for me to maintain/debug and I don't want to make the same mistake with this vessel. Thanks in advance.

 

[Ursus]

Hmm... some good questions... Of course I'm a total newbie at programming (or at least I was when I started the project on which I'm working), but...

I'll proceed on the assumption that I am on the right track. What is a good way for the members of BEACON_MANAGER/MESH_MANAGER to get access to the members of Ananke (eg, SomeFunction/SomeVariable)? Do I need to make those members public and then pass an Ananke pointer to the BEACON_MANAGER/MESH_MANAGER constructors? If so, how would I declare such a pointer? I can't do this because class Ananke would not be defined at the point of the declaration (see comment):

I've been putting a function in my nested classes called "Init", and calling that function from the constructor of the vessel class (oops... actually, for some reason, I've been calling it from clbkPostCreation for some reason; I don't see why it wouldn't work when called from the constructor function, though). I don't know if that's really the "right way", but it seems to work:

void ICOVD1::clbkPostCreation ()
{
    Panels.Init (this);
    Accelerometer.Init (this);
    Scoop.Init (this);
    NavRelay.Init (this);
    EngineController.Init (this);
    FuelManagement.Init (this);
}

void cPanelCommon::Init (ICOVD1 * vesselpointer)
{
    mtActive = 0;
    mt_hVessel = vesselpointer;
    HelmPanel.Init (vesselpointer, this, &HelmMFDGroup);
    DockPanel.Init (vesselpointer, this, &DockMFDGroup);
    EngPanel.Init (vesselpointer, this, &HelmMFDGroup);
    HelmMFDGroup.Init (vesselpointer, this);
    DockMFDGroup.Init (vesselpointer, this);
}

Panels is an object of the cPanelCommon class, nested within the ICOVD1 class. ICOVD1 is the vessel class.

 

[computerex]

tblaxland, if there is a circular dependency within the classes, you can simply declare the class before declaring the Ananke vessel class, as long as you don't define any implicit inline functions within the class definition that are referencing the forwardly declared classes.

Also, instead of caching a pointer to the Ananke vessel class, I would cache the handle instead, that way you could get the vessel interface and cast it to an Ananke class pointer. That way you could check if the vessel is valid with the oapiIsVessel API call.

 

[tblaxland]

OK, thanks guys for the help. I'll have a play with that and see how I go.

Any opinion on the global consts vs static const members?

 

[martins]

As computerex said, you can use a forward declaration to validate a class name before you declare that class:

class Ananke;

class BEACON_MANAGER {
    BEACON_MANAGER(Ananke *hAnanke) {
        //...
    };
//...
};

After the forward declaration, the class name is valid. That is, you can use pointers to the class (which is what you want). You can't yet instantiate the class or access class members until it is declared.
You could also consider making Ananke a friend of BEACON_MANAGER, to give BEACON_MANAGER access to protected members of Ananke (i.e. making BEACON_MANAGER a "trusted" member of Ananke):

class Ananke: public VESSEL2 {
friend class BEACON_MANAGER;
public:
    Ananke (OBJHANDLE hVessel, int flightmodel)
        : VESSEL2 (hVessel, flightmodel);
//...
private:
    BEACON_MANAGER *BeaconManager;
//...
}

Finally, use the "this" pointer when constructing the BEACON_MANAGER instance from within the Ananke scope:

Ananke::Ananke (OBJHANDLE hVessel, int flightmodel) {
    BeaconManager = new BEACON_MANAGER(this);
}

 

[agentgonzo]

This will probably turn out to be quite a long post, as I tend to ramble on, but hopefully some or all of it is useful.

Class declarations: I would suggest against your ALL_CAPITALISED convention. There's not a lot of point to it and it's not consistent with your Ananke class's naming convention. Pick one convention and stick to it. Generally, ALL_CAPITALISED names are macros or constants. I'd just go with MeshManager etc.

As for the global vs static const, I'm not sure how your Ananke class is used, so I'll just ramble on about general coding standards. Under strict OO philosophy, you shouldn't be able to have global variables - everything shoud be in a class (this is why C++ isn't an OO language). Static members should be used when all instances of the class share the same resource. As it's a const, I'm presuming that it's hard coded in the source and that all members will use this value. In this case, as it's hard coded and const, it will be the same across all instances of the class if it's static or not, but you may as well make it static so that it's explicitely shared. You could also use a #define too. I'm assuming that you'll only be having one instance of your Ananke class anyway (some people like to use singleton classes for this, but for a lot of the time I can't see the point) which would make the static descriptor useless. In the end, it doesn't make a lot of difference. If the value is only used in the class, then I'd suggest staying away from the global variable option as technically it can be accessed from outside the class, which your design would not want to happen. Encapsulation tries to get data hidden where it's not needed and will thus prevent you using this value outside the class where it shouldn't be.

As for your BeaconManager and MeshManager. I'm not sure why you have chosen to have them represented as pointers in the class. I'd personally have them as objects inside the class that would be allocated on the stack and instantiated implicitely in the constructor. The code would look like the following:
Ananke.h:

class BEACON_MANAGER {
     //...
};

class MESH_MANAGER {
     //...
};

class Ananke: public VESSEL2 {
//...
         void SomeFunction();
         SOME_TYPE SomeVariable;

         BEACON_MANAGER BeaconManager;
         MESH_MANAGER MeshManager;
//...
}

Ananke.cpp:

class Ananke: public VESSEL2 {
public:
    Ananke (OBJHANDLE hVessel, int flightmodel)
        : VESSEL2 (hVessel, flightmodel)
        , BeaconManager(<args>)
        , MeshManager(<args>);
    ...

As for your Wanting MeshManager and BeaconManager to access the main Ananke class, ideally you should try to minimise this kind of behaviour. The point of OO is to get each object to be a separate object with minimal dependencies on other obects. Getting stuff into your MeshManager is fine as it's a member object of Ananke, so you can just have calls in Ananke that call MeshManager.MyFunc() etc. Why does MeshManager need to call Ananke? If it's just a case of returning information to Ananke, then you can have it as the return value of a function or call a Setxxx() function with a load of references or pointers to set variables. It makes the code a lot more readable if the flow of control is one-way (Ananke calling MeshManager) and MeshManager just returning information when asked for it.

If you really want MeshManager calling Ananke, then you can do it with the passing of the this pointer as mentioned above. You'll need to be careful with circular dependencies as you can't include Ananke.h in MeshManager.h and vice versa. You should include MeshManager.h in Ananke.h, but at the top of MeshManager.h, declare

class Ananke;

Do not include any inline functions that call methods or data of Ananke in MeshManager.h as they won't know that they exist. Then in MeshManager.cpp include Ananke.h so that it can call the functions.

Having several Init functions is a good idea if the constructor gets bloated and unreadable as you can very quickly see what it's doing without trawling loads of lines of code.

 

[tblaxland]

As computerex said, you can use a forward declaration to validate a class name before you declare that class...
...After the forward declaration, the class name is valid. That is, you can use pointers to the class (which is what you want). You can't yet instantiate the class or access class members until it is declared.

Thankyou for clarifying.

You could also consider making Ananke a friend of BEACON_MANAGER, to give BEACON_MANAGER access to protected members of Ananke (i.e. making BEACON_MANAGER a "trusted" member of Ananke)

Interesting, I hadn't heard of these class friends before (makes me feel like I'm back in school ).

Class declarations: I would suggest against your ALL_CAPITALISED convention. There's not a lot of point to it and it's not consistent with your Ananke class's naming convention. Pick one convention and stick to it. Generally, ALL_CAPITALISED names are macros or constants. I'd just go with MeshManager etc.

Fair point.

As it's a const, I'm presuming that it's hard coded in the source and that all members will use this value. In this case, as it's hard coded and const, it will be the same across all instances of the class if it's static or not, but you may as well make it static so that it's explicitely shared. ...If the value is only used in the class, then I'd suggest staying away from the global variable option as technically it can be accessed from outside the class, which your design would not want to happen. Encapsulation tries to get data hidden where it's not needed and will thus prevent you using this value outside the class where it shouldn't be.

Your presumption is correct. Static const it is then.

As for your BeaconManager and MeshManager. I'm not sure why you have chosen to have them represented as pointers in the class.

Inexperience

As for your Wanting MeshManager and BeaconManager to access the main Ananke class, ideally you should try to minimise this kind of behaviour. The point of OO is to get each object to be a separate object with minimal dependencies on other obects. Getting stuff into your MeshManager is fine as it's a member object of Ananke, so you can just have calls in Ananke that call MeshManager.MyFunc() etc. Why does MeshManager need to call Ananke? If it's just a case of returning information to Ananke, then you can have it as the return value of a function or call a Setxxx() function with a load of references or pointers to set variables. It makes the code a lot more readable if the flow of control is one-way (Ananke calling MeshManager) and MeshManager just returning information when asked for it.

Fair comment on the flow of control. I haven't designed MeshManager etc in any great detail yet but I'll take this comment on board as I do.

If you really want MeshManager calling Ananke, then you can do it with the passing of the this pointer as mentioned above. You'll need to be careful with circular dependencies as you can't include Ananke.h in MeshManager.h and vice versa.

This is the trouble I got myself into when restructuring AttitudeMFD. It is going to take me a while to undo that mess so best not to get into a similar situation again.

Thank you very much for such helpful advice - it has really clarified my thinking on this. Now for the really big decision of the evening... or

?

 

[agentgonzo]

Thank you very much for such helpful advice - it has really clarified my thinking on this. Now for the really big decision of the evening... or

?

:leaving: --> --> :dance2:--> -->

--> -->-->:compbash:

 

[tblaxland]

:leaving: --> --> :dance2:--> -->

--> -->-->:compbash:

-->:beach:? Oh wait, I just found this keyboard thingy that I can type words with... :lol:

Class declarations: I would suggest against your ALL_CAPITALISED convention. There's not a lot of point to it and it's not consistent with your Ananke class's naming convention. Pick one convention and stick to it. Generally, ALL_CAPITALISED names are macros or constants. I'd just go with MeshManager etc.

Going back to this one, OrbiterSDK uses ALL_CAPITALISED for class names, eg, VESSEL, MFD, BEACONLIGHTSPEC, etc. Question is, do you stick with the SDK conventions or go with something else like this? (EDIT: or even like this used for the JSF, as recommended by Stroustrup) Personal preference I guess.

 

[mjessick]

-->
Going back to this one, OrbiterSDK uses ALL_CAPITALISED for class names, eg, VESSEL, MFD, BEACONLIGHTSPEC, etc. Question is, do you stick with the SDK conventions or ...

I see a benefit to using my own convention for my variables, because then it is clearer which code is dealing with the SDK.

 

[tblaxland]

Thanks

The coding is coming along nicely and I thought I would fill you in on the structure that I arrived at (you never know, it might be of use to someone else ).

To avoid having the MeshManager and BeaconManager classes calling Ananke I took computerex's advice of caching the object handle. The MeshManager and BeaconManager classes do their work (AddMesh, AddBeacon, etc) by accessing the vessel interface returned by oapiGetVesselInterface.

Per agentgonzo and mjessick's comments, I've changed my naming style to camelCaps notation.

Next, I moved the global consts to static const class members. Per agentgonzo's comments, it doesn't make a lot of difference performance-wise but it does make the code cleaner.

Finally, I drew up a rough UML class diagram so I could get an overall picture in my mind of what I was doing. This is attached FYI. It does not show all the detail of the various classes but it shows the overall structure pretty well.

Thankyou everyone for helping me get here.