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

Shares

When you refactor, do you have unit tests covering you? …If not, why not? …If so, how do you know?

People trying to weigh down pitchfork as it tilts dangerously

Source: http://www.ntd.tv/2017/02/14/2016-fails-will-stay-ever/

To me, it seems that the state of refactoring has gotten worse across the industry. Both managers and programmers and managers say the word “refactoring” more than ever. But they almost always mean, “I’m going to change a bunch of stuff. Then at the end, we need to make sure I didn’t break anything.”

But that’s not refactoring. That’s rewriting.

What is refactoring?

Refactoring is moving in small steps, with each step verified by unit tests.

To see a demo of what I mean, watch my screencast Refactoring Demo: Is It More than Just Changing Stuff? In the comments, Vui Nguyen shared,

Wow, this is like “iterative refactoring”, which is a term I just made up :) I like the idea of refactoring so that the code still works every step of the way, instead of leaving things broken until the end.

If her term “iterative refactoring” helps you, use it. To me, it’s simply “refactoring”. But since the term has been so misunderstood, call it “iterative refactoring” if it helps communicate the idea:

  • You can break a large refactoring into small moves.
  • A single refactoring move has many steps.
  • After any step that may introduce a failure, we run the unit tests.

The key thing is that we don’t refactor without tests, and the tests should never fail. We start in green, and end in green. And we check often.

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

I keep saying this: The Refactoring book changed the way I code.

The code smell almost everyone missed

Let’s return to the Code Smells exercise. I asked, “How many code smells do you see below?”

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()
    }
}

Graham Lee, author of Test-Driven iOS Development, left a two-word reply:

No tests.

Obama mic drop

Code without tests is, by definition, legacy code. And maybe now you can see why. Without tests, we can’t do any refactoring.*

* With one exception: when you need to refactor something to make it testable. That’s when I pull out my copy of Working Effectively with Legacy Code.

That’s why, before I begin any refactoring, I first check to see if the code in question has tests …But how does one do that?

Code coverage can lie

I started by enabling code coverage, and running all unit tests. And look at that, I see 100% coverage for that method! So we’re good, right?

But the code coverage was a lie.

I knew that because when I first came across this viewDidLoad method, it had no tests. And I wanted to start by extracting the helpers setUpNormalView and setUpAlternateView. (Reminder: I anonymized this code. Those aren’t the real method names.) So I wrote tests to cover both. One test sets isAlternateStyle on the view model, and the other doesn’t.

Now do you see the problem?

The lines that set the placeholder values aren’t tested. They just happen to be incidentally hit on the way through each branch. Because we test both sides of the conditional, we have “100% coverage”. But it doesn’t tell us if the code is safe to refactor!

Five years ago when I released my XcodeCoverage tool, I wrote this:

If the coverage shows a hole, I know that area is lacking unit tests.

In other words, where there is no coverage, we know that no tests cover that line. But the opposite is not true! When a line shows in code coverage, it doesn’t mean tests cover it in any meaningful way.

How can we check for meaningful coverage?

There is a way to see whether the tests actually check the code we want to refactor. Here’s what I wrote in Who Tests the Tests? Increase Confidence in Your Tests:

There’s a simple way to test the tests: Deliberately break something in production code.

So that’s what I did for the code in question. Look at the placeholder values:

override func viewDidLoad() {
    super.viewDidLoad()

    if viewModel.isAlternateStyle {
        // Alternate style needs different placeholder text.
        textField1.placeholder = "BOGUS"
        textField2.placeholder = "BOGUS"
        setUpAlternateView()
    } else {
        textField1.placeholder = "BOGUS"
        textField2.placeholder = "BOGUS"
        setUpNormalView()
    }
}

As you can see, I’m simply setting all the placeholder values to “BOGUS”. Then I ran the unit tests.

Bill & Ted say "BOGUS"

There were no test failures.

This proved to me that despite the 100% coverage, the tests didn’t actually cover those lines!

(This is “mutation testing”. On other platforms, mutation testing is automatic. The testing program introduces a change in the production code, and reruns the tests. For example, adding a ! to reverse a conditional, or swapping && with ||. Or as I did, assigning bogus values. I’d love to see automatic mutation testing for Swift, wouldn’t you?)

Don’t refactor without tests!

Once I knew that the placeholder lines weren’t covered, the path was clear: I added tests for them.

These tests failed until I restored the original code. This gave me the confidence that now, I had enough test coverage to begin refactoring.

Let’s summarize:

  • Refactoring is moving in small steps, with each step verified by unit tests.
  • Unit tests are essential for refactoring.
  • Before any refactoring, ask: how good are the tests for that section of code?
  • Lack of code coverage tell us that no tests cover a section of code.
  • But having code coverage doesn’t tell us anything about the tests.
  • Check test quality by deliberately breaking a section of code to see if any tests fail.
  • Add any missing tests before starting to refactor.
  • While refactoring, start in green, stay in green, and end in green.

Next time, we’ll start to refactor those code smells.

What questions do you have? What challenges do you face as you begin applying these refactoring principles? Please share in the comments below.

About the Author Jon Reid

When I was a kid, programming was fun. But working in Silicon Valley, I saw poor code lead to fear, with real human costs. Searching for ways to make life better, I learned about Design Patterns, Refactoring, and Test Driven Development (TDD). Programming became fun again! I've now been doing TDD in Apple environments for 17 years. I'm committed to software crafting as a discipline, with the hope of raising us all to greater effectiveness and joy.

follow me on:
  • Josh says:

    This is awesome, Jon! I’d always wondered how to verify that tests are strong enough, and intentionally breaking the production code makes perfect sense. Thanks!

  • Balaji says:

    Hi Jon,
    Thank you for the post. I have a question on below rule.
    “Before any refactoring, ask: how good are the tests for that section of code?”

    How to refactor code (legacy code) which doesn’t have any unit tests?

    • Jon Reid says:

      Balaji, that’s a really good question. Because quite often, in order to make something testable, it needs to be refactored! I felt stuck on this question until I read Working Effectively with Legacy Code.

      Let me summarize a few things:

      Use automated refactorings
      “Lean on the compiler” through type safety
      Make snapshot tests for views. This is a type of “characterization test”.
      When all else fails, do a manual test that confirms your change.

  • >