Sign in to follow this  
yckx

How many Getters should it take to get an object?

Recommended Posts

I've been looking at my code lately, and there are a few places where I have code such as:
[source lang="cpp"]
#include "Display.hpp"

[font="Lucida Console"]void SomeClass::Update( float dt )
{
[/font][font="Lucida Console"] Camera& camera = _app.GetDisplay().GetCamera();[/font]
[font="Lucida Console"] // do stuff[/font]
}
[/source]
[font="Lucida Console"]_app[/font] is a reference to the one and only instance of the [font="Lucida Console"]Application[/font] class, which gets passed around as a global entry point to the subsystems and whatnot. The thing I don't like about it is that I'm having to [font="Lucida Console"]#include "Display.hpp"[/font] solely for the [font="Lucida Console"]GetCamera()[/font] method. One little getter.

I'm tempted to just add a [font="Lucida Console"]GetCamera([/font]) to my [font="Lucida Console"]Application[/font] class, which would [font="Lucida Console"]return Display::GetCamera()[/font]. This would let me uncouple [font="Lucida Console"]Display.hpp[/font] from these files, but the idea is to provide access to systems, not every potentially useful object in the game. So I'm torn.

What are your thoughts on the matter?

Share this post


Link to post
Share on other sites
[quote name='RDragon1' timestamp='1306471231' post='4816299']
What is a Display, and why does it have Camera data?
[/quote]
Display is a subsystem. It pretty much owns everything related to putting stuff on the screen.

[quote name='Hodgman' timestamp='1306471542' post='4816300']
Chains of getters is a code-smell.
Hiding that smell by making a higher level getter that contains the chain makes it look better, but is actually just making the design worse ([i]now not only does app know about displays, but it also knows directly about cameras too[/i]).
[/quote]
Yes, I didn't like either method. That's why I asked.

[quote]
The problem is that you've got this global 'app', meaning that every part of your program knows about app.
[url="http://en.wikipedia.org/wiki/Law_of_Demeter"]Ideally[/url], each part of the program only knows about the parts that it absolutely has to. So I would rewrite your sample as:[code]void SomeClass::Update( float dt, Camera& camera )
{
// do stuff
}[/code]Now SomeClass doesn't know about Apps or Displays, and only knows about the thing it needs to (the Camera).
[/quote]
Thanks for the guidance.

Not every part of my program knows about app. It's not a true global. But each subsystem does. I've taken pains to not pass it any further down the chain than that.

This is my first project of any real size, and going into this I figured I'd end up with design issues. Even questions like what should own the camera--it seems obvious to me that it's a display-related class, but I've found good design isn't always obvious design. Your suggestion should have been obvious; I think I've been looking at my code for too long ;)

Share this post


Link to post
Share on other sites
[quote name='yckx' timestamp='1306473900' post='4816305']
Not every part of my program knows about app. It's not a true global. But each subsystem does. I've taken pains to not pass it any further down the chain than that.
[/quote]

Then think about this: do all your subsystems really need to know whole application?
I highly doubt that.
Maybe one subsystem only needs to know render, other needs to know motion.
Then the subsystem only depends on the smallest module. So only depends on render instead of whole application.

It's best to only depend on the module you need, never try to depend the module you don't need.

With this rule, in your case, the subsystem knows a camera object instead of app, so you can write
this->camera->doSomething()
instead of
_app.GetDisplay().GetCamera().doSomething()

Share this post


Link to post
Share on other sites
I would think about how you can get rid of needing to access the camera entirely. What sort of things are you needing the camera for? Think about a model/view split. Ideally, your model should have all it needs (not always possible) and then the view should look at that model and deal with it appropriatly.

Another option is some form of event/message system, you could send camera events (change position, change fov, etc) and have the display pick them up and deal with them. That supports setting quite well but getting is difficult. One way to get around that is for the model part to have a camera thats just data and something to send messages to the display.

I do a similar approach to this and never have to access camera stuff directly. I guess it all depends on what type of data you need from your camera.

Share this post


Link to post
Share on other sites
I personally have a "world" class with a list of cameras and a list of meshes. I pass around a viewport class containing the current display dimensions.

Share this post


Link to post
Share on other sites
The general consensus on "modern" design is: Tell, don't ask.

In Java, the Dependency injection is preferred these days. So instead of asking for objects, they are passed in directly:[code]class SomeClass {
public:
SomeClass(Camera * c, Display * c) // constructor merely sets all the dependencies
: camera(c)
, display(d)
{}
void update() {
camera->foo();
display->bar();
}
private:
Camera * camera;
Display * display;
};[/code]
This concept can transfer to C++ directly.

However, since C and C++ allow for free functions, another preferred option is to not use member functions at all. So the above would be just a plain function:[code]void update(Camera * c, Display * display) {
camera->foo();
display->bar();
}[/code]

Share this post


Link to post
Share on other sites
[quote name='Nanoha' timestamp='1306481490' post='4816338']
I would think about how you can get rid of needing to access the camera entirely. What sort of things are you needing the camera for? Think about a model/view split. Ideally, your model should have all it needs (not always possible) and then the view should look at that model and deal with it appropriatly.

Another option is some form of event/message system, you could send camera events (change position, change fov, etc) and have the display pick them up and deal with them. That supports setting quite well but getting is difficult. One way to get around that is for the model part to have a camera thats just data and something to send messages to the display.
[/quote]
Well, this particular instance of code is in the World::Update() method. The camera is highly mobile, changing position and rotation based on player movement. I'm just updating the camera transform based on its relation to the player, and the player's relation to the world.

I've considered an event system. I created a test app with libsigc++, but ran into major problems trying to get classes to register events and listeners with the system. But that's for another thread. I haven't tried any other libraries yet. Other than Boost.signals, I'm not sure what's available and good.

[quote name='owl' timestamp='1306481834' post='4816340']
I personally have a "world" class with a list of cameras and a list of meshes. I pass around a viewport class containing the current display dimensions.
[/quote]
Moving the camera to the World class may a better solution than passing it, in this case.

Share this post


Link to post
Share on other sites
Given that I know nothing about your code, I'm confused why you have Camera within Display. While logically a display is displayed with a camera I would think your object structure, if you are making them interdependent would be inverted. Cameras [i]use [/i]displays to show their field of view Your binding of a camera to a display may also seems to negate the ability to have multiple monitor support. This is all just reading logically and trying to rationalize how your objects interrelate so take this with a grain of salt but you may definitely want to re-examine why you have those objects linked the way you do.

Share this post


Link to post
Share on other sites
[quote name='landlocked' timestamp='1306516115' post='4816491']
Given that I know nothing about your code, I'm confused why you have Camera within Display. While logically a display is displayed with a camera I would think your object structure, if you are making them interdependent would be inverted. Cameras [i]use [/i]displays to show their field of view Your binding of a camera to a display may also seems to negate the ability to have multiple monitor support. This is all just reading logically and trying to rationalize how your objects interrelate so take this with a grain of salt but you may definitely want to re-examine why you have those objects linked the way you do.
[/quote]
Well, as I said above, Display is a subsystem, rather than a display in the sense that you seem to interpret the name, which is understandable. It owns the Window object, the Renderer object, handles creation of the dialog boxes. It owned the camera object largely because I needed to put the camera [i]somewhere[/i]. I've put the camera in the World class, as owl mentioned that he did, and it does simplify a lot of code. I had to do a bit of fudgery since the Menu is essentially another World object, and also needed the camera. But I just realized that I could probably have a static camera in the base class....

I considered including multiple monitor support, but I don't have multiple monitors ;) Also, I don't think this would work well split across multiple screens:
[img]http://uploads.gamedev5.net/gallery/album_205/gallery_37843_205_82548.png[/img]

Share this post


Link to post
Share on other sites
Eh, who gives a f? It's only two-level deep. I prefer keeping my architecture clean than worrying about two getters.

As long as it's not getApplication().getScreens().getCurrentScreen().getDisplay().getRenderer().getWorld().getCamera(), you are fine.

Share this post


Link to post
Share on other sites
[quote name='alnite' timestamp='1306537853' post='4816605']
Eh, who gives a f? It's only two-level deep. I prefer keeping my architecture clean than worrying about two getters.

As long as it's not getApplication().getScreens().getCurrentScreen().getDisplay().getRenderer().getWorld().getCamera(), you are fine.
[/quote]
QFE

Share this post


Link to post
Share on other sites
[quote name='alnite' timestamp='1306537853' post='4816605']
Eh, who gives a f? It's only two-level deep. I prefer keeping my architecture clean than worrying about two getters.
[/quote]
I can respect that approach to a certain extent. Especially since it's code that likely no one will ever use but me. But it seemed that there should be a better way. I wasn't worriedd--hell, the code worked just fine. It just looked off to me, and I'm not the type to let something like that go when it's improvable.

And my architecture [i]is[/i] cleaner for it ;)

Share this post


Link to post
Share on other sites
Think about it from a testability point of view (you [b]are [/b]writing tests, right?); if you have
[code]void SomeClass::Update( float dt ) { Camera& camera = _app.GetDisplay().GetCamera(); // do stuff }[/code]

then to test this, you need to make sure you have an app object populated with a display populated with a camera.

whereas for
[code]void SomeClass::Update( float dt, Camera& camera )
{
// do stuff
}[/code]

All you need is a Camera object, which can be passed into the method.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this