GET GET GET GET GET DO - Overdesigning? Efficiency vs Consistency

Question

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?

Answer 1

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 ;-)

Answer 2

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);