It is an argument, not a trojan horse!

#php #oop

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.');
    }
}