Practical .NET

No Comment, Part 3: Writing 'Really Obvious Code'

Peter looks at how rewriting some complex code -- purely to make it easier to read -- eliminates the need for writing comments. He even adds a comment to some code.

• By Peter Vogel
• 09/12/2013

In a previous column, I discussed why comments in code aren't a good idea. I followed that with another column that looked at some of the objections readers raised (and some things I got wrong). My claim in both columns is simple: Almost everything comments are supposed to do (with three exceptions) is better handled by writing code that's intended to be read -- what I call Really Obvious Code (ROC). If you're interested in those exceptions, take a look at the second column. The opposite of ROC is Write Only Code (WOC): code that's understood only by the original programmer (the natural output of "job security programming").

In response to those columns, several readers asked for some examples of ROC. That's what this column is about. I do realize the distinction between ROC and WOC is not a bright line: It's a continuum with an area in the middle about which reasonable people can disagree. Where such disagreements exist, you should err on the side of making the code more obvious -- as the following example demonstrates.

Rewriting WOC
For my example, I'm using an application that I'm working on for one of my clients. One of the issues is enabling, disabling, hiding and revealing buttons on the page. Managing those buttons (and other UI components) is primarily controlled by two items: The status of the items being reviewed (as set through radio buttons on the form) and the user's role (clerk, analyst, manager). However, other options also control what buttons are available -- the status of the orders being displayed, for instance.

This could lead to some really WOC-y code (the following examples are all "air code," by the way, so don't complain if it doesn't compile). That code, with typical comments, is shown in Listing 1. Most developers would probably find these comments accurate and useful even though they mostly just echo the code.

'Enable and disable buttons based on the 
'currently selected status and user's role

'First, check which status is selected
If rbOne.Checked = True then
  Select Case User.Role
    'If the user is a manager they can 
    'delete orders
    Case "Manager" Then
      'Check to see if any orders are in unknown status
      If List.Where(function(x) x.Status = "Unknown").
                    ToList.Count = 0 Then
        'If there aren't unknown orders, 
          'enable the del button
          btDel.Enabled = True
      Else
        'If there are orders in the unknown status, 
          'disable button
          btDel.Enabled = False
      End If
    Case Else
      'Don't let other roles click the button
        btDel.Enabled = False
  End Select
Else
  If rbTwo.Checked = True then
    ...

The first thing to do is put this into a method with a name that describes what the code does, eliminating the need for a comment that describes what the code does:

Public Sub EnableDisableFormButtons
  If rbOne.Checked = True then
    ...

The next step, of course, is to rename the RadioButtons (to reflect what status each one represents) and the button (to reflect what it allows the user to do):

If New.Checked = True Then
  ...
  DeleteOrdersButton.Enabled = True

Even more useful is to create a new method that checks the RadioButtons and returns the correct status. Adding an enumeration for each status isn't essential, but I'd probably do it anyway:

Public Enum OrderStatus
  New
  Approved
  Unknown = -1
End Enum

With that enumeration in place, that new method for returning the current selected status looks like this:

Public Function SelectedStatus() As OrderStatus
  If rbNew.Checked Then 
    Return OrderStatus.New
  End If
  ...

The original code now looks like Listing 2.

Public Sub EnableDisableFormButtons()

  Select Case SelectedStatus()
    Case OrderStatus.New
      Select Case User.Role
        Case "Manager"
	  If List.Where(function(x) x.st = "Unknown") Then
	    DeleteOrdersButton.Enabled = True
	  Else
	    DeleteOrdersButton.Enabled = False
         End If
        Case Else
	   DeleteOrdersButton.Enabled = False
      End Select
    Case OrderStatus.Approved

At this point, a good case could be made that no comments are required except, perhaps, to explain the LINQ query. Erring on the side of writing obvious code, I'd move that LINQ query into a separate method called UnknownOrdersInCollection and rewrite the code to the version in Listing 3.

Public Sub EnableDisableFormButtons

  Select Case OrderStatus
    Case OrderStatus.New
      Select Case user.Role
        Case "Manager" 
	  If Not UnknownOrdersInCollection Then
	         DeleteOrdersButton.Enabled = True
	  Else
	         DeleteOrdersButton.Enabled = False
         End If
        Case Else
	  DeleteOrdersButton.Enabled = False
      End Select
   Case OrderStatus.Approved
  ...

At this point, no comments are needed for a competent programmer (meaning, someone you'd let maintain this code) to understand what the code does.

'If it doesn't have the word invoice in the description, 
'don't add it to the list
If InStr(dr("description"), "Invoice") <> 0 Then 
  CompTypes.Add(New New_Item_String(dr("description"), dr("code")))
End If

Enhancing the ROC-ability
But I'd go further. First, I can simplify the code by setting button's Enabled property before checking any of the conditions. That's something worth doing, independent of the commenting issue. In fact, for that change, I'd consider adding a comment about why I'm setting the button's Enabled property to False. Notice that the comment in the following code explains the reason why I'm setting the Enabled property on the button, but not how the code disables the button:

Public Sub EnableDisableFormButtons

  'Normally, users can't delete orders
  DeleteOrdersButton.Enabled = False
  Select Case OrderStatus
    Case OrderStatus.New
      If user.Role = "Manager" AndAlso
        Not UnknownOrdersInCollection Then
  	  DeleteOrdersButton.Enabled = True
      End If
    Case OrderStatus.Approved

Then I'd take the code for each user role and put it into a class, creating a separate class for each role. Each of those classes now only has to handle the conditions for a single role. I could imagine a comment at the top of each class that describes the class (something like "`Manage UI settings for users in the Manager role"), but if I give the class a good enough name, I wouldn't bother, as shown in Listing 4.

Public Class UIManagementForManagers
  Inherits UIManagentForUserRole

Public Property DeleteOrdersButton As Boolean
Public Property ApproveOrdersButton As Boolean

Public Sub New()
  'Normally users can't delete orders
  Me.DeleteOrdersButton = False
  Select Case OrderStatus
    Case OrderStatus.New
      If Not UnknownOrdersExist Then
        DeleteOrdersButton = True
      End If
    Case OrderStatus.Approved
      ...

Integrating this into the method that manages the UI actually makes the method longer, as shown in Listing 5.

Public Sub EnableDisableFormButtons
Private UISettings as UIManagementByUserRole

Select Case User.Role
  Case "Manager"
    UISettings = New UIManagementForManagers
  Case "Analyst"
    UISettings = New UIManagementForAnalysts
  Case Else
    UISettings = New UIManagementForUserRole
End Select

Me.DeleteOrderButton.Enabled = UISettings.DeleteOrdersButton
Me.ApproveOrderButton.Enabled = UISettings.ApproveOrdersButton
...

This raises the question: If the resulting code is more verbose than the original commented version, how is this better? First, I'd make the case that should I need to add a new role or change the rules for any role, this code would be easier to maintain and extend (I've actually implemented a version of the State pattern).

But, for this column, the real payoff is that I have no comments to maintain -- that job has gone away. If I change what the programmer sees, I change the way the program works; if I need to change the way the program works, I change what the programmer sees. The program's documentation (the ROC) is always both a complete and accurate description of what the program does. And that's a good thing.