I fixed a bug yesterday caused by a loop that terminated too early. It took me a while to understand the fact that it was terminating too early, and I realised that I’ve never written this type of bug because of the way that I tend to write code. Basically, the method looked like this:
public void someMethod() {
for ( String key : keys ) {
if (someCondition()) {
return;
}
doSomeProcessingForThe(key);
}
}
As you can see, the innocent looking return statement breaks out of the entire method, and what they really wanted was a continue statement instead. Note that TDD had been applied to this codebase, and it’s just another reminder that driving your code with tests isn’t enough (you also want to think of ensuring you write more comprehensive tests, particularly for loops).
I’m writing this post because I though it’s interesting that I realised a certain coding style removes (and possibly adds) a whole class of bugs. Had I been writing the code to start with, I probably would have avoided the use of the return (and/or continue) statement resulting in the following code:
public void someMethod() {
for ( String key : keys ) {
if (!someCondition()) {
doSomeProcessingForThe(key);
}
}
}
Prevention is easy with tools like checkstyle, PMD or findbugs (and maybe some of them already check for this). If not, I think it’d be easy to add.
There are definitely cases where you’ll want to shortcut the loop in order to avoid the work of going over a number of items that don’t need going over. You could leave that kind of short-cut ’til you find a performance problem, but if you’re looking for, say, the presence of an item matching certain criteria in a collection and that collection is a lazily-instantiated collection from the database using ORM, I might be inclined to use the short-cut.
That doesn’t mean that I disagree with the point, just that I wouldn’t necessarily always avoid shortcut-processing styles.
Geoffrey,
I agree that there are cases where you might want to shortcut a loop, however I think this pattern is less common for something that doesn’t return anything (void). Funnily enough, I tend to shortcut a loop that returns something as I find it often reads better.