clean-code-php: "Bad" example for "Functions should do one thing" has a bug

@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.

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.

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.

@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.

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

I think that’s good idea