Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
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....)

share|improve this question

1 Answer 1

up vote 3 down vote accepted

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 :)

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

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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