With the last blog post, we laid the foundation for proper refactoring. We found a few small bugs, fixed them, and most importantly: wrote a couple of functional tests to make sure that the example webhook calls from the README.md file are working. Why are the functional tests so important? Couldn’t we just refactor the code and test the requests, in the end, to make sure they still work? First off, the code wasn’t working in the first place 😉Second, you might want to test in between your implementations if everything is still working. This would take you a lot more time doing it manually every time and this usually results in not testing all edge cases manually which will be more error-prone than simply running the tests automatically after every change. And this, for me, is the most important part: we can make sure everything we tested before is still working after every change we make!
That being said, let’s grab a coffee and talk about: Cleaning up the mess. 🧹
Starting small
As Martin Fowler put it:
Refactoring is a disciplined technique for restructuring an existing body of code, altering its internal structure without changing its external behavior.
Its heart is a series of small behavior preserving transformations. Each transformation (called a “refactoring”) does little, but a sequence of these transformations can produce a significant restructuring. […]Source: https://refactoring.com/
With this in mind, we will focus on small, safe steps to restructure the code without changing its behavior. One of the first and easiest steps is to remove unused use-statements and reformat the code to have a consistent coding style (which increases readability and makes your colleagues/maintainers happy).
Moving logic parts into separate methods
The code in the DefaultController looks a lot like it’s doing almost the same but once for images and once for videos. That’s the moment you’re brain probably goes: “Strategy Pattern?”. But even when you’re not familiar with design patterns, you can still see that there is something similar happening. So my next, safe step is moving these logic blocks into separate methods – e.g. handleImage(...) and handleVideo(...). Probably the easiest and most time-saving way is to use PHPStorm’s Refactor action: “Extract Method…” or use its shortcut (on Mac: Cmd+Opt+M):
Without looking at the newly extracted methods, our main controller action looks a lot cleaner now:
/**
* @Route("/", name="homepage")
* @param Request $request
* @param FakeRepository $repo
* @return JsonResponse
*/
public function indexAction(Request $request, FakeRepository $repo, LoggerInterface $logger)
{
$filename = $request->query->get('file');
$qualityKey = $request->query->get('quality');
$directory = $request->query->get('directory');
$type = $request->query->get('type');
$success = false;
try {
if ($type === 'image') {
$success = $this->handleImage($logger, $filename, $repo, $directory);
} elseif ($type === 'video') {
$success = $this->handleVideo($logger, $filename, $repo, $directory, $qualityKey);
}
} catch (EntityNotFoundException $notFoundHttpException) {
return new JsonResponse(['status' => 'not found'], 404);
}
if ($success) {
return new JsonResponse(['status' => 'ok'], 200);
}
return new JsonResponse(['status' => 'error'], 500);
}
The two methods almost have the same method signature. We could make the $qualityKey optional and go with a 5-parameter method but, when we think about our code’s extensibility, what happens when we add a third file type (e.g., “audio”) with additional parameters?
Data Transfer Objects (DTOs)
Another solution could be to pack all our relevant information from the request into a DTO. In order to do this, we create a new class DistributionDTO with the properties: $filename, $directory, $type, $qualityKey. As this DTO is getting all its data directly from the request, I decided to create a static factory method:
public static function fromRequest(Request $request)
{
$filename = $request->query->get('file');
$directory = $request->query->get('directory');
$type = $request->query->get('type');
$qualityKey = $request->query->get('quality') ?? '';
return new self($filename, $directory, $type, $qualityKey);
}
The whole DTO now looks like this: https://github.com/moritzwachter/refactoring-challenge-1/blob/2e52d80884c9e91a14619217c9a4d7812f10eead/src/DTO/DistributionDTO.php
When we now look back at our controller action, we can simplify the parameters we inject into our handle-methods:
$distributionData = DistributionDTO::fromRequest($request);
// ...
if ($distributionData->getType() === 'image') {
$success = $this->handleImage($logger, $repo, $distributionData);
} elseif ($distributionData->getType() === 'video') {
$success = $this->handleVideo($logger, $repo, $distributionData);
}
// ...
And all of a sudden, the methods have the same signature! Now, we can think about moving those methods into their own classes/services.
Separation of concerns
In our next step, we want to move the different logical parts into separate classes, so that each class has only one job to do. For me personally, there’s always the question: “On which level are you looking at the different concerns?”. Maybe it’s enough to make sure the ImageDistribution service is only taking care of the distribution but what about the extraction of the id from the filename? In our example, it’s not used anywhere else so it solely serves the purpose of getting the id for handling the image distribution. Should we still move it to another service? I would say: it depends! It depends on your personal coding style, it depends on whether you want to write tests for it separately or not, it depends on whether you want to reuse this method in the future because you already know there will be another use-case.
It depends on a lot of different factors, which is why I will postpone my decision until I’ve completed the first step:
- One controller to handle the request and output
- One factory to decide which strategy should be used
- Two strategies for distributing images and videos
Strategy Pattern
Let’s start with the two strategies! We already did a little preparation by using a DTO and moving the logic to separate methods. Next, we’re going to extract the methods into new classes.
<?php
namespace App\\Distribution;
use App\DTO\DistributionDTO;
interface DistributionStrategyInterface
{
public function handleDistribution(DistributionDTO $distributionDTO): bool;
}
We streamlined the method signatures, so that it’s now possible to create an interface for our (currently) two strategies. As you might have noticed, I did not include the dependencies (repository and logger) because they will be injected via constructor arguments:
<?php
namespace App\\Distribution;
use App\DTO\DistributionDTO;
use App\Mocks\FakeRepository;
use Psr\Log\LoggerInterface;
class ImageDistributionStrategy implements DistributionStrategyInterface
{
/** @var FakeRepository */
private $repository;
/** @var LoggerInterface */
private $logger;
/**
* @param FakeRepository $repo
* @param LoggerInterface $logger
*/
public function __construct(FakeRepository $repo, LoggerInterface $logger)
{
$this->repository = $repo;
$this->logger = $logger;
}
public function handleDistribution(DistributionDTO $distributionDTO): bool
{
$this->logger->debug('Request is of type: image.');
$filename = $distributionDTO->getFilename();
$firstUnderscore = strpos($filename, '_');
$sU = strpos($filename, '.', $firstUnderscore + 1);
$id = (int)substr($filename, $firstUnderscore + 1, $sU - $firstUnderscore - 1);
$image = $this->repository->getImageById($id);
$image->setDirectory($distributionDTO->getDirectory());
$image->setIsDistributed(true);
$this->repository->save($image);
return true;
}
}
(The same has been done for the VideoDistributionStrategy). As we don’t have a factory yet, but I still want to make sure everything is still working, I added the strategies to the controller:
public function indexAction(
Request $request,
ImageDistributionStrategy $imageStrategy,
VideoDistributionStrategy $videoStrategy
) {
$distributionData = DistributionDTO::fromRequest($request);
$success = false;
try {
if ($distributionData->getType() === 'image') {
$success = $imageStrategy->handleDistribution($distributionData);
} elseif ($distributionData->getType() === 'video') {
$success = $videoStrategy->handleDistribution($distributionData);
}
} catch (EntityNotFoundException $notFoundHttpException) {
return new JsonResponse(['status' => 'not found'], 404);
}
if ($success) {
return new JsonResponse(['status' => 'ok'], 200);
}
return new JsonResponse(['status' => 'error'], 500);
}
Tests are still green! Great, now I can restructure the code a little more. As you can see, both strategies are calling the same method. So we can just assign the strategy inside of the “if-else if”-condition and move the method call beneath it.
try {
if ($distributionData->isImageType()) {
$chosenStrategy = $imageStrategy;
} elseif ($distributionData->isVideoType()) {
$chosenStrategy = $videoStrategy;
} else {
throw new \\InvalidArgumentException('Invalid type');
}
$success = $chosenStrategy->handleDistribution($distributionData);
} catch (EntityNotFoundException $notFoundHttpException) {
return new JsonResponse(['status' => 'not found'], 404);
} catch (\\InvalidArgumentException $invalidArgumentException) {
$success = false;
}
I did a couple of things here. First, I’ve added two helper methods isImageType(), isVideoType() which return the same condition, but I think they might improve the readability here. Then, I introduced the new variable $chosenStrategy and moved the method call beneath the if-condition section. Now the tests were failing for the scenario of an unknown type (like “audio”, for example), so I needed to throw a suitable exception in the else clause and catch it to set $success to false. We will get rid of the $success variables in the end – bear with me for now 😉.
New classes = new tests?
Now would be the perfect time to add (unit) tests to those two strategies, so we can make sure that we don’t spot any critical issues and we can use them, later on, to make sure any changes to the code don’t affect the outcome (negatively). I did both unit tests with a lot of mocking and – as we are already having fake/mock objects – functional tests with our “real” FakeRepository and services.
Both can be found here: https://github.com/moritzwachter/refactoring-challenge-1/tree/2cd144a2dd9d3f07bb35b332e47ca8840bd718d3/tests/Distribution
With these tests, we can now improve the hideous part of the distribution code: the id extraction from the filename. Feel free to do so on your own! I will write a blog post about this topic soon and won’t get into this right now.
The Factory
As I’ve mentioned in the last chapter, I want to extract the logic, which strategy to use, out of the controller. We will use a factory to do so:
<?php
namespace App\\Distribution;
use App\DTO\DistributionDTO;
class DistributionFactory
{
/** @var ImageDistributionStrategy */
private ImageDistributionStrategy $imageStrategy;
/** @var VideoDistributionStrategy */
private VideoDistributionStrategy $videoStrategy;
public function __construct(
ImageDistributionStrategy $imageDistributionStrategy,
VideoDistributionStrategy $videoDistributionStrategy
) {
$this->imageStrategy = $imageDistributionStrategy;
$this->videoStrategy = $videoDistributionStrategy;
}
/**
* @param DistributionDTO $distributionData
* @return DistributionStrategyInterface
* @throws \\InvalidArgumentException
*/
public function getDistributionStrategy(DistributionDTO $distributionData): DistributionStrategyInterface
{
if ($distributionData->isImageType()) {
return $this->imageStrategy;
}
if ($distributionData->isVideoType()) {
return $this->videoStrategy;
}
throw new \\InvalidArgumentException('Invalid type');
}
}
Okay, looks pretty straightforward. What have we gained by moving this code to a separate class? In the end, we generated far more lines of code than we had before! The first advantage is that we can now write unit tests solely for the logic of which strategy to use. This might not be relevant as you only have two simple filename checks, but when you have 5 types and maybe more complex logic on which strategy to use, this might come in handy! The second advantage is that we can add strategies without touching the code of the controller. Remember, the controller’s only concern should be the handling of requests, addressing the right services, and returning their responses.
You could even go one step further now and let the strategies decide on their own if they should be used or not. Every strategy would implement a method supports(DistributionDTO $dto) and the factory iterates through a collection of tagged services (e.g. distribution.strategy) and pick the only which returns true. (If you are interested in reading more about this, feel free to contact me 😉).
Finally, we need to update the controller to work with the factory now:
public function indexAction(
Request $request,
DistributionFactory $factory
) {
$distributionData = DistributionDTO::fromRequest($request);
$success = false;
try {
$chosenStrategy = $factory->getDistributionStrategy($distributionData);
$success = $chosenStrategy->handleDistribution($distributionData);
} catch (EntityNotFoundException $notFoundHttpException) {
return new JsonResponse(['status' => 'not found'], 404);
} catch (\\InvalidArgumentException $invalidArgumentException) {
$success = false;
}
if ($success) {
return new JsonResponse(['status' => 'ok'], 200);
}
return new JsonResponse(['status' => 'error'], 500);
}
And the very final step is to remove the $success variable and restructure the error/exception handling properly. We might need to adjust a few methods and tests, too (see repository).
<?php
namespace App\\Controller;
use App\Distribution\DistributionFactory;
use App\DTO\DistributionDTO;
use Doctrine\ORM\EntityNotFoundException;
use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\Routing\Annotation\Route;
class DefaultController
{
/**
* @Route("/", name="homepage")
* @param Request $request
* @param DistributionFactory $factory
* @return JsonResponse
*/
public function indexAction(
Request $request,
DistributionFactory $factory
) {
$distributionData = DistributionDTO::fromRequest($request);
try {
$chosenStrategy = $factory->getDistributionStrategy($distributionData);
$chosenStrategy->handleDistribution($distributionData);
return new JsonResponse(['status' => 'ok'], 200);
} catch (EntityNotFoundException $notFoundHttpException) {
return new JsonResponse(['status' => 'not found'], 404);
} catch (\\InvalidArgumentException $invalidArgumentException) {
return new JsonResponse(['status' => $invalidArgumentException->getMessage()], 500);
} catch (\\Exception $e) {
return new JsonResponse(['status' => 'error'], 500);
}
}
}
Summary
In this blog post, I showed you my approach to solving the refactoring challenge after we already did some precautionary measures (i.e., functional tests) to make sure we don’t break any functionality. We were able to extract the distribution logic and identify a useful design pattern. We wrote tests for these strategies and created a factory to decide which strategy to use by handing over the request parameters we are now passing on via data transfer objects (DTOs). Finally, we were able to restructure the controller to a nicely readable 13-line-method with proper exception handling.
