Function wrappers with no args: bad practice?

Question

My colleague likes to write classes containing methods looking like this:

public function doTaskA()
{
    return $this->doTask('A');
}
public function doTaskB()
{
    return $this->doTask('B');
}
private function doTask($task)
{
    ...

I say this is bad practice, and would prefer to just make the doTask method public. I think if you really feel it is necessary to hardcode the possible values of $task in the class, then just make them class constants and call doTask(self::TASK_A) rather than writing a new function purely as a wrapper.

The worst case I saw of this, was a class with more than a dozen of these wrappers, and the doTask method was barely twenty lines long.

So, is this actually bad practice? Or is it just a matter of style? Or am I wrong, and it is actually considered good practice, as he insists?

Answer 1

It depends on scenarios.

Multiple Identical Calls

Consider having do to a particular task a number of times throughout your code. Doing foo.doTask(args) might get tedious. Having to provide the arguments each time, can also be error prone. Having a wrapper which wraps it up for you can be convenient and also less error prone. Also, imagine if you need to change the value of one of the arguments in that function, you will have to go through all the calls and make multiple changes. Using the wrapper approach will mean that the change will be localised.

Mutliple Slightly Different calls

On the other end of the spectrum, having a lot of slightly different wrappers, one for each possible type, can yield too many wrappers which eventually just confuse the user of your API. Changing the code might be confusing since you might not be quite so sure about what you want to change.

In either case though, wrappers should be useful if you need to provide controlled access to your methods, so that is another thing you would need to keep in mind.

Answer 2

This is not a bad practice. Overloading a single method to do different things depending on parameters is actually an antipattern. If different strings leads to different behavior it is good design to created distinct methods.

Eg.

x.deleteFile()
x.sendMail()

is a much better interface than

x.doTask("DeleteFile")
x.doTask("SendMail")

even if they internally call the same private method. This fact is an implementation detail which should be encapsulated.

Consider the downsides of the second solution:

• you cannot add parameters to one of the tasks in the future.
• you cannot safely change the name of a task
• you cannot inspect the code to see where the "SendMail" functionality is used
• spelling errors will not be detected by the compiler but will cause the program to fail at runtime.
• you are are creating a 'god-method' which is an antipattern because it breaks the single-responsibility principle and high-cohesion/low-coupling principle.
• you cannot easily see what tasks the "god-method" supports.
• you cannot segregate the interface as per the SOLID principles, which means you cannot override or supply an alternative implementation of "DeleteFile" without also supplying an alternative implementation of "SendMail".

So you should definitely listen to your colleague and consider his viewpoints carefully!

Answer 3

I wouldn't say this is bad practice as a general rule, but it does lean away from flexible programming practices such as use of dependency injection.

If the 'dependencies' of a class or function can be passed in as arguments, it makes the class / function more re-useable, easier to refactor, and less of a black box to the programmer who has to use the class. I've found that adhering to this style throughout a project leads to flexible and maintainable code.

Never say never or always in programming, but I think you can make a reasonable argument for steering colleagues away from this style of coding.

npinti's argument also holds. If you find yourself calling the same function with the same parameters over and over, and an IoC container (see dependency injection above) can't do this work for you, then you may want to wrap it with a dedicated function, though the name should clearly indicate that it is a variation on the more general function which accepts parameters.

Answer 4

The worst case I saw of this, was a class with more than a dozen of these wrappers, and the doTask method was barely twenty lines long.

I think this needs better addressing than the issue of using function wrappers, which as pointed out by the others answers tend to have more advantages than disadvantages.

Assuming you need minimally one line per method argument in doTask($task), something like below:

// PHP I think?
private function doTask($task)
{
    // ...
    if ($task === 'A') actuallyDoTaskA();
    if ($task === 'B') actuallyDoTaskB();
    // ...
}

doTask() now becomes more of a hindrance since it is simply delegating calls to other methods, which should have been called by doTaskA()/doTaskB() in the first place.

In conclusion, such function wrappers can prove beneficial if you want to make the arguments used across your codebase consistent, or if it is used in a manner that reduces code duplication (e.g. different value substitutions for string formatting by callers). However, if the underlying method being called becomes another 'function wrapper' in itself, then you should really consider removing one level... preferably the underlying method.