Refactoring-Challenge 1 – Solution (1)

Did you complete the challenge? Have you found all the bugs and inconsistencies inside the code?

As promised, today, I will present to you my approach towards the challenge. And I’ll try to verbalize my thought process entirely.

The beginning

We might change quite a few things inside this code and I don’t want to test several requests every time by hand, so let’s start by creating a functional test:

<?php

namespace App\Tests\Controller;

use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;

class DefaultControllerTest extends WebTestCase
{
	const HOST = '<http://refactoring.localtest.me>';

    public function testIfReadmeRequestsAreWorking()
    {
        $host = '<http://refactoring.localtest.me>';
        $client = static::createClient();
        $client->request('GET', self::HOST . '/?file=video_12345_q8c.mp4&directory=video1&quality=q6a&type=video');

        $response = $client->getResponse();
        $this->assertSame(200, $response->getStatusCode());

        $client->request('GET', self::HOST . '/?file=image_12345.png&directory=image2&type=image');
        $response = $client->getResponse();
        $this->assertSame(200, $response->getStatusCode());
    }
}

When you run this test for the first time, you will get the error message: Error: Call to undefined method App\\Controller\\DefaultController::get(). So it seems as if “someone” used the old way of receiving services from the container with this->get(..). Thankfully, with the stack trace, we get the exact line number that needs changing.

public function indexAction(Request $request, FakeRepository $repo, LoggerInterface $logger)
{
    // ...

    # removed line 28: $logger = $this->get('logger');
    # added LoggerInterface to the function parameters

    // ...

    $logger->debug('Request is of type: image.');

Tests are green, everything works now, right? Well, let’s see about that.

Error handling

What happens in the case of an error? The readme provides us with two links for the types video and image. What happens for audio or something that cannot be found like asdfaswefawkk? When we have a first glance at the code, we might assume that any type other than video or image will return a 500 error response.

public function testNonExistingTypeParameter()
{
    $client = static::createClient();
    $client->request('GET', self::HOST . '/?file=video_12345_q8c.mp4&directory=video1&quality=q6a&type=audio');

    $response = $client->getResponse();
    $this->assertSame(500, $response->getStatusCode());
    $this->assertSame(['status' => 'error'], $response->getContent());
}

Which results in:

There was 1 failure:

1) App\\Tests\\Controller\\DefaultControllerTest::testNonExistingTypeParameter
Failed asserting that '{"status":"ok"}' matches JSON string "{"status": "error"}".
--- Expected
+++ Actual
@@ @@
 {
-    "status": "error"
+    "status": "ok"
 }

The first thing I noticed was, that “someone” had a little error inside the return statement.

// original returning status code 200:
return new JsonResponse(['status' => 'error', 500]);

// fixed:
return new JsonResponse(['status' => 'error'], 500);

But it was still not working, so you have to dig a little deeper into the code. There you will probably notice that the $success variable is wrongly initialized as true. So unless an entity is not found (see catch-block), $success will be true even if the type cannot be handled.

Parsing filenames

While going through the code, I noticed that I spared you with one annoying little bug that I added to the internal challenge I gave to my colleagues! It was parsing the entity’s id from the filename but with some one-off errors (like 1234_) and misleading variable namings.

Nevertheless, let’s have a look at how the ids are fetched from the filenames:

// images
$firstUnderscore = strpos($filename, '_');
$sU = strpos($filename, '.', $firstUnderscore + 1);
$id = (int) substr($filename, $firstUnderscore + 1, $sU - $firstUnderscore - 1);

$image = $repo->getImageById($id);

// video
$firstUnderscore = strpos($filename, '_');
$secondUnderscore = strpos($filename, '_', $firstUnderscore + 1);
$id = (int) substr($filename, $firstUnderscore + 1, $secondUnderscore - $firstUnderscore - 1);

$video = $repo->getVideoById($id);

First things first! The code looks very familiar but the naming is a bit off. What is sU supposed to mean other than secondUnderscore? But it’s not an underscore, so we might generalize the parsing to:

Find the character positions in front of and directly after our id, cut out the part between.

I would love to refactor the code right here and now but so far, we haven’t even checked if this part of the code is working properly.

/**
 * @dataProvider parsingImageIdDataProvider
 *
 * @param $filename
 * @param $expectToFindEntity
 */
public function testParsingImageId($filename, $expectToFindEntity)
{
    $url = self::HOST . "/?file=${filename}&directory=irrelevant&quality=irrelevant&type=image";
    $client = static::createClient();
    $client->request('GET', $url);

    $response = $client->getResponse();

    if ($expectToFindEntity) {
        $this->assertSame(200, $response->getStatusCode());
    } else {
        $this->assertSame(500, $response->getStatusCode());
    }
}

protected function parsingImageIdDataProvider(): array
{
    return [
        ['image_12345.irrelevant', true],
        ['image_12346.gif', true],
        ['image-something_12347.mp4', true],
        ['image_12348.jpg', false],
        ['image_12349.jpg', false],
        ['image12349.jpg', false],
        ['im_age_12345.jpg', false],
    ];
}

Before writing the data providers, I had a look inside the FakeDatabase where a couple of mock objects are created (ids 12345-12347). In analogy, I tested the same for the video type. Interestingly, there is absolutely no validation on file endings or other parameters added to the URL or the image name except for the quality URL parameter on videos (which can be completely different inside the filename!).

The foundation stone has been laid

The code still isn’t pretty, but at least it’s not throwing any more exceptions or false success messages. Furthermore, we added functional tests that will help us in the next chapter to make sure our refactoring is not breaking any functionality. With the insight we gained by testing and fixing the code, we can now ‘go to our manager’ and ask how the code is actually supposed to work and refactor it in the next part, to make it:

• more readable
• more extensible
• more testable

Or simply put: better.

Stay tuned for part 2! I am looking forward to seeing all your solutions to the refactoring challenge (part 1 & 2) as a Pull Request on GitHub!

https://github.com/moritzwachter/refactoring-challenge-1/tree/solution/challenge-part-1