Informazioni su codice pulito

Quando prendi decisioni come questa, è meglio valutare i pro e i contro del caso specifico. In questo caso, dal momento che il tuo esempio è una funzione di comodità su una sola riga, usa il modo più conveniente possibile. Non c'è motivo di fare una funzione di comodità diversa da un'interfaccia semplicissima e facile da usare. Per quanto riguarda i test, non è nemmeno necessario testarlo: tutto ciò che è di per sé è un condizionale, il test può essere contro $ requestObject. Non è necessario testare un singolo condizionale di solito, la cosa più importante è se $ requestObject è impostato correttamente.

Vorrei anche aggiungere che se non vedi lo stesso codice ripetuto 3 volte o più potresti non aver nemmeno bisogno di chiamare il tuo metodo, potresti semplicemente farlo in linea. Presumo da quando lo hai scritto, è qualcosa che accade molto.

Hai ragione che $requestObject non sembra appartenere come parametro a outputResponse . Si suppone che outputResponse stia emettendo la risposta, non interpretando il significato dei parametri della richiesta. Tuttavia, la tua seconda versione continua a farlo:

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

Vedi, il valore predefinito è xml. Ma non dovrebbe. Il fatto che le tue richieste siano predefinite in XML è parte dell'elaborazione dei parametri di richiesta, quindi non l'hai completamente rimosso dalla funzione. Dovrebbe essere davvero:

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

D'altra parte, non dovresti davvero duplicare la logica per decidere la rispostaTipo dappertutto. Alla versione minima, definiamo una funzione per questo:

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

Questo ti dà due funzioni facilmente controllabili. Puoi testare ciascuna attività (capire il formato di output, produrre il formato di output in modo indipendente, lasciando qualcosa di simile a ciò che segue dappertutto: è un grande miglioramento, ma ancora più duplicazione di quello che vogliamo.

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

Potremmo scrivere una funzione:

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

Che ti dà la stessa identica interfaccia che avevi prima, ma ha i componenti suddivisi per test più semplici. Tuttavia, stai ancora ripetendo le chiamate a produceResponse dappertutto e questo è sospetto. Quindi guarderei fare qualcosa di diverso.

Ho intenzione di indovinare che quello che hai è più o meno così:

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

Quello che dovresti fare è ridisporre il codice in modo che sia più simile a questo:

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

E chi chiama questa funzione si occupa di formattarlo. In questo modo dovrebbe esserci solo un posto che ha effettivamente bisogno di fare quella chiamata. Senza sapere di più su come è strutturata la tua app, non so esattamente come farlo nel tuo caso.