Interesting List bug
I found an interesting bug in the code at work the other day. The task involved looping through a list and creating a new list of items generated from the input list’s items. The core problem was that the implementation didn’t create a new list, but added to the original, i.e. (not the real code but the same pattern):
List list = generateInitialList();
for (int i = 0; i < list.size(); i++) {
MyClass thing = (MyClass)list.get(i);
MyOtherClass newThing = createMyOtherClassFromMyClass(thing);
list.add(newThing);
}
[/java]
This is interesting for a few reasons. It's bad in that it uses get(index) instead of using an iterator; in this case we’re using a (rather weak) implementation of quasi-type-safe lists that subclass ArrayList (old pre-JDK5 codebase) so that’s safe, but it’s a dangerous Java 1.1-style antipattern that would be potentially expensive if the list were a LinkedList.
The cool part about this is how it fails. It loops through the N elements in the list, and since it’s adding to the original list, the list is getting larger. It calls size() every time, so unless it’s stopped by an exception (e.g. if there were no casting) it would go on forever. Of course at the N+1st iteration it fails since it tries to cast the first MyOtherClass to a MyClass, and that dies with a ClassCastException.
If he’d used an iterator, it would have failed earlier since by adding to a list that’s being iterated, he’d get a ConcurrentModificationException.
And if he’d continued with the get(index) antipattern but called size() once and stored it in a temporary variable, e.g.
for (int i = 0, size = list.size(); i < size; i++) {
MyClass thing = (MyClass)list.get(i);
MyOtherClass newThing = createNewThingFromThing(thing);
list.add(newThing);
}
[/java]
then it would have looped through all N items, created N new MyOtherClass instances, and then it would have failed on a ClassCastException in the calling code that iterated through this list’s items since that code expects a list containing only MyOtherClass instances.
Of course this would have never made it into the code base if it had been even trivially unit tested (his initial lists must have been empty, I suppose) but that’s another topic.
