.st0{fill:#FFFFFF;}
iOS Refactoring & Design

How to Improve Code Comments with Little Refactorings 

 March 20, 2018

by Jon Reid

9 COMMENTS

Tweet0
Share0
Share0

Does your code have comments? Sometimes they’re helpful. But most of the time…

Disclosure: The book links below are affiliate links. If you buy anything, I earn a commission, at no extra cost to you.

As Jeff Atwood explains, code tells you how, but comments tell you why. A well-placed code comment is a level above the code itself, explaining why something is written the way it is. But! We can express most comments as code, using well-named identifiers. The first edition of  the Refactoring book called this Introduce Explaining Variable. Martin Fowler has since renamed that refactoring to Extract Variable.


Table of Contents
Code Comments: Example from Exercise
Moving Things Around
Rules of Thumb for Code Comments and Refactoring
Improving a Short, Smelly Method

Code Comments: Example from Exercise

Let’s look at a specific example from my Code Smells exercise:

override func viewDidLoad() {
    super.viewDidLoad()
    if viewModel.isAlternateStyle {
        // Alternate style needs different placeholder text.
        textField1.placeholder = strings.field1Hint
        textField2.placeholder = strings.field2Hint
        setUpAlternateView()
    } else {
        textField1.placeholder = strings.field1Placeholder
        textField2.placeholder = strings.field2Placeholder
        setUpNormalView()
    }
}

First, I apologize for the awkward names. Many people said the generic names like textField1 and textField2 were a code smell. Normally, yes! But they’re so generic because I want to keep my job: I anonymized a section of real code I had encountered at work.

Mark Bernstein responded to the code smells exercise with many good observations, including:

The first smell is the comment; if part of the code needs explanation, it can be explained by making it a method.

In other words: extract and name. This might be as a variable or as a method, depending on how many lines we’re talking about. But give it a descriptive name.

Moving Things Around

How many lines are we talking about for this comment? It says,

// Alternate style needs different placeholder text.

Clearly, this applies to the next two lines, which set the placeholder property for textField1 and textField2. But wait, they’re also set in the else clause. And the comment is talking about the difference.

What if we separated that if-else block into two such blocks? Like this:

override func viewDidLoad() {
    super.viewDidLoad()
    if viewModel.isAlternateStyle { // 👈
        // Alternate style needs different placeholder text.
        textField1.placeholder = strings.field1Hint
        textField2.placeholder = strings.field2Hint
    } else {
        textField1.placeholder = strings.field1Placeholder
        textField2.placeholder = strings.field2Placeholder
    }
    if viewModel.isAlternateStyle { // 👈
        setUpAlternateView()
    } else {
        setUpNormalView()
    }
}

This unifies the things that are the same (setting the placeholders for the text fields). It separates the things that are different (setting up the rest of the view to match the desired style).

“But now we’re repeating the if statement!” Sometimes, to clean things up, you first need to make it messier. If you’ve ever organized a messy closet, you know what this is like.

Now the comment could move up so it comes before the first if statement:

    // Alternate style needs different placeholder text.
    if viewModel.isAlternateStyle {
        textField1.placeholder = strings.field1Hint
        textField2.placeholder = strings.field2Hint
    } else {
        textField1.placeholder = strings.field1Placeholder
        textField2.placeholder = strings.field2Placeholder
    }

The code comment now lives in the right place: it attaches itself to the entire if-else block. Try to make the scope of your comments obvious.

But at this point, it’s pretty clear that the comment isn’t adding any value. It’s just describing the code.

When you see a code comment that only describes the code, delete the comment.

Rules of Thumb for Code Comments and Refactoring

Here are some rules of thumb for code comments, with methodical refactoring steps you can take.

Click to Tweet

Let’s summarize what we’ve seen in this example. First, about code comments:

  1. Make the scope of your comment clear. You can do this for a single line by attaching the comment to the end of the line. For multiple lines, use blank space to separate the commented part from the rest of the code.
  2. Use your comment to explain why the code is necessary. Don’t explain the how; that’s what the code does.
  3. If a comment only repeats the code, delete it. ’Nuff said.
  4. Can you transform the comment into an identifier? Use Extract Variable or Extract Function. Then you’ll have self-documenting code!

Finally, some observations about refactoring:

  1. If your IDE provides automated refactoring, use it. (Xcode is getting better on this front, but still refuses to extract certain things. AppCode is the winner for automated refactoring.)
  2. Bring similar things closer together. Move dissimilar things further apart.
  3. Don’t be afraid to make a slight mess while you clean, even if it means increasing duplication. Some things are hard to see until you move them around.

But! Before we do any refactoring, let me slam the brakes. We’re not ready to make any changes to the original code! There’s one big, big principle about refactoring we need to look: do you have tests?

Search your codebase for comments. Do any have any ambiguous scope? Can you change any into code? Can you simply delete any? Let us know how you did in the comments below…

All Articles in this series

Improving a Short, Smelly Method

How Many Code Smells Can You Find in This Short Method?

How to Improve Code Comments with Little Refactorings

Do You Refactor without Tests? It’s Time for Safety

When You Refactor, Are You Using Small Steps?

Refactoring: How Do You Clear Up a Mess, Safely?

 Previous

How Many Code Smells Can You Find in This Short Method?

 Next

Do You Refactor without Tests? It’s Time for Safety

0 shares
Tweet 0
Share 0
Share 0

Jon Reid

Author Image
About the author
Tweet0
Share0
Share0

Programming was fun when I was a kid. But working in Silicon Valley, I saw poor code lead to fear, with real human costs. Looking for ways to make my life better, I learned about Extreme Programming, including unit testing, test-driven development (TDD), and refactoring. Programming became fun again! I've now been doing TDD in Apple environments for 20 years. I'm committed to software crafting as a discipline, hoping we can all reach greater effectiveness and joy. Now a coach with Industrial Logic!

 Find out more

  • Great article. Here is another “why” comment I use: the URL of an OpenRadar or StackOverflow answer explaining the necessity or appropriateness of some hack.

    Reply

  • I’d add another rule: remove any commented out code that you come across. It provides no value and can be incredibly confusing for someone coming to the codebase later on.

    Reply

  • Thanks for the article. I’d refactor viewDidLoad as (keeping same level of abstraction by removing those if to viewModel and setUpView function) :

    override func viewDidLoad() {
    super.viewDidLoad()
    textField1.placeholder = viewModel.field1Hint
    textField2.placeholder = viewModel.field2Hint
    
    setUpView(viewModel.isAlternateStyle)
}
func setUpView(isAlternateStyle: Bool) {
    if viewModel.isAlternateStyle {
        setUpAlternateView()
    } else {
        setUpNormalView()
    }
}
    Reply

  • I like your post. I would have tried a different way to clean this up. I would have moved the placeholder assignment into functions like so:

    override func viewDidLoad() {
       super.viewDidLoad()
       if viewModel.isAlternateStyle {
           useHintForPlaceHolder()
           setUpAlternateView()
       } else {
           usePlaceholder()
           setUpNormalView()
       }
   }
   func useHintForPlaceholder() {
       textField1.placeholder = strings.field1Hint
       textField2.placeholder = strings.field2Hint
   }
   func usePlaceholder() {
       textField1.placeholder = strings.field1Placeholder
       textField2.placeholder = strings.field2Placeholder
   }
   func setUpAlternateView() {
       // many things
   }
   func setUpNormalView() {
       // many things
   }

    Then, I would have considered whether the placeholder assignment was part of the setup. If it was, I would have moved it into the setup like so:

    override func viewDidLoad() {
       super.viewDidLoad()
       if viewModel.isAlternateStyle {
           setUpAlternateView()
       } else {
           setUpNormalView()
       }
   }
   func setUpAlternateView() {
       useHintForPlaceHolder()
       // many things
   }
   func setUpNormalView() {
       usePlaceholder()
       // many things
   }
   func useHintForPlaceholder() {
       textField1.placeholder = strings.field1Hint
       textField2.placeholder = strings.field2Hint
   }
   func usePlaceholder() {
       textField1.placeholder = strings.field1Placeholder
       textField2.placeholder = strings.field2Placeholder
   }
    Reply

    • Thanks for thinking it through, Russ! I cover my own approach in related articles, with detailed steps on how I did it. But it wasn’t clear that there was a series, and why this post focused only on the comment smell. So I just added a big block at the bottom of each post to make it more obvious. Check them out when you have time.

      Reply
    • {"email":"Email address invalid","url":"Website address invalid","required":"Required field missing"}
    >