Sign in to follow this  
Eddycharly

code factorization problem

Recommended Posts

I'm refactoring some code at work, and i face a problem that could hopefully be solved using templates. I have an adapter class with a bunch of methods that almost do the same thing. Every method do some checks (always the same), then if possible, the method instantiates an object (an action class), and calls the perform method on it(the perform method can have one or zero argument), then if the perform method succeeded, it eventually assigns the result to an out parameter (the method can be void, and in this case, it doesn't have to affect anything). It makes a lot of code duplication, and as all the methods have many things in common, i thought about making a template method to regroup all the common code. The problem is that the signature of the method is not always the same of course. It's not the same if i have an argument or not, idem for the out parameter. It makes four cases : - one argument, one out parameter - zero argument, one out parameter - one argument, zero out parameter - zero argument, zero out parameter I can write four template methods, but again, it introduces code duplication, i'll write almost the same code four time. Something like this :
class Adapter
{
	void aMethod1 ()
	{
		handleMethod<Action1> ();
	}

	void aMethod2 (int arg)
	{
		handleMethod<Action2> (arg);
	}

	void aMethod3 (bool & ret)
	{
		handleMethod<Action3> (ret);
	}

	void aMethod4 (int arg, bool & ret)
	{
		handleMethod<Action4> (arg, ret);
	}

	template<typename ACTION>
	void handleMethod ()
	{
		// do the checks

		// if all checks ok instantiate action
		ACTION action;

		// perform action (no arg)
		action.perform ();
	}

	template<typename ACTION, typename ARG>
	void handleMethod (ARG arg)
	{
		// do the checks

		// if all checks ok instantiate action
		ACTION action;

		// perform action (one arg)
		action.perform (arg);
	}

	template<typename ACTION, typename RET>
	void handleMethod (RET ret)
	{
		// do the checks

		// if all checks ok instantiate action
		ACTION action;

		// perform action (no arg)
		action.perform ();

		// assign out parameter
		ret = action.result ();
	}

	template<typename ACTION, typename ARG, typename RET>
	void handleMethod (ARG arg, RET ret)
	{
		// do the checks

		// if all checks ok instantiate action
		ACTION action;

		// perform action (one arg)
		action.perform (arg);

		// assign out parameter
		ret = action.result ();
	}
};
So now my question is, is there a way to do that with only one template method instead of four ? Or is there a better solution to factorize code in this situation ? All suggestions are welcome. Thanks.

Share this post


Link to post
Share on other sites
If you need all those four different handle method signatures, you'll need four methods.

You might not need to do that. It looks like you have two optional arguments, you could check out boost::optional if you want to make that explicit. This does introduce a third party lib to consumers and change your signatures. Alternately, consider returning your return value, which would reduce you to two signatures without much fuss.

Lastly, consider making a single "Params" struct, passed in by reference. That would reduce an arbitrary number of signatures like this down to one.






Share this post


Link to post
Share on other sites
Unfortunately i can't return my value directly because the methods already return something (it wasn't clear in my code exemple).

About making a single struct "Param", how would you code that ?
I mean, every method takes a diferent argument type (or no argument), it would be nice if template virtual methods existed but they don't ;-)

Gonna check boost::optional.
Thanks.

Share this post


Link to post
Share on other sites
Quote:
Original post by Eddycharly
About making a single struct "Param", how would you code that ?
I mean, every method takes a diferent argument type (or no argument), it would be nice if template virtual methods existed but they don't ;-)


I don't see where there's anything 'virtual' in your code. Are you trying to turn generic functions into objects of some kind, and call them without having to worry about their signatures? Then (a) consider how the client code is going to know what to pass to the functions; and (b) consider if what you really want is to just use boost::function ;)

Share this post


Link to post
Share on other sites
I'm not sure if boost::function can help me here.

The client knows what to pass to the function. In my case, the client is any method of the adapter, it has a well known signature and knows the argument and return types, so that's not a problem.

What i'm trying to do is to make a templated implementation to be used internally by all the methods because all the methods do essentially the same thing.

The templated method must do these things :
- do some checks and return an error code if a check fails (not a problem, the checks are always the same)
- if all checks ok, instantiate an action object (not a problem, the type of the action object is an explicit parameter of the templated method)
- call the perform method on the object previously instantiated (here there's a problem because the signature is not always the same, sometimes it takes an argument, sometimes no)
- assign the result of the action to an out parameter (here there's a problem too, because not all actions return something)


In order to prevent code duplication, there must be only one templated method.

One possible way to do it is probably to introduce two more template arguments : the method INVOKER and the result ASSIGNER, and to let the caller specify the one he wants to use.
The down side is that it will have to create two temporary objects to store the argument value and the refence to the out parameter.

Something like :

template<typename ACTION, typename INVOKER, typename ASSIGNER>
void handleMethod (INVOKER invoker, ASSIGNER assigner)
{
// do the checks

// if all checks ok instantiate action
ACTION action;

// perform action (invoker takes care of invoking perform on action with the correct arguments)
invoker.invoke (action);

// assign out parameter (assigner takes care of assigning action's result to the out parameter if needed)
assigner.assign (action);
}


Like this i have one templated method.
I just need a VoidInvoker, an ArgInvoker, a VoidAssigner and a RetAssigner classes so that the caller can use that to instantiate the templated method.

For exemple :

void aMethod1 ()
{
handleMethod<Action1> (VoidInvoker (), VoidAssigner ());
}

void aMethod2 (int arg)
{
handleMethod<Action2> (ArgInvoker (arg), VoidAssigner ());
}

void aMethod3 (bool & ret)
{
handleMethod<Action3> (VoidInvoker (), RetAssigner (ret));
}

void aMethod4 (int arg, bool & ret)
{
handleMethod<Action4> (ArgInvoker (arg), RetAssigner (ret));
}


Anyway, i don't like it at all, and i would like to find something simpler.
It seems to be over complicated.

Share this post


Link to post
Share on other sites
I assume the only reason your Adapter class is a class at all rather than a namespace is because it's used with some other templated code.

You're also mixing up terminology slightly; C++ doesn't have "methods", leave that for Java and C#, instead C++ has "member functions". [smile]

It seems to me that you don't need templates:


class Adapter
{
public:
void func1 ()
{
if (!checks()) return;
Action1().perform();
}

void func2 (int arg)
{
if (!checks()) return;
Action2().perform(arg);
}

void func3 (bool & ret)
{
if (!checks()) return;
Action3 a;
a.perform();
ret = a.result();
}

void func4 (int arg, bool & ret)
{
if (!checks()) return;
Action4 a;
a.perform(arg);
ret = a.result();
}

private:
bool checks();
};

I would then accept the remaining commonalities within those functions.
I imagine a better refactoring does exist but it probably requires changes at a wider scope than just this class.

Share this post


Link to post
Share on other sites
The adapter is a class because it also implements the ActionVisitorInterface.
It's because some actions are asynchronous, but that's another story (that's why i have a perform method and a result method, to be able to switch between synchronous and asynchronous actions easily).

Finally, i think i will go for your suggestion, that's what i did in the first place.
That's a bit more typing as i have a lot of methods (ooops, member functions), and the cpp file grows quickly, but it's easyer to read.
And if someone adds a new member functions in the adapter and forgets to do the checks for exemple, then, i'll kill him ;-)

Thanks.

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