1

I know the disadvantages of public variables / advantage of private variables with GET and SET functions, but currently I am working on my first own 'real' game using Ogre3D (C++).. Meanwhile, I sometimes need 6-7 getFunctions() to access the data I need to perform a single operation on it. So, I am wondering if this isn't overkill/overdesigning and when it may be usefull to store a new direct pointer to certain objects in other classes/functions. But this doesn't sound like good consistancy, good programming style and/or readability. (I am from Germany and actually dont really know if 'consistancy' is the right word to use, correct me if not. I mean the homogeneity of the code-structure eg the whole code follows a tight programming style)

So, how much time does it take for one GetFunction (return pointer to object)? Is it overdesigning to have about 5-7 Get's?

Or: For which X in |R calls of an ObjectPointer over Y in |R getFunctions() is it recommendable to create a local copy of the object-pointer?

May some of you say this is over-optimizing, but another aspect is the readability with getFunctions()..

An example from my game to get the number of Swordsman the local player has in a certain "base".

int numSwordsman =  GameManager::getSingletonPtr()->getMatchInstance()->getPlayerData()->getLocalPlayer()->getGameElementManager()->getBase(EWT_MAINBASE)->getSwordsmann();

Even worse is something like this 

Ogre::Vector2 newVec2 = Ogre::Vector2(GameManager::getSingletonPtr()->getSceneManager()->getCameraNode()->getViewport()->getActualHeight() / GameManager::getSingletonPtr()->getMainGameLoop()->getMouse()->getState()->x.abs,GameManager::getSingletonPtr()->getSceneManager()->getCameraNode()->getViewport()->getActualWidth() GameManager::getSingletonPtr()->getMainGameLoop()->getMouse()->getState()->y.abs);

Beside the fact that you need about 20-30 seconds to understand whats going on there, its a huge pain in the......to type that.

Personally I prefer to write such code like 

Ogre::Vector2 newVec2 = Ogre::Vector2(GameManager::getSingletonPtr()->
     getSceneManager()->getCameraNode()->getViewport()->getActualHeight()
          / GameManager::getSingletonPtr()->getMainGameLoop()->getMouse()->getState().X.abs,
     GameManager::getSingletonPtr()->
          getSceneManager()->getCameraNode()->getViewport()->getActualWidth() 
          / GameManager::getSingletonPtr()->getMainGameLoop()->getMouse()->getState().Y.abs);

But for someone who doesn't know my coding style, it may be confusing as **** and beyond every human known programming style and practice.

What do you think about this topic? Is this whole Get->Get->Get->Get->Get overdesigning? Or inevitable necessary? How do you handle 'far away' object pointers? Unimportant topic or relevant for performance intensiv applications? Any tipps and tricks about code-design? Or are 7-8 'Get's still standard?

Deduplicator
  • 44,692
  • 7
  • 66
  • 118
Sebastian Müller
  • 115
  • 7
  • 5
    `GameManager::getSingletonPtr()->..` can probably be replaced by a variable (or two), the object to which it refers, accessed multiple times. This isn't for "performance", but for "code that doesn't suck to read". – user2864740 Dec 19 '14 at 09:25
  • If you return pointers with `getX()` you should check each step for validity. I'd prefer returning references and use a syntax like `root.getX().getY().getZ()`.` – πάντα ῥεῖ Dec 19 '14 at 09:26
  • Since you are using singleton, you could also use a static "wrapper" function for most often used getter: instead of GameManager::singleton()->getx()->gety()->getz() you could use e.g. GameManager::getXYZ()... – Robert Dec 19 '14 at 09:28
  • I don't have a definitive answer (and of course there isn't one), but here comes a few observations: 1) OGRE is of notorious bad design, and your readability issues are a direct consequence of this and 2) Your classes should probably work on a few OGRE-related object, to which you can keep references as data members. – slaphappy Dec 19 '14 at 09:29
  • Use local variables! That is what they are for. You don't need to re-get the singleton every single time, unless you are expecting it to change due to state updates. – djgandy Dec 19 '14 at 09:30
  • @djgandy _"Use local variables!"_ For singletons this means use local references. – πάντα ῥεῖ Dec 19 '14 at 09:31
  • _@SebastianMüller_ Also have a read about what I've been answering for a similar question yesterday: [Creating private variable objects](http://stackoverflow.com/a/27558005/1413395) – πάντα ῥεῖ Dec 19 '14 at 09:42
  • @πάντα ῥεῖ - Well if we are going to get picky, the singleton is actually a pointer, not a reference. Although I think we both know this is all semantics and the point being made was to dereference to locally scoped variables – djgandy Dec 19 '14 at 09:58
  • 3
    "advantage of private variables with GET and SET functions". Those are few and far between. If you have many such getter-setter pairs, question your design. Member variables with just getters are more OK. Also, look up "the law of Demeter". – n. m. could be an AI Dec 19 '14 at 10:30
  • Chained getters are the Java version of three-star programming (http://c2.com/cgi/wiki?ThreeStarProgrammer), and should be avoided. The trick is, to define member functions that do relevant work instead of just exposing data. Of course, if it's the design of your framework that's broken, you are out of luck; in that case the only thing that's left to you is to mitigate the problem by use of local variables, or to switch to another framework. – cmaster - reinstate monica Dec 19 '14 at 10:37

2 Answers2

2

I would rewrite your code example as:

auto GM = GameManager::getSingletonPtr();
auto VP = GM->getSceneManager()->getCameraNode()->getViewport();
auto mouse = GM->getMainGameLoop()->getMouse()->getState();

Ogre::Vector2 newVec2 = Ogre::Vector2(
     VP->getActualHeight() / mouse.X.abs,
     VP->getActualWidth() / mouse.Y.abs);

Just for readability; efficiency-wise I doubt it can be worse, and it might be better.

(Also, with no knowledge of what is happening below the surface, I'd rather grab mouse state once: I don't want to get X and Y either side of a mouse movement!)

If efficiency is a concern, compile with all optimizations on, then look at the generated assembler code. Compare the before/after of a code refactoring for clarity; maybe it will be the same. (The only thing to be careful of here is that compiler technology is always improving, so a best practice now might not be a best practice in two years.)

One more tip: read Martin Fowler's Refactoring book, to improve your intuition for when adding a layer of abstraction is good, and when collapsing a layer of abstraction is good. Yeah, I know it is now 15 years old, but IMHO the advice has held up very well... even if the examples are all in java ;-)

Darren Cook
  • 27,837
  • 13
  • 117
  • 217
  • I think it's more about the generell coding style, so thanks DarrenCook! – Sebastian Müller Dec 19 '14 at 10:36
  • @cmaster Yes, fair point, I could've chosen `gameManager` and `viewport`. Consistency throughout the project is most important, so if someone sees `VP` (or whatever) they know it is a viewport object. – Darren Cook Dec 19 '14 at 10:46
  • 1
    @cmaster: They are self-explaining. Short names have small scopes, and these are in fact such local names. You'll almost always have their definition visible whenever you seem them used. Naming it `viewport` might even suggest that the definition is _not_ local. – MSalters Dec 19 '14 at 11:23
  • 1
    Inferring scope from length of a name, that's a new one to me! I don't think anyone should be doing that. A chosen name should be clear and descriptive. It is there to make the life of the human easier so exploit that. The exception is as Darren said, if there is already another convention in place and re-factoring is not an option. – djgandy Dec 19 '14 at 11:46
1

This is how I'd change what you have given so far. Key reasons are readability and most important of all debug-ability. Storing off results of functions into locals allows for easy state inspection in the debugger. I make no guarantees this is syntactically correct, it was written in notepad, but you get the idea.

The statement of actual work is now much clearer and not obscured buy your hunt for state, which is pretty hideous. Your program does some to suffer from object orientation overload, but maybe that's the engine you are using. I can't really say from a snippet.

// How many are dereferenced is down to the judgement of the programmer
// If you think inspection of the state is useful in the debugger or
// validation against null is required then dereference.
// If it's never null, or simply not useful, then I don't really have 
// an objection to chain dereferencing.
SingletonPtrType singleton_ptr = GameManager::getSingletonPtr();
SceneManagerType scene_manager = singleton_ptr->getSceneManager();
CameraNodeType camera_node = scene_manager->getCameraNode();
MouseStateType mouse_state = singleton_ptr->getMainGameLoop()->getMouse()->getState();

ViewPortHeightType height  = camera_node->getViewport()->getActualHeight();
ViewPortWidthType width = camera_node->getViewport()->getActualWidth();

Ogre::Vector2 newVec2 = Ogre::Vector2(height / mouse_state.X.abs, width / mouse_state.Y.abs);
djgandy
  • 1,125
  • 6
  • 15
  • Thanks for your answer! The first code snippet (numSwordsman) is mainly my own written game logic. I tend to put everything possible into a class. What do you think about a singleton class that holds all often accessed pointers, for example the singleton-instance, the actual viewport, height and width of the window, pointer to the local player etc.. Is something like that recommendable? – Sebastian Müller Dec 19 '14 at 10:13
  • 1
    What will that class achieve? You then just have more pointer aliases. Forget Object Oriented design for now and just write your program. The program is clearly not OO at all from what I can see so far and trying to be OO when you don't understand what OO entails just leads to very bad practices. My gut would say that your camera node should be getting mouse position updates rather than you using all the state to compute your newVec2. Half the code disappears then. – djgandy Dec 19 '14 at 10:33
  • That code snippet was just imaginary and should only represent the generell problem but thanks for the tipps! – Sebastian Müller Dec 19 '14 at 10:38
  • If there's one bit of code that would benefit from `auto`, it would be this. – MSalters Dec 19 '14 at 11:20
  • I agree, my use of C++ pre-dates that keyword though. – djgandy Dec 19 '14 at 11:29