Does this approach to pass frequent method parameters implicitly seem evil?

Started by
10 comments, last by Squared'D 10 years, 4 months ago

(excuse any mistakes in the code - i have not actually tried this yet and because globals are evil, im probably unaware of how to use them correctly. Will try to fix any problems.)

Imagine the following case:

We have a pile of game entities, each having logic, and each often needing access to the scene object so they can interact with the surroundings.

We then have code to update all these different entities as in:


void Scene::update()
{
    runMachines(*this); //Lots of moving parts - may affect surroundings
    growGrass(*this); //Need to check other objects - grass may impale those who stand on it while its 
    growing
    glowLight(*this); //Heat the neighbors
    for each object do
    {
        for each otherobject do
        {
            checkCollisions(*this)
            doGravity(*this)
            sayHi(*this)
        }
    }
    etc.
}

As you can see, quite a lot of Scene in there. I realize that this is not much. But if we take the hypothetical situation where the number of such objects is 4 (since we dont want to combine everything into a single scene object for example) and the logic is a lot more complicated, this gets quite redundant. In most practical situations, scene could be considered a global. We dont do that, because two scenes should be able to exist, and making it a global would remove ALL passing of Scene around.

So, what if there was a compromise such that:

-Scene is still passed around as a parameter where it does not feel like mindless repetition of a fact the code should understand (im basically looking for a statement like "using Scene" here)

-It is still obvious where scene might be passed. If its a global, we have no control over where scene can be accessed.

-It does not limit the amount of scenes to 1.

So, i was thinking of a class that sets a scope-limited global, which can be set in a scope, and then accessed from whatever methods you call from that scope as it were a global.

Implementation:


template<typename T, int identifier=0>
class Using
{
public:
    Using(T* ptr) : prevImplicit(implicit), implicit(ptr)
    {
    }

    ~Using()
    {
        implicit=prevImplicit;
    }

    static T* get()
    {
        assert(implicit!=nullptr);
        return implicit;
    }

private:
    static T* implicit=nullptr;
    T* prevImplicit;
}

Usage:

Using the example, instead of passing Scene to each method, we set a temporary scope limited global:

Using<Scene> scene(*this);

And within the methods needing access to the scene we do:

Scene* scene = Using<Scene>::get();

and then use it to apply whatever logic needed. The scene thus does not have to be passed in as a parameter, it is now a less evil global! :DD

Now, to explain some details.

We might have a scope using Using to apply a scope limited global. But what if a method is called from this scope, which also uses Using to also set a global of type Scene?

We have two mechanisms to handle this case:

1) We store the previously set value in the temporary Using class. When we return back to the first scope from the subscope, the value of the global will be reset to what it was before. Please do note that there might be some corner cases related to threads and lambdas and those might need special approaches.

2) We have a second template parameter, identifier, which allows us to have more than one such "implicit parameter". This is more for cases where you want to use implicit parameters, but have two or more of the same type. If you are handling eg. an array of them, it does no longer make sense to try to pass them as implicit parameters. Enums can be created to have eg PRIMARY_SCENE and SECONDARY_SCENE. I cant come up with any better examples of when youd need this, but the important thing is that it is trivial to pass in multiple objects of the same type.

So, the example update method would look as follows:



void Scene::update()
{
    Using<Scene> scene(this);
    runMachines(); //Lots of moving parts - may affect surroundings
    growGrass(); //Need to check other objects - grass may impale those who stand on it while its
growing
    glowLight(); //Heat the neighbors
    for each object do
    {
        for each otherobject do
        {
            checkCollisions()
            doGravity()
            sayHi()
        }
    }
    etc.
}

Is there anything wrong with this approach? Please do remember that i wanted to try and find a compromise between being explicit about dependencies, while at the same time allowing this kind of gameplay code to be less strict, since it is likely to change more rapidly and such (which is why we use scripting languages for that kind of code - it does not need to be so strict)

In most cases this does not really help much, but im sure there are cases where such a construct might be useful.

o3o

Advertisement

It sounds a little bit like you're getting ahead of yourself and trying to design away a problem you might not even have yet. You've got a lot of "what if" scenarios that are causing you to come up with these clever code solutions, and while the Using<T> solution is certainly workable, you might not find that it's truly useful.

In your example your Scene::Update function is manually calling a lot of really specific functions (growGrass, glowLight, etc). In reality, you'll probably end up with a list of modules or something that the scene iterates through and updates, like:


void Scene::update()
{
    for (int i = 0; i < mModules.size(); i++) {
        mModules[i].update(this);   // mModules contains GrowGrassModule and GlowLightModule, etc
    }

    for (int i = 0; i < mObjects.size(); i++) {
        mObjects[i].update(this);
    }

    for (int i = 0; i < mObjects.size(); i++) {
        for (int j = i; j < mObjects.size(); j++) {
            checkCollisions(this, mObjects[i], mObjects[j]);
        }
    }
}

Passing the scene pointer into these functions isn't much of a hardship, I don't think. And if it is, I think just having a global pointer would probably be easier and infinitely more understandable than the Using<T> system you've devised. The Using object you've created is essentially a stack of Scenes, and when I look at it, it makes me think that you really need a stack of Scenes. If you aren't using a stack of scenes, then this added functionality Using<T> provides can be confusing.

Choosing between passing pointers or using global variables or using some kind of complicated templated stack of global pointers isn't going to significantly affect you engine's architecture, so I would go for whatever's simplest for your needs now. If you find that it's not working out for you, you'll be able to change how to convey the Scene pointer later without too much work. It'll be a refactor, for sure, but it won't be a huge, difficult one.

Is there anything wrong with this approach?

It's based on a global variable, and that's never a good idea. What may save you time now, will inevitably annoy the hell out of you later on (when you realise you need to decouple the last years worth of dev time from that global).

No, i dont actually have an use for this and am not even intending to implement it. Its just something i came up with to deal with a problem that would exist if i magnified my code by a factor of 10. The reason i posted this was to see if it can be really considered a global anymore, since it is now limited to scope (at least to some degree) and does not limit the object only to a single one.

A better example: Imagine you have a logger class. You can either:

A) Pass it everywhere. This allows you to give a subsystem its own logger, and pass that instead. However, its tedious to pass a logger to every method (lets assume you might need a logger in any method for debugging purposes and dont want to be forced to modify 10 methods to add the ability when you need it)

B) Make it a global. No passing. But if you wanted a different logger for a particular subsystem, you are forced to modify all the code within that subsystem to use a different logger global. ew.

C) Use a construct like the Using class. This allows having a global logger class at the beginning. But it also allows you to give a subsystem its own logger - simply by adding a single Using temporary to a single location of code (wherever you run the logic of this subsystem. Though i have to note, its not really the subsystem that owns the logger, you are just saying that whatever logging happens when i call this method, is processed using this particular logger) You may also not use a global, and pass the logger as a parameter, but only through the high level code, resorting to the Using construct when it no longer is practical to pass it as a Parameter (when you get to rapidly evolving low level code).

Its not generally very useful, but there definitely are rare use cases where it actually makes sense and might improve productivity.

o3o

For the logger example have a set of filters that messages get logged under and can be enabled or disabled for specific users/scenarios.

EDIT: Bonus points if you can enable and disable logs by changing a value in the debugger. "Oh, the sound effects system is messing up, I'll just enable the logging for that through the debugger while the game is running".

"Most people think, great God will come from the sky, take away everything, and make everybody feel high" - Bob Marley

So, what if there was a compromise such that:
-Scene is still passed around as a parameter where it does not feel like mindless repetition of a fact the code should understand (im basically looking for a statement like "using Scene" here)
-It is still obvious where scene might be passed. If its a global, we have no control over where scene can be accessed.
-It does not limit the amount of scenes to 1.


The 'code' doesn't understand anything; the whole point of parameters is to express to the person writing/reading/maintaining the code the dependencies and requirements of that function. This 'using' construct does not make it obvious where it is used any more than a global does; lets say in your example you have a function which is called which DOESN'T require the scene parameter, in conventional code this is clear by the lack of parameter passing, with the 'using' system this is unclear as to what is going on.

This whole thing smells of your (terrible) 'contexts' idea from a few months back and is no better.

So, what if there was a compromise such that:
-Scene is still passed around as a parameter where it does not feel like mindless repetition of a fact the code should understand (im basically looking for a statement like "using Scene" here)
-It is still obvious where scene might be passed. If its a global, we have no control over where scene can be accessed.
-It does not limit the amount of scenes to 1.


The 'code' doesn't understand anything; the whole point of parameters is to express to the person writing/reading/maintaining the code the dependencies and requirements of that function. This 'using' construct does not make it obvious where it is used any more than a global does; lets say in your example you have a function which is called which DOESN'T require the scene parameter, in conventional code this is clear by the lack of parameter passing, with the 'using' system this is unclear as to what is going on.

This whole thing smells of your (terrible) 'contexts' idea from a few months back and is no better.

I understand that this is worse than passing the objects explicitly. But some things are still kept globals, so there must be a reason for doing so (result in evil or not), and this would offer a compromise between globals and explicit passing as parameter.

It could be considered a method of limiting the use of globals to a single piece of code instead of the whole codebase, since the object can be passed as a parameter to some point, and then continue off as a scope limited global.

I still consider this better than the previous post (which IIRC included adding language features...?) since this works using existing C++ features and is a lot simpler and thus it can be tested in practise.

Maybe i will try to implement it and see whether it works (for a logging system) after i can render text in my project. As in be able to print random information from random places deep in the code hierarchy without having to modify each and every class, which is something that should be avoided since those classes have absolutely nothing to do with being able to log something.

I had already forgotten that the only use case where this actually makes any sense is as a logging system... I guess its kind of like exceptions, they are somewhat similiar.

Or perhaps i just shouldnt post when im tired, especially about topics i have not much practical experience of ._. Please forgive me D:

o3o


A) Pass it everywhere. This allows you to give a subsystem its own logger, and pass that instead. However, its tedious to pass a logger to every method (lets assume you might need a logger in any method for debugging purposes and dont want to be forced to modify 10 methods to add the ability when you need it)

If you really need it in all(most) methods of a class:

D) Pass a reference to the constructor and save it in a member variable.

Is there anything wrong with this approach? Please do remember that i wanted to try and find a compromise between being explicit about dependencies, while at the same time allowing this kind of gameplay code to be less strict, since it is likely to change more rapidly and such (which is why we use scripting languages for that kind of code - it does not need to be so strict)

In most cases this does not really help much, but im sure there are cases where such a construct might be useful.


The construct is pretty evil as others have pointed out. And the final sentence should have told you that you probably need to rethink the issue and come up with a more suitable reason for wanting this abstraction. I'll provide a reason and an example of how I solved such a problem simply as an alternative solution. My reasoning for needing a transformation such as this was that I was configuring the main loop from script and some things could be C++ and others could be script (lua specifically) experiments. C++ tended to be stabilized code which I could use more common argument passing, construction references etc, but until I moved everything out of script I pretty much needed the update to be "void update( void )" since it was stored in a vector to be configured at startup. I figure this is a valid and concrete reason for needing to remove parameters from the update calls, yet I still wanted the C++ to take parameters.

In steps C++11 and the new shiny built in adapter pattern implementation. A combination of std::function, std::bind and lamba's can pretty much implement any form of adapter you can ever imagine. Unfortunately your example is too simplistic in order to show a decent usage, so I'll make a simple example based on a kinda real case:


// Assumption.  I have experimental items implemented in lua.
// Problem.  I can't pass the luaState as a parameter since the experiments
//   are usually in unique instances of luaState.
// How to pass the appropriate luaState to a generic "update" function?

// Update loop consists of:
typedef std::function< void ( void ) >  Updater_t;
typedef std::vector< Updater_t >        UpdateVector_t;

UpdateVector_t  gUpdates;  // Evil, horrible global update list.  Just for example.


LuaState  gExperiment1( "Experimental1.lua" );
LuaState  gExperiment2( "Experimental2.lua" );

// In order to update experiment 1, I need to call:
//  gExperimental.Call( gExperimental1.Ref( "Update" ) );
// Getting a ref is kinda expensive so best not to call it every time...
// Using bind as an adapter:
gUpdates.push_back(
 std::bind( &LuaState::Call, std::ref( gExperimental1 ), gExperimental1.Ref( "Update" ) ) );
gUpdates.push_back(
 std::bind( &LuaState::Call, std::ref( gExperimental2 ), gExperimental2.Ref( "Update" ) ) );

// Run the updates.
for( Update_t& update : gUpdates )
  update();
If you are not familiar with std::bind, I highly suggest you get familiar. It is one of the best ways to decouple code and implement function level adapter patterns you are likely to find.

Anyway, hopefully this is appropriate to your actual goals or at least provides another alternative which avoids that construct which is just hiding a global in fancy templates.


A) Pass it everywhere. This allows you to give a subsystem its own logger, and pass that instead. However, its tedious to pass a logger to every method (lets assume you might need a logger in any method for debugging purposes and dont want to be forced to modify 10 methods to add the ability when you need it)

If you really need it in all(most) methods of a class:

D) Pass a reference to the constructor and save it in a member variable.

The problem with this one is that its a waste of memory. Although it might be a good solution in real code, as i said i dont have a real problem requiring an urgent solution here so i would rather avoid such things. Just thought it was interesting since i havent seen anything similiar before.

I would also avoid std::function, since it was horrible for performance when i used it, however since i was passing lambdas i could make the type a template parameter instead of using std::function, which is all ive ever needed.

It seems this is an evil construct, but if you compare it to for example singletons, it seems more flexible and potentially less evil if used correctly.

o3o

This topic is closed to new replies.

Advertisement