Closures power and concise style vs readability and maintainability?

Question

Is large use of closures actually a bad thing for readability and maintainability of the code? For example, consider the following code with (comments removed on purpose):

$withMobile  = function($c) { return $c->getMobile() !== null; };
$extractAll  = function($c) { return $c; };
$extractId   = function($c) { return $c->getId(); };

$allUniqueCustomers = $data->customers->filter($withMobile)->map($extractAll);
$customersIds       = $allUniqueCustomers->filter($withMobile)->map($extractId)->toArray();

$isNew = function($c) use($customersIds) { return !in_array($c->getId(), $customersIds); };

foreach($data->tags as $tag) :
    foreach($tag->getCustomers()->filter($isNew)->filter($withMobile) as $newCustomer) :
        $allUniqueCustomers->add($newCustomer);
    endforeach;
endforeach;

$sanitize       = function($m) { return preg_replace("/^(\+|00)/", '', $m); };
$extractMobile  = function($c) use($sanitize) { return $sanitize($c->getMobile()); };

// Unique sanitized phone numbers of customers merged with customers from tags
$customersMobiles = $allUniqueCustomers->map($extractMobile);

It looks good to me. But i love closures.

Would you consider this a nightmare to maintain and change in future in respect of traditional more verbose programming with loops?

Answer 1

Lambda/anonymous functions and closures are not sophisticated enough in PHP to really be reliable at this moment, and probably wont be any time soon, if ever. They are usually too bulky to be worthwhile and typically can be replaced with a normal function or loop and be easier to read and quicker to process.

The most common way to use a lambda function, and about the only way I find acceptable, is as a callback function, which is typically called in line. But that's a matter or preference. There are other cases where they are acceptable, but, as traditional closures as seen in other languages, they fall short.

If you are planning on using a lambda function or closure more than once, then you have just defeated their purpose. As Mahn pointed out, these functions are not compiled at runtime, instead they are compiled on each use. This makes running them costly. At this point you might as well have just created a traditional function. Which, by the way, is essentially what you are doing anyways by defining them as variables before using them, only backwards. The only difference is that you are running that function twice instead of once and defining it for use in the local scope only, which is the same thing as defining a function within another function, which most people consider bad taste.

While these examples can be followed easily enough, I don't think that makes true usages "easy" to read. Sure you can figure out what's going on, but the longer they are the more difficult it becomes.

Would I consider this a nightmare? No. Would I thank you for doing this? No. In my opinion it is much better to follow a sequence of functions than read through a list of lambda functions. And no offense to Mahn, but his I would find a nightmare trying to maintain. Mostly due to his indentations though...

Also, as Mahn pointed out, why are you declaring your foreach loops that way? Traditionally that format is only used in templates.

Answer 2

It's not that bad — one can quickly grasp what it is doing and where is it going, which should be good news. I would personally recommend however the following when dealing with closures:

• If the closure is only to be used once, hardcode it into the arguments. That way it's a bit easier to follow, since the logic of the closure is right there to be read instead of having to track down the variable to see what it does.
• If the closure is to be used multiple times, consider rewriting it into a "full blown" function of the application instead, it will be easier to follow since the function is not being created on the fly and easier to maintain since it's part of the official API, so to speak.

Here's the possible alternative I would suggest:

$allUniqueCustomers = $data->customers->filter(
                                                function($c) { 
                                                    //Filter by mobile
                                                    return $c->getMobile() !== null; 
                                                }
                                              )
                                        ->map(
                                                function($c) { 
                                                    //Get all
                                                    return $c; 
                                                }
                                            );


$customersIds = $allUniqueCustomers->filter(
                                            function($c) { 
                                                    //Filter by mobile
                                                    return $c->getMobile() !== null; 
                                                }
                                            )
                                   ->map(
                                            function($c) { 
                                                //Extract by id
                                                return $c->getId(); 
                                            }
                                        )
                                   ->toArray();

foreach($data->tags as $tag) :
    $newCustomers = $tag->getCustomers()->filter(
                                            function($c) use($customersIds) { 
                                                //Filter by new customers with mobile
                                                return ((!in_array($c->getId(), $customersIds)) && ($c->getMobile() !== null)); 
                                            }
                                        );

    foreach($newCustomers as $newCustomer) :
       $allUniqueCustomers->add($newCustomer);
    endforeach;

endforeach;

// Unique sanitized phone numbers of customers merged with customers from tags
$customersMobiles = $allUniqueCustomers->map(
                                                function($c) { 
                                                    //Get mobile and sanitize
                                                    return preg_replace("/^(\+|00)/", '', $c->getMobile());
                                                }
                                        );

I would also advise against using the alternative syntax you have on the foreach in your application logic and save it for exclusively templating/mixing html with php only, but I left it there untouched since I suppose that's a matter of taste.