I’m currently working on a bit of legacy code. Good code, to be sure, with a rather low technical debt due to the care and hard work by previous programmers. But of course, there are things that stick out.
An hour or so ago, I started to refactor an old method that was populating various bits and pieces of the form in question. That old method had far too many responsibilities, so I started by extracting a couple of responsibilities, two list views as it where.
You know the drill populating list boxes. You typically have a list of things that you loop through and add them to the list box by creating a list item.
procedure TListFrame.PopulateAListBox( AListOfSomethings : TList<TSomething> );
var
listItem : TListItem;
Something : TSomething;
begin
for Something in AListOfSomethings do
begin
listItem := TheListView.Items.Add;
listItem.Text := Something.Text;
end
end;
Don’t try to compile the above example; I don’t think it builds. But you get the drift. Loop, add, loop, add.
Now consider the following example.
procedure TListFrame.PopulateAListBox( AListOfSomethings : TList<TSomething>; AListOfSomethingElse : TList<SomethingElse> );
var
listItem : TListItem;
Something : TSomething;
SomethingElse : TSomethingElse;
begin
for Something in AListOfSomethings do
begin
listItem := TheListView.Items.Add;
listItem.Text := Something.Text;
end
for SomethingElse in AListOfSomethingElse do
begin
listItem := TheListView.Items.Add;
listItem.Text := SomethingElse.Text;
end
end;
Here the programmer has fallen for the temptation to recycle code, namely the listItem variable. Though it’s perfectly correct syntactically to do this, there are issues with it. In my case, I refactored these two loops using the built-in Delphi refactoring tool Extract Method.
The Extract Method checks what parameters for the new extract method that needs to be created, and in this case, it decided that a listItem parameter is needed. As the listItem variable is used later in the code, it will make that parameter a var parameter, since the refactorer sees that this value is needed later in the code.
I was sloppy and assumed that too. Granted, the original legacy code I was working on was a bit more complicated, but still. Had I been a little bit more careful, I would have saved half an hour of changing the refactored methods later.
Here is what my first, bad refactoring looked like.
procedure TListFrame.PopulateAListBox( AListOfSomethings : TList<TSomething>; AListOfSomethingElse : TList<SomethingElse> ); var listItem : TListItem; begin PopulateSomethingListView( listItem ); PopulateSomethingElseListView( listItem ); end;
Which I later changed to…
procedure TListFrame.PopulateAListBox( AListOfSomethings : TList<TSomething>; AListOfSomethingElse : TList<SomethingElse> ); var listItem : TListItem; begin listItem := PopulateSomethingListView; PopulateSomethingElseListView( listItem ); end;
However, these new methods (PopulateSomethingListView and PopulateSomethingElseListView) have no dependencies on each other. They are perfectly separated, using different data sets and different target list views. But since an earlier programmer was a bit lazy or perhaps didn’t know better, the recycled variable listItem created a false dependency.
This false dependency cost me just half an hour to detect (and an hour to write this blog about it), but in a more complicated piece of code, reusing variables can not only cause obfuscation or confusion but actual bugs. What if some programmer, not careful enough, would start moving code around? Or perhaps new or changed functionality requests would require changing this code, and the programmer wasn’t observant enough to catch the recycling and fail to, perhaps, initialise the recycled variable correctly.
Summary
Recycling is for garbage, glass bottles, metals, wood and other stuff. Reusing is for old furniture, bicycles, cars, baby clothes etc. Don’t recycle or reuse in your code, and when you smell that specific recycling smell in the code your working on, refactor it. It’s very easy, just create a new variable for each section of use. It’ll make further refactorings so much easier, and it will help the code not to rot further.
