About clean code

Question

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);

Answer 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.

Answer 2

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.