1
\$\begingroup\$
private function getFeatureByParam ($classname, $module, $action) {
    $needFeature = array();
    foreach ($this->featureConf as $featureIndex => $feature) {
        if ($feature['classname'] == $classname && 
            $feature['module'] == $module && 
            $feature['action'] == $action) {
                $needFeature = $feature;
                break;
        }
    }
    return $needFeature;
}
private function getFeatureByParam ($classname, $module, $action) {
    foreach ($this->featureConf as $featureIndex => $feature) {
        if ($feature['classname'] == $classname && 
            $feature['module'] == $module && 
            $feature['action'] == $action) {
                return $feature;
        }
    }
    return array();
}

The first style of writing is more understandability.

The second style of writing is more clean and efficient.

But witch one is more better? (Or they are too bad....)

\$\endgroup\$
3
\$\begingroup\$

Personally, option 2.

In option 1, by breaking out of the loop, you have to look down until after the end to see what happens, just to see a return. In option 2, it's obvious what you want and when you get it. Although, I think the condition is a little complex.

There are, of course, other options:

private function getFeatureByParam ($classname, $module, $action) {
    foreach ($this->featureConf as $featureIndex => $feature) {
        if ($feature['classname'] != $classname) continue;
        if ($feature['module'] != $module) continue;
        if ($feature['action'] != $action) continue;

        return $feature;
    }
    return array();
}

private function getFeatureByParam ($classname, $module, $action) {
    foreach ($this->featureConf as $featureIndex => $feature) {
        if ( featureMatches( $feature, $classname, $module, $action ) ) {
            return $feature;
        }
    }
    return array();
}

I'm inclined to option 4 :)

\$\endgroup\$
3
  • \$\begingroup\$ Wow,i had never thought the writing style of option 4. It is more clean and packaging the matching operation.Thank you very much~ \$\endgroup\$ – bloody numen Jan 6 '13 at 7:07
  • \$\begingroup\$ My only issue with this answer is your braceless syntax. But that is more of a point of preference. \$\endgroup\$ – mseancole Jan 7 '13 at 17:44
  • \$\begingroup\$ I only have braceless when it's a precondition-style break, continue, or return. Anything else gets one - it's too dangerous not to. \$\endgroup\$ – Glenn Rogers Jan 7 '13 at 20:13

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.