TURBU Tech

What’s wrong with this code?

[code lang="Delphi"]
procedure TMyCustomChecklistPopupControl.ClosePopup;
var
  i: integer;
begin
  inherited ClosePopup;
  FInternalItemStringList.Clear;
  for i := 0 to Self.CheckedCount - 1 do
    FInternalItemStringList.Add(Self.CheckedItems[i].Name);
end;
[/code]

At first glance, it looks just fine. It’s semantically correct–it will do what you want it to. If you happen to have seen a certain issue before, something might jump out at you, but if not, you probably think this is OK. And most of the time, it is.

This is a simplified version of something I ran into at work today, in one of our custom controls. I ran into it in the debugger, but not because it was raising exceptions or corrupting data. No, the problem was that when I hit the Check All button, selecting all 200 or so items, and then closed the popup, it took left the UI unresponsive for a good 15 seconds or so.

Turns out the problem isn’t in what this code was written to do, but in what else it does. You see, there’s an OnUpdate event handler attached to the internal TSwissArmyKnife TStringList which goes over the data in the list, calculates a few things, and updates some UI elements. And yeah, you want that to happen when you make a change. But you want it to happen once per change, from the user’s perspective. This was happening once per change from the TStringList’s perspective, or in other words, 200+ times in total for a single user action. And it took forever to finish.

You can be a really good programmer and still not know all the ins and outs of the framework you’re working with. I’m always discovering new little details about how things work. Turns out I’ve seen this one before, so when I hit Pause a few seconds in and dropped to the debugger, and saw the following right in the middle of the call stack, I knew what was going on right away.

TStringList.Changed
TStringList.InsertItem
TStringList.AddObject
TStringList.Add

What whoever coded this control apparently didn’t know, probably because they’d just never run across it before, was that Borland anticipated this very problem–or more liklely, because so many VCL classes use TStrings descendantes internally, they ran into it themselves at one point–and put a little switch into TStrings to turn off the OnChanged event handler temporarily.

Once I surrounded this code with a BeginUpdate and EndUpdate pair, the delay on closing up the box went from an angonizing 15 seconds to a tiny fraction of a second that I wouldn’t have noticed at all if I wasn’t watching for it.

Hopefully most of the people reading this are familiar with BeginUpdate and EndUpdate. But if anyone who hasn’t seen it runs across this, now you have a new trick. Please make sure to use it, to spare your end-users some pain. Even if you don’t think it’s likely to be necessary, please use it anyway. When this special checklist control was originally written, years ago, it was intended to hold a dozen or so items at most, not hundreds, and it probably performed fine at that scale. But growing client demand means the app’s working with more data than it used to, and eventually you hit something like this unless you’re careful in your design.