CodeSOD: Insertion

Shalonda inherited a C# GUI application that, if we're being charitable, has problems. It's slow, it's buggy, it crashes a lot. The users hate it, the developers hate it, but it's also one of the core applications that drives their business, so everyone needs to use it.

One thing the application needs to do is manage a list of icons. Each icon is an image, and based on user actions, a new icon might get inserted in the middle of the list. This is how that happens:

 /// <summary> /// Inserts an object (should be an Image) at the specified index /// </summary> /// <param name="index">the index where to add the object at</param> /// <param name="val">the object (an Image) to be added to this ImageCollection</param> public void Insert(int index, object val) { object[] icons = new object[this.list.Count]; object[] newicons = new object[this.list.Count + 1]; for (int i = 0; i < newicons.Length; i++) { if (i != index) newicons[i] = icons[i]; else newicons[i] = val; } this.list.Clear(); this.list.AddRange(newicons); }

A comment like "Inserts an object (should be an Image)", in a strongly typed language, is always a bad sign. And sure enough, the signature of this method continues the bad signs: object val. Insert, in this case, is not a method defined in an interface, there's no reason for it to be val here other than the fact that the original developer probably had a snippet for handling inserting into lists, and just dropped it in.

Then we create two new arrays, both of which are empty. Then we iterate across those empty arrays, copying emptiness from the first one into the second one, except when we're at the index supplied by the caller, in which case we put our input value into the array.

Fortunately, at this point, our program is guaranteed to throw an exception, since icons has one fewer index than newicons, and they're both being indexed by i, which goes up to newicons.Length. This is actually the best case for this block of code, because of what comes next: it empties the list and then inserts the newicons (which is all nulls except for one position) into the list.

List, in this case, is an ArrayList, which definitely doesn't already have an insert method which actually does what it's supposed to.

Shalonda replaced this method with a call to the built-in Insert. This stopped the exceptions from being thrown, but that in turn uncovered a whole new set of bugs that no one had seen yet. As the song goes: "99 little bugs in the tracker, 99 little bugs. Take one down, patch it around, 175 bugs in the tracker!"

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!