Extract, Extract, Extract


In this video and the next we are going to cover a couple of potential solutions to a problem I see infrequently - but enough - when working with client code.

This is the big old if / elseif / elseif / elseif / else.

The solution we will cover in this video is not going to win any awards for clean code, by any means. Instead, we will take a look at one way to extract the problem code to its own class, calling that class as a Symfony service, and being content with the result.

In actuality, this solution may very well be good enough. It is simple, and it works.

The solution in the next video will be a lot more modular, but at the expense of simplicity.

The Problem Code

In our world, the problem we need to solve is to allow an end user to convert our hardcoded PHP array into some other formats.

To do this, a user could browse to our site at, for example:

http://127.0.0.1:8000/

and be shown our array as JSON.

They may also specify the format to convert to as a URL parameter:

http://127.0.0.1:8000/?format=csv

If they decide to send in a format we don't support then we blow up. But that's their fault.

Of course, this is an example app, don't do things like that.

<?php

// /src/AppBundle/Controller/DefaultController.php

namespace AppBundle\Controller;

use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\HttpFoundation\Request;

class DefaultController extends Controller
{
    /**
     * @Route("/", name="homepage")
     */
    public function indexAction(Request $request)
    {
        $data = [
            'abc'   => 123,
            'hello' => 'world',
            'qwe'   => [
                'key' => 'value'
            ],
            'xyz'   => 'alphabet'
        ];

        $format = $request->query->get('format', 'json');

        if ($format === 'json') {
            $converter = json_encode($data);
        } else ($format === 'csv') {
            $converter = $this->array_2_csv($data);
        }

        $output = $converter->convert($data);

        return $this->render('default/index.html.twig', [
            'output' => $output
        ]);
    }

    private function array_2_csv($array) {
        $csv = array();
        foreach ($array as $item=>$val) {
            if (is_array($val)) {
                $csv[] = $this->array_2_csv($val);
                $csv[] = "\n";
            } else {
                $csv[] = $val;
            }
        }
        return implode(';', $csv);
    }
}

Now, there's a bunch of things wrong here that we need to address.

Firstly, this is a controller class but we've spilled our domain logic (json_encode, array_2_csv) in here.

In this example - as things often start out - the entire logic is viewable in one place. It kinda makes sense - why make things more complicated than they need be?

Well, in this case - as in so many others - the one inescapable fact is that our business rules will change. We might need to add in another conversion option - to yaml perhaps.

Now we need to add in an elseif, along with - probably - tie this controller class directly to Symfony's YAML component. Not great.

As a real world note here, this is usually the number one reason for controller bloat. I remember when I would proudly boast to a mentor of mine that I had files with 2500 lines of code in there, like I was winning the league. Symfony best practices advocate a 5-10-20 rule, and I agree that it's good advice.

As a rule of thumb, you should follow the 5-10-20 rule, where controllers should only define 5 variables or less, contain 10 actions or less and include 20 lines of code or less in each action.

For the lazy.

Extracting To Classes

We start feeling pain with this code when we want to re-use the converters. Imagine your boss tells you that we now need to offer the same service, but via a Symfony console command. If you feel hesitant copy / pasting the if / else code from the controller to your console command then congratulations, you are on the path to becoming a better developer.

The first thing we can do to tidy up this code is to extract the implementations to separate classes:

<?php

// /src/AppBundle/Converter/ConvertToJson.php

namespace AppBundle\Converter;

class ConvertToJson
{
    public function convert(array $data)
    {
        return json_encode($data);
    }
}

and

<?php

// /src/AppBundle/Converter/ConvertToCsv.php

namespace AppBundle\Converter;

class ConvertToCsv
{
    public function convert(array $data)
    {
        return $this->array_2_csv($data);
    }

    private function array_2_csv($array) {
        $csv = array();
        foreach ($array as $item=>$val) {
            if (is_array($val)) {
                $csv[] = $this->array_2_csv($val);
                $csv[] = "\n";
            } else {
                $csv[] = $val;
            }
        }
        return implode(';', $csv);
    }
}

Notice how both ConvertToJson and ConvertToCsv have the same method signature for convert:

public function convert(array $data)

This should set off lightbulbs that you have different conversion strategies going on here, and could therefore standardise to an interface:

<?php

// /src/AppBundle/Converter/ConverterInterface.php

namespace AppBundle\Converter;

interface ConverterInterface
{
    public function convert(array $data);
}

Wherever possible, try and code to an interface rather than to a specific implementation. It will make your life easier.

Ok, so we also need to update our two ConvertToX classes to implement this new interface:

<?php

// /src/AppBundle/Converter/ConvertToJson.php

namespace AppBundle\Converter;

class ConvertToJson implements ConverterInterface

and

<?php

// /src/AppBundle/Converter/ConvertToCsv.php

namespace AppBundle\Converter;

class ConvertToCsv implements ConverterInterface

No need for an extra use statement here as ConverterInterface lives inside the same namespace.

Updating The Controller Code

Now that we've extracted all our conversion logic, we can go ahead and begin an initial tidy up of the controller code itself:

<?php

namespace AppBundle\Controller;

use AppBundle\Converter\ConvertToCsv;
use AppBundle\Converter\ConvertToJson;
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\HttpFoundation\Request;

class DefaultController extends Controller
{
    /**
     * @Route("/", name="homepage")
     */
    public function indexAction(Request $request)
    {
        $data = [
            'abc'   => 123,
            'hello' => 'world',
            'qwe'   => [
                'key' => 'value'
            ],
            'xyz'   => 'alphabet'
        ];

        $format = $request->query->get('format', 'json');

        if ($format === 'json') {
            $converter = new ConvertToJson();
        } else ($format === 'csv') {
            $converter = new ConvertToCsv();
        }

        $output = $converter->convert($data);

        // replace this example code with whatever you need
        return $this->render('default/index.html.twig', [
            'output' => $output
        ]);
    }
}

Our controller is still directly tied to the two Converter classes, but at least we've encapsulated their implementations.

If we knew for sure that we'd never need to add another implementation of Converter ever again then this would likely be good enough. No point over-engineering for the sake of it.

But of course, later that very same week, the Sales team tell a potential client that you know what? Of course we can convert to YAML. And XML. In walks your boss, and suddenly you've got a couple of tickets to start working on. Boo.

Fast Forward One Sprint

Over the past two weeks you and your team of 5 have discussed, debated, and dipped out into Starbucks in order to flesh out the implementations of the ConvertToYaml and ConvertToXml classes. Top tip: always expense expensive coffee consumption, if at all possible :)

Anyway, the following implementation is agreed upon:

<?php

// /src/AppBundle/Controller/DefaultController.php

namespace AppBundle\Controller;

use AppBundle\Converter\ConvertToCsv;
use AppBundle\Converter\ConvertToJson;
use AppBundle\Converter\ConvertToYaml;
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\HttpFoundation\Request;

class DefaultController extends Controller
{
    /**
     * @Route("/", name="homepage")
     */
    public function indexAction(Request $request)
    {
        $data = [
            'abc'   => 123,
            'hello' => 'world',
            'qwe'   => [
                'key' => 'value'
            ],
            'xyz'   => 'alphabet'
        ];

        $format = $request->query->get('format', 'json');

        if ($format === 'json') {
            $converter = new ConvertToJson();
        } elseif ($format === 'csv') {
            $converter = new ConvertToCsv();
        } elseif ($format === 'yaml') {
            $converter = new ConvertToYaml();
        } elseif ($format === 'xml') {
            $converter = $this->get('crv.converter.convert_to_xml');
        }

        $output = $converter->convert($data);

        // replace this example code with whatever you need
        return $this->render('default/index.html.twig', [
            'output' => $output
        ]);
    }
}

It turns out that converting to YAML was really easy. Symfony have a YAML component which has a static method called dump that takes an array and returns, as you might expect, some YAML.

Following the lead from the two previous implementations, we've crafted this masterpiece:

<?php

// /src/AppBundle/Converter/ConvertToYaml.php

namespace AppBundle\Converter;

use Symfony\Component\Yaml\Yaml;

class ConvertToYaml implements ConverterInterface
{
    public function convert(array $data)
    {
        return Yaml::dump($data);
    }
}

Where things got more interesting was in dealing with XML. Don't things always get 'interesting' when XML is involved?

In order to convert from XML we needed to bring in another Symfony component - the Serializer.

However, unlike with the ConvertToYAML::convert implementation, we couldn't just call a static method this time. Instead, we needed a way to inject the Serializer, and as such we've ended up with a Symfony service. If you are at all unfamiliar with Symfony's services I would point you in the direction of this short tutorial series.

Now, we could have grabbed the Serializer service from the container and passed that in when instantiating a new ConvertToXml(). But the nicer way to do this is to declare the class as its own service and let Symfony handle injecting the dependency. Then, all we need to do is grab the service which will be pre-configured.

For reference the service definition might look like:

# /app/config/services.yml

services:
    crv.converter.convert_to_xml:
        class: AppBundle\Converter\ConvertToXml
        arguments:
            - "@serializer"

Extracting The if / else

The next way to tidy up this code is to extract the big if / elseif / else conditional from our controller entirely.

We can take this logic and wrap it in another Symfony service. This doesn't resolve the tight coupling problem thoroughly, it merely hides it from the controller.

We start by defining a new class called Conversion:

<?php

// /src/AppBundle/Service/Conversion.php

namespace AppBundle\Service;

use AppBundle\Converter\ConvertToCsv;
use AppBundle\Converter\ConvertToJson;
use AppBundle\Converter\ConvertToObject;
use AppBundle\Converter\ConvertToXml;
use AppBundle\Converter\ConvertToYaml;
use Monolog\Logger;

class Conversion
{
    /**
     * @var Logger
     */
    private $logger;
    /**
     * @var ConvertToXml
     */
    private $convertToXml;

    public function __construct(
        Logger $logger,
        ConvertToXml $convertToXml
    )
    {
        $this->logger = $logger;
        $this->convertToXml = $convertToXml;
    }

    public function convert($data, $format)
    {
        switch ($format) {
            case 'json':
                $converter = new ConvertToJson();
                break;
            case 'csv':
                $converter = new ConvertToCsv();
                break;
            case 'yml':
                $converter = new ConvertToYaml();
                break;
            case 'xml':
                $converter = $this->convertToXml;
                break;
            case 'object':
                $converter = new ConvertToObject();
                break;
            default:
                throw new \DomainException('no matching converter');
        }

        return $converter->convert($data);
    }
}

Into this class we inject the Logger service, and also the ConvertToXml service we created earlier.

The associated service definition now looks like:

# /app/config/services.yml

services:
    crv.conversion:
        class: AppBundle\Service\Conversion
        arguments:
            - "@logger"
            - "@crv.converter.convert_to_xml"

    crv.converter.convert_to_xml:
        class: AppBundle\Converter\ConvertToXml
        arguments:
            - "@serializer"

Our service's convert method is a slightly different signature to that of our ConverterInterface, so we need to ensure we update the controller code appropriately:

<?php

// /src/AppBundle/Controller/DefaultController.php

namespace AppBundle\Controller;

use Sensio\Bundle\FrameworkExtraBundle\Configuration\Route;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\HttpFoundation\Request;

class DefaultController extends Controller
{
    /**
     * @Route("/", name="homepage")
     */
    public function indexAction(Request $request)
    {
        $data = [
            'abc'   => 123,
            'hello' => 'world',
            'qwe'   => [
                'key' => 'value'
            ],
            'xyz'   => 'alphabet'
        ];

        $format = $request->query->get('format', 'json');

        $output = $this->get('crv.conversion')->convert($data, $format);

        // replace this example code with whatever you need
        return $this->render('default/index.html.twig', [
            'output' => $output
        ]);
    }
}

And overall this may very well be good enough.

Yes, this is simple, and basic. But it works.

You could go further with this, splitting the conversion process from the instantiation of the ConvertToX instance. You might place the switch statement into its own class, calling that ConverterFactory, and then passing the ConverterFactory into the Conversion service.

It's entirely up to you, and the comfort level of your team how deep you go with this. This is a contentious point so consider this as one opinion.

What follows next is the sort of thing you might see in Enterprise codebases, and with it comes an increased level of complexity. It also works well if you want to make a re-usable Bundle of your own.

Code For This Course

Get the code for this course.

Episodes

# Title Duration
1 Extract, Extract, Extract 10:09
2 Introducing The Compiler Pass 13:03