Today we are going to take into account a real example I was working on in the last days. It’s a quite simple case study that does not involve any knowledge about our software or domain so is perfect to show how, with a little bit of work, you can take strongly coupled classes and make them decoupled, reusable and more testable.
Scenario
Basically we wanted to create a class that takes an Entity representing a File (named FileEntity into following snippets) and apply a watermark on it through a service (named WatermarkGenerator). It’s not a pain-in-the-back-task, is it? Also is worth to note that in this first phase we want to handle only .pdf files.
To reach our goal, we create a WatermarkGenerator class that provides API to watermark files, PdfWatermarker – used into WatermarkGenerator – that applies watermark and PdfWatermarkText (not reported here) used to process watermark text.
It’s important to note that all namespaces, use directive and other parts of code not reported (including PHPDocs) are considered useless (and inopportune) for full comprehension. Also this process did not follow a TDD/BDD approach as this refactor involved a pre-existent code (that you will find below).
Existent code
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 |
<?php class WatermarkGenerator { protected $entityFileManager; protected $em; public function __construct(EntityFileManager $entityFileManager, EntityManager $em) { $this->entityFileManager = $entityFileManager; $this->em = $em; } public function watermark(FileEntity $fileEntity) { if (!$fileEntity->canBeWatermarked()) { throw new WatermarkException("..."); } if (!$this->isPdf($fileEntity)) { throw new WatermarkException("..."); } try { $this->applyWatermark($fileEntity); } catch (\Exception $e) { // exception handling } return true; } protected function applyWatermark(FileEntity $fileEntity) { // in here (omitted) are performed operations strictly coupled to FileEntity logic and structure // such like $fileEntity->removeSignatureIfPresent(); (*1; see below) $inputPdfPath = $this->entityFileManager->getInputPath($fileEntity); $outputPdfPath = sprintf('%s%s', $this->entityFileManager->getOutputPath(), $fileName); $text = $this->generateTextForWatermark($fileEntity); $pdfWatermarkText = new PdfWatermarkText($text); $watermarker = new PdfWatermarker($inputPdfPath, $outputPdfPath, $pdfWatermarkText, $fileEntity); $watermarker->watermarkPdf(); $this->entityFileManager->addFileToEntityFile($outputPdf, $fileEntity->getFileName(), $fileEntity); $this->em->persist(fileEntity); $this->em->flush(); return true; } protected function isPdf(FileEntity $fileEntity) { // all logic for check if FileEntity is pdf } protected function generateTextForWatermark(FileEntity $fileEntity) { // all logic for watermark generation. relative to FileEntity structure } } |
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 |
<?php class PdfWatermarker { private $originalPdf; private $newPdf; private $watermark; protected $fileEntity; public function __construct($originalPdf, $newPdf, PdfWatermarkText $watermark, FileEntity $fileEntity) { $this->originalPdf = $originalPdf; $this->newPdf = $newPdf; $this->watermark = $watermark; $this->fileEntity = $fileEntity; $this->validateAssets(); } private function validateAssets() { if (!file_exists($this->originalPdf)) { throw new \Exception("Inputted PDF file doesn't exist"); } } public function watermarkPdf() { // some logic here switch ($this->fileEntity->getSignaturePosition()) { // perform logic based on signature position } // some logic here return true; } } |
EntityFileManager is just a service that helps us to abstract FileSystem (remote, local, etc) and provide facilities to access and write files. It’s not interesting for this article but we need to include it as takes part into refactor process.
As you can see here there are two huge design problems with WatermarkGenerator. As a matter of fact it is strictly coupled to FileEntity – so, basically, you cannot reuse it with any other entity representing a file – and it also uses EntityManager facilities; in particular this broke SOLID principle.
Also PdfWatermarker is strictly coupled to FileEntity
Single responsibility principle … what?
To “master” this principle first question you have ALWAYS to ask yourself is
What this class is supposed to do?
If you can’t answer without using “and” word, well, your class is breaking SRP.
In this example, an answer to the question would be
This class is used for watermark a file and persist it to db
Refactor list
- Decouple
WatermarkGeneratorandPdfWatermarkerfromFileEntity. - Follow SOLID principle and separate watermarking of
FileEntityfrom its persist operation.
Step 1 – Decoupling
A possible solution for decoupling WatermarkGenerator and PdfWatermarker from FileEntity, is to create a generic WatermarkableEntity (that is pretty much a container for our objects) that implements an interface named WatermarkableElementInterface and pass it to WatermarkGenerator and PdfWatermarker.
WatermarkableEntity will contain FileEntity and provide a lot of abstraction on it (we will see how below). But, if is true that PdfWatermarker just needs to call getSignaturePosition() method regardless the class of file entity, that’s not fulfill WatermarkGenerator needs. In fact, based on file entity actual class (FileEntity in our example, but we will need to watermark also other entity types), we could need to perform tasks that are specific to it or – more common – we need a different way to invoke methods. For this reason we introduce a EntityFileWatermarkGenerator that extends WatermarkGenerator.
This is the code
|
1 2 3 4 5 6 7 8 9 10 |
<?php interface WatermarkableElementInterface { public function generateTextForWatermark(); public function getFileName(); public function getSignaturePosition(); } |
As you can see a client (PdfWatermark) needs to know only what methods to call to meet its needs. No details about concrete classes.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 |
<?php class FileEntityWatermarkableElement implements WatermarkableElementInterface { private $fileEntity; public function __construct(FileEntity $fileEntity) { $this->fileEntity = $fileEntity; } public function generateTextForWatermark() { // as you can see here, getTextForWatermark() is now migrated into real entity file class return $this->fileEntity->getTextForWatermark(); } public function getFileName() { return $this->fileEntity->getFileName(); } public function getSignaturePosition() { return $this->fileEntity->getSignaturePosition(); } } |
Introducing FileEntityWatermarkGenerator.php
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 |
<?php class FileEntityWatermarkGenerator extends WatermarkGenerator { protected $em; public function __construct(EntityFileManager $entityFileManager, EntityManager $entityManager) { parent::__construct($entityFileManager); $this->em = $entityManager; } // We cannot use type hinting due to inheritance public function watermark($fileEntity) { // logic strictly related to FileEntity. Migrated from WatermarkGenerator if (!$fileEntity->canBeWatermarked()) { throw new WatermarkException("..."); } $fileEntityWatermarkableElement = new FileEntityWatermarkableElement($fileEntity); try { parent::watermark($fileEntityWatermarkableElement); } catch (\Exception $e) { // exception handling here } return true; } protected function applyWatermark(WatermarkableElementInterface $fileEntityWatermarkableElement) { // in here (omitted) are performed operations strictly coupled to FileEntity logic and structure // such like $fileEntity->removeSignatureIfPresent(); (*1; see above) $outputPdf = parent::applyWatermark($fileEntityWatermarkableElement); $this->entityFileManager->addFileToEntityFile( $outputPdf, $fileEntityWatermarkableElement->getFileName(), $fileEntityWatermarkableElement ); $this->em->persist($fileEntityWatermarkableElement); $this->em->flush(); return true; } } |
This works following this logic: all operations relative to FileEntity (pre and post watermark operations) are done right here but all “common” watermark operations are done through parent:: calls (WatermarkGenerator). You also might have noticed that type hinting in this scenario is not possibile; we will tackle it later.
Let’s see how WatermarkGenerator and PdfWatermarker are changed
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 |
<?php class WatermarkGenerator { protected $entityFileManager; public function __construct(EntityFileManager $entityFileManager) { $this->entityFileManager = $entityFileManager; } // We cannot use type hinting due to inheritance public function watermark($watermarkableElement) { if (!$this->isPdf($fileEntity)) { throw new WatermarkException("..."); } $this->applyWatermark($watermarkableElement); // this will call function of fileEntityWatermarkGenerator return true; } protected function applyWatermark(WatermarkableElementInterface $watermarkableElement) { $fileName = $watermarkableElement->getFileName(); $inputPdfPath = $this->entityFileManager->getInputPath($watermarkableElement->getEntityFile()); $outputPdfPath = sprintf('%s%s', $this->entityFileManager->getOutputPath(), $fileName); $text = $watermarkableElement->generateTextForWatermark(); $pdfWatermarkText = new PdfWatermarkText($text); $watermarker = new PdfWatermarker( $inputPdfPath, $outputPdfPath, $pdfWatermarkText, $watermarkableElement ); $watermarker->watermarkPdf(); return $outputPdfPath; } } |
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 |
<?php class PdfWatermarker { private $originalPdf; private $newPdf; private $watermark; protected $watermarkableEntity; public function __construct( $originalPdf, $newPdf, PdfWatermarkText $watermark, WatermarkableElementInterface $watermarkableEntity ) { $this->originalPdf = $originalPdf; $this->newPdf = $newPdf; $this->watermark = $watermark; $this->watermarkableEntity = $watermarkableEntity; $this->validateAssets(); } private function validateAssets() { if (!file_exists($this->originalPdf)) { throw new \Exception("Inputted PDF file doesn't exist"); } } public function watermarkPdf() { // some logic here switch ($this->fileEntity->getSignaturePosition()) { // perform logic based on signature position } // some logic here return true; } } |
As you might have noticed above we can pass pretty much every type of entity file type to WatermarkGenerator as them will be enveloped into an external object implementing an interface. This is a huge advantage as we’re not declaring actual class of entity file anywhere in WatermarkGenerator.
Looking at these snippets of code we noticed that inheritance between FileEntityWatermarkGenerator and WatermarkGenerator could lead to inflexible scenarios (like every time you choose to use inheritance) and is also difficult to read what’s going on when FileEntityWatermarkGenerator‘s watermark() function is invoked. Moreover you can notice that watermark() can’t take advantage of type hint feature for argument passing due to inheritance (we can always slightly change names, it’s true, but this is just an example of kind of things you should keep an eye on with inheritance).
Composition over inheritance
We will not discuss here the benefits of composition over inheritance (you can query google and choose a “random” link; them are all just good) but we choose to follow the “rule” and implement a collaboration between FileEntityWatermarkGenerator and WatermarkGenerator.
Code changes as follows
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 |
<?php class FileEntityWatermarkGenerator { protected $em; protected $entityFileManager; protected $watermarkGenerator; public function __construct( WatermarkGenerator $watermarkGenerator, EntityFileManager $entityFileManager, EntityManager $entityManager ) { $this->em = $entityManager; $this->entityFileManager = $entityFileManager; $this->watermarkGenerator = $watermarkGenerator; } // TYPE HINT! public function watermark(FileEntity $fileEntity) { // logic strictly related to FileEntity. Migrated from WatermarkGenerator if (!$fileEntity->canBeWatermarked()) { throw new WatermarkException("..."); } $fileEntityWatermarkableElement = new FileEntityWatermarkableElement($fileEntity); try { // this is a collaborator, now. $outputPdfPath = $this->watermarkGenerator->watermark($fileEntityWatermarkableElement); $this->saveWatermarkedFile($fileEntityWatermarkableElement, $outputPdfPath); } catch (\Exception $e) { // exception handling here } return true; } protected function saveWatermarkedFile( FileEntityWatermarkableElement $fileEntityWatermarkableElement, $outputPdfPath ) { // in here (omitted) are performed operations strictly coupled to FileEntity logic and structure // such like $fileEntity->removeSignatureIfPresent(); (*1; see above) $this->entityFileManager->addFileToEntityFile( $outputPdfPath, $fileEntityWatermarkableElement->getFileName(), $fileEntityWatermarkableElement ); $this->em->persist($fileEntityWatermarkableElement); $this->em->flush(); return true; } } |
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 |
<?php class WatermarkGenerator { protected $entityFileManager; public function __construct(EntityFileManager $entityFileManager) { $this->entityFileManager = $entityFileManager; } // TYPE HINT! public function watermark(WatermarkableElementInterface $watermarkableElement) { if (!$this->isPdf($fileEntity)) { throw new WatermarkException("..."); } return $this->applyWatermark($watermarkableElement); } protected function applyWatermark(WatermarkableElementInterface $watermarkableElement) { $fileName = $watermarkableElement->getFileName(); $inputPdfPath = $this->entityFileManager->getInputPath($watermarkableElement->getEntityFile()); $outputPdfPath = sprintf('%s%s', $this->entityFileManager->getOutputPath(), $fileName); $text = $watermarkableElement->generateTextForWatermark(); $pdfWatermarkText = new PdfWatermarkText($text); $watermarker = new PdfWatermarker( $inputPdfPath, $outputPdfPath, $pdfWatermarkText, $watermarkableElement ); $watermarker->watermarkPdf(); return $outputPdfPath; } } |
Step 2 – Single Responsibility Principle (SRP)
Second phase expects to take out EntityManager and logic marked as *1 into code snippets, as those functionalities aren’t relevant to FileEntityWatermarkGenerator. This kind of operation is really easy to perform and understand.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 |
<?php class FileEntityWatermarkGenerator { protected $watermarkGenerator; // No more EntityManager injected public function __construct(WatermarkGenerator $watermarkGenerator) { $this->watermarkGenerator = $watermarkGenerator; } public function watermark(FileEntity $fileEntity) { // logic strictly related to FileEntity. Migrated from WatermarkGenerator if (!$fileEntity->canBeWatermarked()) { throw new WatermarkException("..."); } $fileEntityWatermarkableElement = new FileEntityWatermarkableElement($fileEntity); try { return $this->watermarkGenerator->watermark($fileEntityWatermarkableElement); } catch (\Exception $e) { // exception handling here } } } |
Now this seems to fit well into SRP definition. Isn’t it?
Here will not be reported persistence code as it is straightforward as we invoke watermark() function and perform operations that were into saveWatermarkedFile() function elsewhere (typically into a service called *Manager where * stands for actual type of manager).
We are now very close to our goal but it is important to notice that WatermarkGenerator yet relies onto EntityFileManager. This should be a common class, but that way we are implicitly saying that every concrete entity file class should have its physical file onto file system. And what about, tomorrow, we decide to introduce a new WatermarkableEntity type that relies onto streams (or whatever?). Furthermore, why should WatermarkerGenerator be the one who keeps logic about file retrieve and write? That sounds also like a violation of SRP.
So … where to migrate this collaborator and the logic about file system?
Our answer is “into FileEntityWatermarkableElement“, because we are sure that this element will always relies onto filesystem and because that way none of the clients who use it need to know anything about implementation details. Sounds like a very good solution.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 |
<?php class FileEntityWatermarkableElement implements WatermarkableElementInterface { private $fileEntity; private $entityFileManager; public function __construct(FileEntity $fileEntity, EntityFileManager $entityFileManager) { $this->fileEntity = $fileEntity; $this->entityFileManager = $entityFileManager; } public function generateTextForWatermark() { return $this->fileEntity->getTextForWatermark(); } public function getFileName() { return $this->fileEntity->getFileName(); } public function getSignaturePosition() { return $this->fileEntity->getSignaturePosition(); } public function getInputPath() { return $this->entityFileManager->getInputPath($this); } public function getOutputPath() { return sprintf( '%s%s', $this->entityFileManager->getOutputPath(), $this->getFileName() ); } } |
It’s important to notice that getInputPath() and getOutputPath() should be included into WatermarkableElementInterface contract as are invoked by class that relies on WatermarkableElementInterface (not reported here for shortness; e.g.: WatermarkGenerator that now has not EntityFileManager injected anymore)
Conclusions
Classes are not strictly coupled anymore. They can be combined to obtain different ways to retrieve a file, place a watermark on it and write back content onto any destination (file system, stream, and so on) just by creating a new version of <ConcreteEntityFile>WatermarkableElement and, if “domain logic” is present, a new version of <ConcreteEntityFile>WatermarkerGenerator that should be used with common WatermarkGenerator.
If you did not follow a pure BDD/TDD approach (as I did, because I found already written code to modify) and tried to write tests onto first version of code, you would probably noticed that a lot of mocks and stubs were involved into this process, making test classes very messy! If you take a look to post-refactor code you probably understand at a first and shallow glance that code is now easier to test.
Finally, one possibile upgrade to this code could be a factory introduction to obtain a WatermarkerGenerator able to watermark not only .pdf but a lot of different types of file (so, remove dependency between WatermarkerGenerator and PdfWatermarker).
