I'm a vocal proponent of self-documenting code, having meaningful identifiers, code that is not littered with useless comments.
Actually, when I need to add a comment, I usually see that as a failure in writing readable code
(one exception being workarounds for defects in 3rd party / legacy code).
It's easy to find texts on this topic, but not so easy to find actual examples.
However, recently, when writing some code I almost automatically added some comments to explain that code,
only to realize a bit later that this is one example of when code should've been more readable in the first place...
I wrote this method for setting item heights in a grid;
the idea here was that (an integer) number of pixels should be divided equally into heights of consecutive rows:
Private Sub SetColumnItemHeights(
ByVal AItems As HierarchyItemsCollection,
ByVal AHeight As Integer, ByVal ADepth As Integer)
For Each LItem As HierarchyItem In AItems
If LItem.Items.Count = 0 Then
ColumnsHierarchy.SetRowHeight(LItem, AHeight)
Exit Sub
Else
Dim LItemHeight As Integer = CInt(Math.Round(AHeight / ADepth))
ColumnsHierarchy.SetRowHeight(LItem, LItemHeight)
SetColumnItemHeights(LItem.Items, AHeight - LItemHeight, ADepth - 1)
End If
Next
End Sub
When reviewing this code before a commit
(after only maybe an hour of testing / verification and other tasks),
I noticed that the code isn't that clear even to me;
so, without much thinking, I added two comments to clarify some areas:
Private Sub SetColumnItemHeights(
ByVal AItems As HierarchyItemsCollection,
ByVal AHeight As Integer, ByVal ADepth As Integer)
For Each LItem As HierarchyItem In AItems
If LItem.Items.Count = 0 Then
ColumnsHierarchy.SetRowHeight(LItem, AHeight)
Exit Sub
Else
' Current item's height is the remaining height / remaining depth:
Dim LItemHeight As Integer = CInt(Math.Round(AHeight / ADepth))
ColumnsHierarchy.SetRowHeight(LItem, LItemHeight)
' Now the remaining height is the original height - current item's height:
SetColumnItemHeights(LItem.Items, AHeight - LItemHeight, ADepth - 1)
End If
Next
End Sub
And I almost committed that...
But then I thought that the comments shouldn't actually be needed there;
the code is so small and simple that it shouldn't need as many as two comments!
So, basically, the code wasn't clear enough...
But why?
Well, the main reason being that the names of parameters didn't actually convey the real meaning of what they're for...
AHeight here would most probably be understood as the height for each item.
So, to make things clear, two small refactorings later I got code that is (IMO) much more understandable:
Private Sub SetColumnItemHeights(
ByVal AItems As HierarchyItemsCollection,
ByVal ARemainingHeight As Integer, ByVal ARemainingDepth As Integer)
For Each LItem As HierarchyItem In AItems
If LItem.Items.Count = 0 Then
ColumnsHierarchy.SetRowHeight(LItem, ARemainingHeight)
Exit Sub
Else
Dim LItemHeight As Integer = CInt(Math.Round(ARemainingHeight / ARemainingDepth))
ColumnsHierarchy.SetRowHeight(LItem, LItemHeight)
SetColumnItemHeights(LItem.Items, ARemainingHeight - LItemHeight, ARemainingDepth - 1)
End If
Next
End Sub
Especially, the changes to the lines that previously needed comments made the code totally clear now
(where from an item is getting its own height and what is passed to the recursive call (remaining height - item's height)).
Also, the names of the routine's parameters are much more in line now with what they actually are for...
This looked clear enough now, but actually, we can make this code even a bit clearer by adding some Assert statements:
Private Sub SetColumnItemHeights(
ByVal AItems As HierarchyItemsCollection,
ByVal ARemainingHeight As Integer, ByVal ARemainingDepth As Integer)
For Each LItem As HierarchyItem In AItems
If LItem.Items.Count = 0 Then
Debug.Assert(RemainingDepth = 1)
ColumnsHierarchy.SetRowHeight(LItem, ARemainingHeight)
Exit Sub
Else
Debug.Assert(RemainingDepth > 1)
Dim LItemHeight As Integer = CInt(Math.Round(ARemainingHeight / ARemainingDepth))
ColumnsHierarchy.SetRowHeight(LItem, LItemHeight)
SetColumnItemHeights(LItem.Items, ARemainingHeight - LItemHeight, ARemainingDepth - 1)
End If
Next
End Sub
Hmm... actually it would be pretty nice to have (syntax-)coloring of Assert statements in the IDE...
Top
|
Texts
