4

Example: I have a method that formats the output based on a parameter from an object. The question is, should I pass the object that holds the parameter to the method, or just the required param?

Still trying to grasp clean coding so I might be wrong, my take is that the 2nd approach is cleaner because it doesn't require you to pass the entire object, and is easy to test.
However, it makes calling the method ugly now since I have to check for the param every time when I call the method. And its used very very often.

In contrast, the first approach while not that easy to test, because I have to mock the requestObject, is much easier to use.

So which method should I go with? Or is there an even better approach?

// The output method with object as input
public function outputResponse($message, $requestObject) {
    if (isset($resquestObject->param['response_type']) && $requestObject->param['response_type'] == 'json') return json_ecode($message);
    return xmlOutput($message);
}

// So you call it like
$this->outputResponse($message, $requestObject);

The other approach...

// The output method with just the param as input
public function outputResponse($message, $responseType) {
    if (responseType == 'json') return json_ecode($message);
    return xmlOutput($message);
}

// So you call it like
$responseType = (isset($resquestObject->param['response_type']) && $requestObject->param['response_type'] == 'json') ? 'json' : '';
$this->outputResponse($message, $responseType);

2 Answers 2

1

When you're making decisions like this it's best to weigh the pros and cons of the specific case. In this case, since your example is a one line convenience function, use the most convenient way possible. There's no reason to make a convenience function anything other than a super simple and easy to use interface. As for testing, you don't even need to test it: all that it is in and of itself is a conditional, your test can be against the $requestObject. It's not neccessary to test a single conditional usually, the more important thing is if $requestObject is set up correctly.

I'd also like to add that unless you see this same code repeated 3 times or more you might not even need to call out to your method, you might just do it inline. I'm assuming since you've written it, it's something that happens a lot.

2
  • 2
    Yes you're right its used very often. What you say makes sense, in fact that's my first implementation. But after a day, the guilt of clean code creep in although as you said, its really a super simple function. And I tend to face this kind of decision very often. Hence some tips from the pros is much appreciated. As of now I'll stick with the first method.
    – resting
    Commented Jan 30, 2013 at 2:30
  • @resting Keep asking, your question is perfectly reasonable and I hope I was able to help you and anyone else facing this kind of a decision. Commented Jan 30, 2013 at 2:33
3

You are correct that $requestObject doesn't seem like it belongs as a parameter to outputResponse. outputResponse is supposed to be outputting response, not interpreting the meaning of request parameters. However, your second version is still doing that:

public function outputResponse($message, $responseType) {
    if (responseType == 'json') return json_ecode($message);
    return xmlOutput($message);
}

See, it defaults to xml. But it should not. The fact that your requests default to XML is part of request parameter processing, so you've not entirely removed it from the function. It should really be:

public function outputResponse($message, $responseType) {
    switch($responseType) {
         case 'json':
               return json_encode($message);
         case 'xml':
               return json_encode($message);
         default:
               throw new BadInputError($responseType);
}

On the other hand, you really shouldn't be duplicating the logic to decide the responseType all over the place either. At the version least, lets define a function for it:

public function determineRequestType($requestObject)
{
    if( isset($requestObject->param['response_type']) && $requestObject->param['response_type'] == 'json' )
         return 'json';
    else
         return 'xml';
}

That gives you two fairly easily testable functions. You can test each task (figure out the output format, produce the output format independently. This leaves something like the following all over the place. It's a big improvement, but still more duplication than we want.

$this->outputResponse($message, determineRequestType($requestObject));

We could write a function:

function produceResponse($message, $requestObject) {
     return $this->outputResponse($message, determineRequestType($requestObject));
}

Which gives you the exact same interface you had before, but has the components broken up for easier testing. However, you are still repeating calls to produceResponse all over the place and that's suspect. So I'd look at doing something different.

I'm going to guess that what you've got is something like this many times over:

function handleFoobarRequest($requestObject)
{
     // pull parameters from requestObject
     // do whatever the request actually requests
     $message = // message constructed from request results
     $response = produceResponse($requestObject);
     write_response($response);
}

What you should really do is rearrange the code so that its more like this:

function handleFoobarRequest($requestObject)
{
     // pull parameters from requestObject
     // do whatever the request actually requests
     $message = // message constructed from request results
     return $message;
}

And whoever calls this function takes care of formatting it. That way there should only be one place that actually needs to make that call. Without knowing more about how your app is structured I don't know how exactly to do that in your case.

4
  • +1 because your answer obviously has more effort put into it than mine. Commented Jan 30, 2013 at 2:45
  • Interesting. Your guess is fairly correct. I'm using Cakephp, its not exactly the same, but works roughly as what you assumed. handleFoobarRequest is the controller action, and $requestObject is the request. To output the response is simply return produceResponse($requestObject). Based on your last statement, there's no one calling this function. Its triggered when the url is accessed. So I don't think that would work.
    – resting
    Commented Jan 30, 2013 at 4:01
  • @resting, I don't know CakePHP, but I wonder if there isn't a way to hook into an api that would let you override the process. Commented Jan 30, 2013 at 4:27
  • @WinstonEwert Ah...there actually is. I'll give that a thought too. :)
    – resting
    Commented Jan 30, 2013 at 4:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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