Refactoring

or how I learned not to repeat myself

The saga of my inherited legacy C++ application continues.

I had occasion last week to rewrite a dialog procedure. We needed some extra functionality but what I found along the way amazed me.

The dialog in question has the standard OK and Cancel buttons. It also has two listview controls. The listview controls are used to input and display information for 48 individual but identical entities. 24 on the left and 24 on the right. Interaction is done by double clicking on an item or by right clicking on an item. In the code there are four event handlers, one each for double clicks on the listview and one each for right clicks as aforesaid.

I said above that the listview controls (plural) are used to input and display information for 48 individual but identical entities. So it follows, does it not, that the actions in each of the pairs of event handlers are going to be almost identical, varying only in which listview is acted upon and the indexed item clicked upon. Right? Which not terribly obscure insight would lead the intelligent programmer to write a private, generic, class member function, called from each event handler, passing a reference to the listview control and an index specifying the base index.

Not in this code! Instead we had two complete copies of the same 100+ line function where the only thing that varied was the instance of the listview control and the base index.

Yikes! Fortunately it was pretty easy to collapse all that repeated code into two private functions. I was able to cut about 500 lines of code to less than 200.

What bothers me though is how anyone could have thought this was a good way to go. I can understand that the original function was written and tested on one listview and, once the bugs had been ironed out, extended. But it beggars belief that anyone could have imagined the best way to extend was to copy and paste! And I'm being generous. The 48 seperate entities are directly mapped onto 48 separate entities in our hardware. Now it seems to me that if you're designing a half million dollars worth of hardware the basic specs for that hardware are going to be mapped out before you start writing code. Details will change over the development cycle but one thing that won't change late in the development cycle is the number of entities. Thus, the guy who wrote the code I've inherited already knew that there would be 48 entities to control. A 2 * 24 layout fits much more naturally onto our normal CRT displays than a 1 * 48 layout. So if he knew, before he started, that he'd be mapping those 48 entities onto 2 listviews why on earth didn't he write the code to be more generalised in the first place!

I'm not even concerned about the compiled code duplication (though that's a concern). How about maintainability? If I find a bug in one event handler I have to make the same changes in the other event handler. Heck, I even have to worry about whether the code in the first event handler matches the code in the second one.

No, as far as I'm concerned this was pure laziness. My predecessor got the code kinda working in one event procedure and then couldn't be bothered to generalise the solution. It was easier to use the class wizard to add a second event handler, copy and paste the code and then change the listview reference to the second listview. Bugger the future! Bugger bug fixes (and there were a few along the way).

I know there will be future surprises like this.