3

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?

  • Writing calls that do the same as other, more complicated calls is often a good thing to improve readability of the caller code. In fact, the façade pattern consists largely of doing just that. – Kilian Foth Jun 1 '15 at 8:08
  • Some programmers design the interface to a class, or have it given by a team member, before implementing it. – QuentinUK Jun 1 '15 at 8:13
4

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.

4

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!

  • While I generally agree (and did upvote) I have minor quibbles with bullets 1 and 2. Most languages support varargs, so adding a parameter is possible (if awkward and not checked by the compiler). Changing the name String from "SendMail" to "YoMaildr" will be a mess, but it will also be a mess (though easier with IDE refactoring) if the function name is to be changed from sendMail() to yoMailDr(). Bullets 3, 4, and 6 are excellent. – user949300 Jun 1 '15 at 19:27
  • By "safely change the name" I mean you can search the sourcecode and find everywhere a method name is used and change it. But you cannot do that for a particular value of a string parameter, since it might not even be hardcoded. – JacquesB Jun 2 '15 at 6:06
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.

  • 3
    A string parameter is not a dependency. – JacquesB Jun 1 '15 at 17:26
  • True in that it's not a complex object which you would create an interface for, but the effect on the re-usability and flexibility of the function remains the same. – gb2d Jun 2 '15 at 8:18
  • Nope, these are totally different issues. Dependency injection increase flexibility by reducing coupling between classes. Overloading the behavior of a method through a parameter (rather than having separate methods for separate behavior) increases coupling and reduces flexibility, and makes the method into more of a black box. – JacquesB Jun 2 '15 at 8:26
1

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.

  • If this is what doTask is actually doing, then it is unnecessary, and you may as well just call actuallyDoTaskA directly. Assume that doTask is doing something like passing $task as a var to another function (e.g. a db query), or operating on it directly (e.g. return $task." something";) – Benubird Jun 2 '15 at 9:41
  • The short answer is: if doTask does make consistent usage of the arguments, or reduces code duplication, it's plausible to keep it. However, if you really have a dozen or so similar methods, you probably need to take a step back and re-look at the design of the class. – h.j.k. Jun 2 '15 at 9:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.