DEV Community

Cover image for It is an argument, not a trojan horse!
medunes
medunes

Posted on • Originally published at blog.medunes.net

6 4

It is an argument, not a trojan horse!

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?

image

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.');
        }
    }
}
Enter fullscreen mode Exit fullscreen mode

Ok, can you tell me what's wrong with that thing above? I can tell you my point of view:

image

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

    }
}
Enter fullscreen mode Exit fullscreen mode

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.');
    }
}
Enter fullscreen mode Exit fullscreen mode

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)

Collapse
 
olasunkanmi profile image
Oyinlola Olasunkanmi

Nice implementation. I would rather use abstraction where you can abstract those classes into an interface so that testing can be easy.

👋 Kindness is contagious

Discover a treasure trove of wisdom within this insightful piece, highly respected in the nurturing DEV Community enviroment. Developers, whether novice or expert, are encouraged to participate and add to our shared knowledge basin.

A simple "thank you" can illuminate someone's day. Express your appreciation in the comments section!

On DEV, sharing ideas smoothens our journey and strengthens our community ties. Learn something useful? Offering a quick thanks to the author is deeply appreciated.

Okay