New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
MacDada opened this Issue Sep 5, 2017 · 17 comments

Comments

Projects
None yet
5 participants
@MacDada

MacDada commented Sep 5, 2017

https://github.com/jupeter/clean-code-php#functions-should-do-one-thing

Notice: Undefined variable: db on line 4

I can see 3 ways to fix it:

// OOP: it is a function of an object (a method)
function emailClients($clients) {
    foreach ($clients as $client) {
        $clientRecord = $this->db->find($client);
        if ($clientRecord->isActive()) {
            email($client);
        }
    }
}
// $db is passed as an argument
function emailClients($clients, $db) {
    foreach ($clients as $client) {
        $clientRecord = $db->find($client);
        if ($clientRecord->isActive()) {
            email($client);
        }
    }
}
// the function is a closure
$emailClients = function ($clients) use ($db) {
    foreach ($clients as $client) {
        $clientRecord = $db->find($client);
        if ($clientRecord->isActive()) {
            email($client);
        }
    }
};

See also #38

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba Sep 5, 2017

Collaborator

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

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

Collaborator

TomasVotruba commented Sep 5, 2017

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

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

@MacDada

This comment has been minimized.

Show comment
Hide comment
@MacDada

MacDada Sep 5, 2017

@TomasVotruba Which fix version? ;p

MacDada commented Sep 5, 2017

@TomasVotruba Which fix version? ;p

@MacDada

This comment has been minimized.

Show comment
Hide comment
@MacDada

MacDada Sep 5, 2017

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

MacDada commented Sep 5, 2017

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

@peter-gribanov

This comment has been minimized.

Show comment
Hide comment
@peter-gribanov

peter-gribanov Sep 5, 2017

Contributor

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

Contributor

peter-gribanov commented Sep 5, 2017

@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 comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba Sep 5, 2017

Collaborator

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.

Collaborator

TomasVotruba commented Sep 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@peter-gribanov

peter-gribanov Sep 5, 2017

Contributor

@TomasVotruba issue already exists #38

Contributor

peter-gribanov commented Sep 5, 2017

@TomasVotruba issue already exists #38

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba Sep 5, 2017

Collaborator

Great

Collaborator

TomasVotruba commented Sep 5, 2017

Great

@metamaker

This comment has been minimized.

Show comment
Hide comment
@metamaker

metamaker Sep 5, 2017

Honestly, the good example is debug hell (imagine that these 3 functions are in different files, is that easy for you to jump between them, hey?). The provided "bad" code is easier to extend and understand than good one, though it is also broken. Also, look at https://www.johndcook.com/blog/2009/07/27/baklav-code/.

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

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

vs

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.

End result of improper decomposition. emailClients tries to be good shim, but in the end it wires dependencies too hard (providing name of global function to array_walk, serious?). Also, it has 2 responsibilities : 1. Filter active clients 2. Email active clients. First responsibility should be moved away.

metamaker commented Sep 5, 2017

Honestly, the good example is debug hell (imagine that these 3 functions are in different files, is that easy for you to jump between them, hey?). The provided "bad" code is easier to extend and understand than good one, though it is also broken. Also, look at https://www.johndcook.com/blog/2009/07/27/baklav-code/.

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

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

vs

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.

End result of improper decomposition. emailClients tries to be good shim, but in the end it wires dependencies too hard (providing name of global function to array_walk, serious?). Also, it has 2 responsibilities : 1. Filter active clients 2. Email active clients. First responsibility should be moved away.

@metamaker

This comment has been minimized.

Show comment
Hide comment
@metamaker

metamaker Sep 5, 2017

(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) {
    // Don't ever try to reimplement something that 99% of ORMs can do
    // See https://en.wikipedia.org/wiki/Inner-platform_effect
    // LEARN YOUR FREAKING 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);
}

metamaker commented Sep 5, 2017

(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) {
    // Don't ever try to reimplement something that 99% of ORMs can do
    // See https://en.wikipedia.org/wiki/Inner-platform_effect
    // LEARN YOUR FREAKING 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);
}
@SahinU88

This comment has been minimized.

Show comment
Hide comment
@SahinU88

SahinU88 Sep 6, 2017

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.

SahinU88 commented Sep 6, 2017

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.

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba Sep 6, 2017

Collaborator

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

I think that's good idea

Collaborator

TomasVotruba commented Sep 6, 2017

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

I think that's good idea

@MacDada

This comment has been minimized.

Show comment
Hide comment
@MacDada

MacDada Sep 6, 2017

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…

MacDada commented Sep 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@peter-gribanov

peter-gribanov Sep 7, 2017

Contributor

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

Contributor

peter-gribanov commented Sep 7, 2017

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

@metamaker

This comment has been minimized.

Show comment
Hide comment
@metamaker

metamaker Sep 7, 2017

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

Another point is that in case of DB, usual implementations have significant global state (they keep $connection resource and tables of changed entities, in case of ORMs), so if some component disconnected that globally stored connection or state of entities changed, other component can suck hard. This is not such a big deal when you have single-threaded application (why not to reconnect later or flush changes, yeah?), but what if you have 10 threads that use same global DB instance in parallel with state changing operation? You will not be able to guarantee thread safety before you refactor that singleton.

So, in general, singletons are easy to abuse, and this is why you need to learn how to use them properly. It is useful pattern, but too easy to use and therefore too often used improperly.

@SahinU88

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

Absolutely, it should be. M.b. I would call it 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, imho.

metamaker commented Sep 7, 2017

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

Another point is that in case of DB, usual implementations have significant global state (they keep $connection resource and tables of changed entities, in case of ORMs), so if some component disconnected that globally stored connection or state of entities changed, other component can suck hard. This is not such a big deal when you have single-threaded application (why not to reconnect later or flush changes, yeah?), but what if you have 10 threads that use same global DB instance in parallel with state changing operation? You will not be able to guarantee thread safety before you refactor that singleton.

So, in general, singletons are easy to abuse, and this is why you need to learn how to use them properly. It is useful pattern, but too easy to use and therefore too often used improperly.

@SahinU88

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

Absolutely, it should be. M.b. I would call it 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, imho.

@SahinU88

This comment has been minimized.

Show comment
Hide comment
@SahinU88

SahinU88 Sep 7, 2017

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

SahinU88 commented Sep 7, 2017

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

@metamaker

This comment has been minimized.

Show comment
Hide comment
@metamaker

metamaker Sep 7, 2017

@SahinU88
Actually, if you will go through popular Laravel libraries (e.g. Sluggable trait https://github.com/cviebrock/eloquent-sluggable, 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);

So, 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 use interfaces and Model to make your code more reusable and testable. Laravel sometimes does pretty bad job in explaining how to use framework in good way. Hope, this makes some sense, I am not the best at explanations 😄 .

metamaker commented Sep 7, 2017

@SahinU88
Actually, if you will go through popular Laravel libraries (e.g. Sluggable trait https://github.com/cviebrock/eloquent-sluggable, 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);

So, 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 use interfaces and Model to make your code more reusable and testable. Laravel sometimes does pretty bad job in explaining how to use framework in good way. Hope, this makes some sense, I am not the best at explanations 😄 .

@peter-gribanov

This comment has been minimized.

Show comment
Hide comment
@peter-gribanov

peter-gribanov Sep 7, 2017

Contributor

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

Contributor

peter-gribanov commented Sep 7, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment