"Bad" example for "Functions should do one thing" has a bug

[piotrplenik]

"Bad" example for "Functions should do one thing" has a bug

[dmytro-y-dev]

Honestly, the good example is a debug hell. Imagine that these 3 functions are in different files, it won't be easy for you to jump between them. The provided "bad" code is easier to extend and understand than good one, though it is also broken.

bad code

function emailClients($clients) {
    foreach ($clients as $client) {
        $clientRecord = $db->find($client);

        if ($clientRecord->isActive()) {
            email($client);
        }
    }
}

vs

good code

so called "Baklav code" in analogy to spaghetti code (https://www.johndcook.com/blog/2009/07/27/baklav-code/)

function emailClients($clients) {
    $activeClients = activeClients($clients);
    array_walk($activeClients, 'email');
}

function activeClients($clients) {
    return array_filter($clients, 'isClientActive');
}

function isClientActive($client) {
    $clientRecord = $db->find($client);
    return $clientRecord->isActive();
}

In a Good example, we will need to pass the dependency through 3 functions.
This is very bad.

emailClients tries to be a good shim, but in the end it wires dependencies too hard by providing a name of global function to array_walk. Also, it has 2 responsibilities : 1. Filter active clients 2. Email active clients. First responsibility should be moved away.

[SahinU88]

Just curious, wouldn't it be better to call the function emailActiveClients?
In the end, if I see the function emailClients I wouldn't assume right away, that there is some filtering going on.

And the $db instance could be a class which is accessible in a global way, like some Frameworks do: DB::find( $client ) which would resolve the issue of passing an extra argument.

[dmytro-y-dev]

@SahinU88
Actually, if you will through popular Laravel libraries (e.g. Sluggable trait https://github.com/cviebrock/eloquent-sluggable 3.1k stars, https://github.com/cviebrock/eloquent-sluggable/blob/master/src/Services/SlugService.php), you will see something like next:

class A {
/** @var Model */
private $model;

public function __construct(Model $model) {
  $this->model = $model;
}

public function all() {
  return $this->model->all();
}
}

...

$mymodel = new Client();
$a = new A($mymodel);

You can avoid using Client from facade if you need to generalize behavior. Also, if you use PHPStorm, it will show you warnings about usage of Client methods from facade. It is ok to use Client from facade in controllers/commands (i.e. on top-top level of execution) or for fast prototyping, but when you go deeper into classes that do real job, you should really use interfaces and Model to make your code more reusable and testable.

[TomasVotruba]

Just curious, wouldn't it be better to call the function emailActiveClients?

I think that's good idea

[dmytro-y-dev]

@peter-gribanov

Don't use a Singleton pattern

This requires as well some explanation in Readme.md why it is antipattern. There are several problems with it when it is used improperly. See https://stackoverflow.com/a/138012/2678487 vs https://jorudolph.wordpress.com/2009/11/22/singleton-considerations/, but I would better refer to real literature like GoF's book (sorry, can't find it on the Internet).

Valid case for singleton that I can think of is DB connection pools. Also, common ORM implementations (Hibernate, Doctrine) have significant global state, as they keep $connection resource and tables of changed entities. This is not such a big deal when you have single-threaded application, but what if you have 10-100 threads that use same global DB instance in parallel with modifying operations? You need really solid code to handle connection and ops in a thread safe manner, as you likely will reuse same DB connection between threads for performance reasons.

In general singletons are easy to abuse, but in some cases it is a useful pattern.

@SahinU88

Just curious, wouldn't it be better to call the function emailActiveClients?

Absolutely, it would. M.b. emailOnlyActiveClients, because it receives array of ids of any clients, so name would assume that only active clients from provided list will be emailed. However, emailActiveClients is also good name and it is shorter. emailClients is too generic.

[TomasVotruba]

Thanks for reporting
Could you send PR to fix that please?

I think the 2nd case looks to simplest to explain the example.

[MacDada]

@TomasVotruba Which fix version? ;p

[MacDada]

Mind that the "good" example must be fixed as well.

[peter-gribanov]

@TomasVotruba It's necessary to change the whole example entirely.
In a Good example, we will need to pass the dependency through 3 functions.
This is very bad.

[TomasVotruba]

This issue is about fixing broken code only. For changing the code entirely, please open new issue/PR to prevent mixing many things at once.

[peter-gribanov]

@TomasVotruba issue already exists #38

[TomasVotruba]

Great

[dmytro-y-dev]

(are you good?)

function emailClients($clients, Mailer $mailer, Database $db) {
    $activeClients = activeClients($clients, $db);
    
    array_walk($activeClients, function($client) {
        $mailer->email($client);
    });
}

function activeClients($clients, Database $db) {
    return array_filter($clients, function($client) use ($db) {
        return isClientActive($client->isActive(), $db);
    });
}

function isClientActive($client, Database $db) {
    $clientRecord = $db->find($client);
    return $clientRecord->isActive();
}

function main()
{
    $clients = [1, 2, 3];
    $mailer = MailerFactory::createNiceMailer(); // Or reach me out via Dependency Injection Container
    $db = DatabaseFactory::createNiceDb();

    emailClients($clients, $mailer, $db);
}

vs (are you bad?)

function emailClients($clients, Mailer $mailer, Database $db) {
    foreach ($clients as $client) {
        $clientRecord = $db->find($client);

        if ($clientRecord->isActive()) {
            $mailer->email($client);
        }
    }
}

function main()
{
    $clients = [1, 2, 3];
    $mailer = MailerFactory::createNiceMailer(); // Or reach me out via Dependency Injection Container
    $db = DatabaseFactory::createNiceDb();

    emailClients($clients, $mailer, $db);
}

vs (are you mad?)

function filterActiveClients($clients_ids, Database $db) {
    // Please, don't reimplement something that 99% of ORMs can do
    // See https://en.wikipedia.org/wiki/Inner-platform_effect and know your tools.

    return $db
        ->find(Client::class)
        ->where()
            ->field('active', true)
            ->and()->field('id', 'in', $clients_ids)
        ->endwhere()
        ->all()
    ;
}

function emailClients($clients, Mailer $mailer) {
    foreach ($clients as $client) {
        $mailer->email($client);
    }
}

function main()
{
    $clients_ids = [1, 2, 3];
    $mailer = MailerFactory::createNiceMailer(); // Or reach me out via Dependency Injection Container
    $db = DatabaseFactory::createNiceDb();

    $clients = filterActiveClients($clients_ids, $db);
    emailClients($clients, $mailer);
}

[MacDada]

Just curious, wouldn't it be better to call the function emailActiveClients?
In the end, if I see the function emailClients I wouldn't assume right away, that there is some filtering going on.

@SahinU88 Yep.

And the $db instance could be a class which is accessible in a global way, like some Frameworks do: DB::find( $client ) which would resolve the issue of passing an extra argument.

@SahinU88 Out of the frying pan into the fire…

[peter-gribanov]

And the $db instance could be a class which is accessible in a global way, like some Frameworks do: DB::find( $client ) which would resolve the issue of passing an extra argument.

@SahinU88 Don't use a Singleton pattern

[SahinU88]

@MacDada / @peter-gribanov
The singleton-pattern could also be a service container which is also used in frameworks.
For example in Laravel, you would use something like:

use App\Client; // client is the Eloquent-Model-Repo
...

$clients = Client::all();

I know the example is a little far fetched, but I hope you get the idea.

[peter-gribanov]

@metamaker I agree. Description Singleton incomplete.
This section was created chaotically. It needs to be improved.

You can make PR and expand the description.

PS: Singleton was added to GoF and only later was recognized as an anti-pattern.

[TomasVotruba]

Surpassed by #38