1
\$\begingroup\$
public function getStuff($brand)
{
    $web=FALSE;
        if($this->getWebcorners()):
            foreach ($this->getWebcorners() as $webcorner):
                if(strtolower($webcorner->getBrand()->getName())== $brand):
                    return $webcorner;
                endif;
                if($webcorner->getBrandId()==NULL):
                    $web=$webcorner;
                endif;
            endforeach;
        endif;
    return $web;
}

the function is suppose to list a collection of webcorners and return the first that matches a brand, then the one that is null (in this order). There's a possibility to have many other webcorners according to the brand (but no matching the one that is passed by parameter to the function).

\$\endgroup\$
1
\$\begingroup\$

Just cleaned up your method a little: (Guard condition, better variable name, less nesting)

public function getStuff($brand)
{
    if(!$this->getWebcorners()) return false;
    $fallback=false;
    foreach ($this->getWebcorners() as $webcorner) {
        if(strtolower($webcorner->getBrand()->getName())== $brand) return $webcorner;
        if($webcorner->getBrandId()==NULL) $fallback=$webcorner;
    }
    return $fallback;
}

If you want to return the first item without a brand you have to add the $fallback==false condition to your second if. Otherwise you use the last suitable item.

\$\endgroup\$
  • \$\begingroup\$ I'm not too convinced by the if(!$this->getWebcorners()) return false; as it adds duplicated code without improving anything as far as I can tell. \$\endgroup\$ – SylvainD Apr 12 '13 at 10:56
  • 1
    \$\begingroup\$ @Josay Not everybody likes this guard conditions, it's a matter of taste. The big advantage is that you get rid of all the additional nesting levels which leads to better readability, in my opinion. It's not that much in this example but in the case you can extract 3-4 guards you will see the benefit. In addition to that you can easily identify the special cases of a methods and don't have to thing about the else-branches. \$\endgroup\$ – mheinzerling Apr 12 '13 at 11:07
  • \$\begingroup\$ Thx both I kinda like the version of mnhg, for the readability, but I'don't like so much all of these returns perhaps offering too much leak, or unpredictable side effects...I spend quite a lot of time figure out alternativ solution but nothing came... \$\endgroup\$ – krifur Apr 12 '13 at 13:08
  • \$\begingroup\$ If you rely on the metric to write short methods, with a limited amount of work done, it's pretty easy to address this issue and you can easily put your method under test. If you run some code coverage tool along with your test you will see the next benefit of clear and simple guard conditions, as you can test this branches first and don't have to bother in your "real" tests. \$\endgroup\$ – mheinzerling Apr 14 '13 at 6:57
  • 1
    \$\begingroup\$ @mnhg Rule of thumb in PHP (and many other languages): Return early, keep your code on the left side. But I would set the return statements on a separate line, so they are easier to see. \$\endgroup\$ – fuxia Apr 14 '13 at 8:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.