The example I used in my previous post is a highly simplified case I found in actual code. If differs from the code I encountered in a couple of major ways, it's simplified, it deals in test classes not the domain classes from the example (which are not relevant here nor which I am at liberty to post) and that it doesn't contain a bug I find mildly amusing and somewhat informative. The bug is illustrated here:

public static BusinessObject FindByCriteria(IEnumerable<BusinessObject> businessObjects, DecisionCriteria decisionCriteria)
{
    if (businessObjects == null)
    {
        throw new ArgumentNullException("businessObjects");
    }
    if (decisionCriteria == null)
    {
        throw new ArgumentNullException("decisionCriteria");
    }

    BusinessObject result = null;

    foreach (BusinessObject businessObject in businessObjects)
    {
        if (result == null && businessObject.MatchesCriteria(decisionCriteria))
        {
            result = businessObject;
        }
    }

    return result;
}

Keen observers of this example and my previous post (all none of you) will see that the if clause within the foreach has been modified slightly and the break is gone. The method still returns the result as expected. But it's not quite right. Once the expected instance has been found there's no need to continue. We have our result and it's not going to change. The method should end at this point. This is not the behaviour of the above. Instead we continue to loop through every remaining item in the collection.

This is a relatively trivial error but I'm firmly of the opinion that letting the little things slide leads to the big things getting out of hand. If the collection contains a lot of items we will burn a lot of cycles uselessly looping through it. If we express the logic differently then the behaviour gets worse:

public static BusinessObject FindByCriteria(IList<BusinessObject> businessObjects, DecisionCriteria decisionCriteria)
{
    if (businessObjects == null)
    {
        throw new ArgumentNullException("businessObjects");
    }
    if (decisionCriteria == null)
    {
        throw new ArgumentNullException("decisionCriteria");
    }

    BusinessObject result = null;

    foreach (BusinessObject businessObject in businessObjects)
    {
        if (businessObject.MatchesCriteria(decisionCriteria) && result == null)
        {
            result = businessObject;
        }
    }

    return result;
}

This small change to the if statement ordering seems logically insignificant. However this now ensures that the MatchesCriteria method will be invoked on every item in the collection. In this example this may be trivial but this is not always the case. The code is potentially doing significantly more work that is justified.

It is possible that some of the checks have side effects and therefore need to be executed on every object in the collection. If this is the case then an implementation as above would be sub-optimal on a number of levels. This would be coupling two separate operations (apply logic, find instance) which is poor practice in itself. Additionally if the side effect having operation is invoked as part of an if clause changes to the clause may affect whether the logic is applied. This could manifest as a particularly nasty bug that is very difficult to track down. The multiple operations should be implemented separately.

In summary the problem with the method above is that it doesn't stop when it's finished. The implementor likely just forgot the break statement in this case but the use of a single exit point has led the possibility of this defect. Methods should be more efficient. Make sure yours admit when they're done.