If each function is deprecated, how to recover the beauty that this function provides?

Question

The typical algorithm of search until found, in PHP and for some arrays where we can't use the language array search functions (I think), could be implemented in this way:

    $found = false;

    while (list($key, $alreadyRP) = each($this->relatedProducts) && !$found) {
        if ($alreadyRP->getProduct()->getId() === $rp->getProduct()->getId()) {
            $found = true;
        }
    }
    if (!$found) {
        // Do something here
    }

Please, take it as pseudocode, I didn't executed it. What I like about it, is that it gracefully stops if it is found what we are looking for.

Now, due to the "each" function is deprecated, I have to code something like this:

    $found = false;
    foreach ($this->relatedProducts as $alreadyRP) {
        if ($alreadyRP->getProduct()->getId() === $rp->getProduct()->getId()) {
            $found = true;
            break;
        }
    }
    if (!$found) {
        // Do something here
    }

To put a "break" statement inside a "for" loop is ugly in structured programming. Yes, it is optional, but if we avoid it, the "foreach" will go through all the array, which is not the most efficient way.

Any idea to recover the efficiency and structure that "each" gives in this case?

Thank you.

Answer 1

If you're looking for a rewrite that doesn't involve extra variables, you can replace the each call with calls to current and next:

do {
  $found = (current($this->relatedProducts)->getProduct()->getId() === $rp->getProduct()->getId());
} while (empty($found) && false !== next($array));

This is a mechanical translation, and it relies merely on the definition of each (emphasis mine):

Return the current key and value pair from an array and advance the array cursor

It also suffers the same deficiency of the original each version: it doesn't handle empty arrays.

That said, please don't use each, or any of its siblings, for new code. This from a guy who voted "No" on the RFC! Why? Because the performance sucks:

1017$ cat trial.php
<?php
$array = range(0, 999);                                                       

$begin = -microtime(true);                                                    
for ($i = 0; $i < 10000; $i++) {                                              
    reset($array);                                                            
    $target = rand(333, 666);                                                 
    do {                                                                      
        $found = (current($array) === $target);                               
    } while (empty($found) && false !== next($array));                        
}                                                                             
printf("%.5f\n", $begin + microtime(true));                                   

$begin = -microtime(true);                                                    
for ($i = 0; $i < 10000; $i++) {                                              
    $target = rand(333, 666);                                                 
    foreach ($array as $current) {                                            
        if ($current === $target) break;                                      
    }                                                                         
}                                                                             
printf("%.5f\n", $begin + microtime(true));                                   

1018$ php -d error_reporting=-1 trial.php
8.87178
0.33585

That's nearly nine seconds for the next/current version while not even half a second for the foreach version. Just don't.

Answer 2

The beauty of the each() method is in the eye of the beholder, but there are other reasons to prefer foreach, including this interesting bit of information from the RFC that led to the deprecation of each()

The each() function is inferior to foreach in pretty much every imaginable way, including being more than 10 times slower.

If the purpose of the method is to // Do something here if the $rp is not found in $this->relatedProducts, I think a more "beautiful" way to handle it would be to extract the search through related products into a separate method.

protected function inRelatedProducts($id) {
    foreach ($this->relatedProducts as $alreadyRP) {
        if ($alreadyRP->getProduct()->getId() === $id) {
            return true;
        }
    }
    return false;
}

Moving the related products search into a separate method is advantageous because

• It separates that functionality from the original method so that it becomes reusable instead of being tied to whatever // Do something here does.

• It simplifies the original method so it can focus on its main task

  $id = $rp->getProduct()->getId();
  if (!$this->inRelatedProducts($id)) {        
      // Do something here
  }
  

• It simplifies the search code because if it's contained in its own method, you can just return true; as soon as you find a match, so you won't need to break, or to keep track of a $found variable at all.

---

On the other hand, if this was my project I would be looking for a way to remove the need for this method by populating $this->relatedProducts so that it's indexed by ID (assuming ID is unique there) so the determination could be reduced to

$id = $rp->getProduct()->getId();
if (isset($this->relatedProducts[$id])) { ...

Answer 3

It looks like each is basically a version of current() and next()

http://php.net/manual/en/function.current.php

http://php.net/manual/en/function.next.php

each() gives the current array item, and moves to the next index.

current() gives the current array item, but doen't increment the index.

So, you can replace each() with current(), and inside your foreach use next() to shift the index up

next() gives the next item, and increments the index.

while (list($key, $alreadyRP) = current($this->relatedProducts) && !$found) {
    if ($alreadyRP->getProduct()->getId() === $rp->getProduct()->getId()) {
        $found = true;
    }
    next($this->relatedProducts);
}