Let's talk about something cool: trojan horse parameters. This is how I call this fancy technique which many of us, developers, might have used (or still doing). Ok, so what is it?
As a daily work, you face a problem, think about a solution and start writing a piece of code for it, then you pack it into a [hopefully] well-named function.
Once you're done, a bright idea comes to your mind telling you: Hey! why not extend your solution so that it also includes "case B"? You think a bit and answer: Yes! why not?
Let's illustrate this through an example: You work on a class which is responsible for data persistence, and there are two ways of doing this: either local storage or remote/cloud storage.
And as you appreciated the pop-up idea that asked you to extend your solution so that it covers both cases, you ended up writing this nice code:
<?php
class FileStorage
{
public function store(File $file, $remote = false): void
{
if ($remote) {
$compressedFile = $this->compress($file);
$fileParts = $this->splitFile($compressedFile);
$this->logger->info('Started uploading to AWS/S3 started.');
foreach ($fileParts as $filePart) {
$this->awsClient->upload($filePart);
}
$this->logger->info('Finished uploading to AWS/S3 started.');
} else {
$fp = fopen($this->getStoragePath() . $file->getName(), 'w');
fwrite($fp, $file->getContent());
fclose($fp);
$this->logger->info('File successfully saved on local storage.');
}
}
}
Ok, can you tell me what's wrong with that thing above? I can tell you my point of view:
If you have a look at it twice, you'll realise the $remote parameter is actually acting as a trigger or switch button, responsible for a complete change of the function's behavior. You can conclude that in case of $remote = true we have a totally standalone logic pushing data to S3/AWS, and in case $remote = false a second different logic is writing data to local disk.
Which rule are we violating here?
Single Responsibility Principle ?
Separation of Concerns ?
Modularity ?
Rigidity ?
This tiny little optional parameter turned into a trojan horse that changes the logic of the function 180 degrees, in a totally silent way.
So, why not be transparent, modular, flexible and single responsible? Besides, no big effort must to be taken here, just split and rename ! The only thing we have to do is to get rid of the optional/default parameter and move each block under the if statement into a separated new function, which name should reflect the actual task being achieved; something like this:
<?php
class FileStorage
{
public function saveToDisk(File $file): void
{
$fp = fopen($this->getStoragePath() . $file->getName(), 'w');
fwrite($fp, $file->getContent());
fclose($fp);
$this->logger->info('File successfully saved on local storage.');
}
public function upload(File $file): void
{
$compressedFile = $this->compress($file);
$fileParts = $this->splitFile($compressedFile);
$this->logger->info('Started uploading to AWS/S3.');
foreach ($fileParts as $filePart) {
$this->awsClient->upload($filePart);
}
$this->logger->info('Finished uploading to AWS/S3.');
}
}
You can even go a step further towards clean-code by coding to an interface, not a concrete implementation (class), and moving each logic to a separated, ensuring even more modular parts and being closer to the Open/Closed Principle
<?php
interface StorageInterface
{
public function save(File $file): void;
}
class AwsStorage implements StorageInterface
{
public function save(File $file): void
{
$compressedFile = $this->compress($file);
$fileParts = $this->splitFile($compressedFile);
$this->logger->info('Started uploading to AWS/S3 started.');
foreach ($fileParts as $filePart) {
$this->awsClient->upload($filePart);
}
$this->logger->info('Finished uploading to AWS/S3 started.');
}
}
class LocalStorage implements StorageInterface
{
public function save(File $file): void
{
$fp = fopen($this->getStoragePath().$file->getName(), 'w');
fwrite($fp, $file->getContent());
fclose($fp);
$this->logger->info('File successfully saved on local storage.');
}
}
And that was it!
I would also be glad to know your opinion about the topic if you consider it from a different point of view.
Happy coding!
Top comments (1)
Nice implementation. I would rather use abstraction where you can abstract those classes into an interface so that testing can be easy.