Logging and getting the response in a controller

Question

I have an action that does two things:

1. It sends a chain of commands, and gets the response from an external API that we call wrapper.
2. It logs the chain of commands and the tunnel (look at tunnels as different machines) the command was sent to.

My problem is the contents of the if statement. The two things (sending the command/getting the response, and logging the exchange) are happening in that if block, and they both need the chain and tunnel variables. I'd ideally want to have two functions, getResponseFromWrapper() and logCommandAndResponse(), effectively reducing the number of statements inside my if block to just two.

public function indexAction(Request $request)
{
    $response = '';
    $form = $this->createForm(new TerminalType());
    $form->handleRequest($request);

    if ($form->isValid()) {
        $data = $form->getData();
        $tunnel = $data['tunnel'];
        $chain = $data['chain'];

        $response = $this->retrieveResponseFromWrapper($chain, $tunnel);
        $this->get('command_logger')->log($chain, $tunnel, $response);
    }

    return $this->render('terminal/index.html.twig', [
        'title' => 'Terminal',
        'form' => $form->createView(),
        'response' => $response
    ]);
}

In other words, I feel like the three first lines in the if block are making it a bit more difficult for the reader to know what's happening.

Better code might be:

public function indexAction(Request $request)
{
    $response = '';
    $form = $this->createForm(new TerminalType());
    $form->handleRequest($request);

    if ($form->isValid()) {
        list($chain, $tunnel) = $this->extractChainAndTunnelFromForm($form);
        $response = $this->retrieveResponseFromWrapper($chain, $tunnel);
        $this->get('command_logger')->log($chain, $tunnel, $response);
    }

    return $this->render('terminal/index.html.twig', [
        'title' => 'Terminal',
        'form' => $form->createView(),
        'response' => $response
    ]);
}

This is much shorter. However, returning two values from a function leaves a bad taste in my mouth.

How can I improve the first code block and reduce the number of statements in the if block? Do I need to create something like a DTO (i.e. have my extractCommandAndTunnelFromForm() create a plain old PHP object and set the tunnel and chain as its properties)? Do I need to inject the commandLogger service into the service that sends commands to the wrapper, and remove logging from the controller entirely?

Answer 1

First of all, the first code is perfectly fine in matter of conciseness and readability. You're being a lil bit obsessive, but it's not perfect in matter of design.

Second, if I really had to do so to just improve the readability, I would refactor retrieveResponseFromWrapper() to accept a, let's say, WrapperRequest instance. On the other hand, I would refactor extractChainAndTunnelFromForm() to a createWrapperRequestFromForm() which obviously returns a WrapperRequest instance after its own internal processing of form data:

if ($form->isValid()) {
    $request  = $this->createWrapperRequestFromForm($form);
    $response = $this->retrieveResponseFromWrapper($request);

    // If you wanna go shorter for the sake of sanity:  
    //
    // $response = $this->retrieveResponseFromWrapper(
    //     $request = $this->createWrapperRequestFromForm($form)
    // );

    // I also recommend to move this out, controllers just log access things
    $this->get('command_logger')
         ->log($request->getChain(), $request->getTunnel(), $response);
}

Now that I wrote it, you see, it's shorter and more unreadable! IMO shorter code and minimal number of methods are not necessarily an indication of more concise and readable code. The number of statements in a if block is not an effective factor to measure readability.

I remember when I was trying to find one-liners for every damn single thing I wrote. I went insane and even swapped ifs with equivalent AND/OR statements. It was of course a pleasure but I've learnt to embrace simplicity and verbosity in order to be more readable for others.

Back to the design thing; The above code is a better code, not because it's shorter or it has just one return value, but because it separates your domain logic apart from the controller and it avoids implementation leaks. This controller does not know about $tunnel and $chain, it just transports them as blackbox data. That's why we often need Data Validation components separated from the controller and the Form Validation component.

I think that would be a better idea to refactor our controllers as just a Transport Layer around HTTP instead of such minor improvements. To pursue this I always think that my app needs be fully functional when the data is fed through terminal instead of HTTP, without regular forms.

To conclude; A reader must always tend to read the code in Service/Repo/Model/... classes in order to understand the business logic. Reading the controller code of a well-designed application must be boring cause it's just the means not the thing!