Make a Mess to Clean a Mess: Refactoring, Continued

Shares

Refactoring is moving in small steps, with each step verified by unit tests. As I demonstrated last time, these steps are much smaller than most people are used to. Let’s continue the same example to learn some new things about refactoring.

Reading is not as good as doing. I learned from the folks at Big Nerd Ranch that our brains make important connections when we type code. Download my code and walk through the refactoring steps yourself.

Where we last left things

Here’s the code smells exercise as we left it last time.

override func viewDidLoad() {
    super.viewDidLoad()

    if viewModel.isAlternateStyle {
        textField1.placeholder = viewModel.strings.field1Hint
        textField2.placeholder = viewModel.strings.field2Hint
    } else {
        textField1.placeholder = viewModel.strings.field1Placeholder
        textField2.placeholder = viewModel.strings.field2Placeholder
    }

    if viewModel.isAlternateStyle {
        setUpAlternateView()
    } else {
        setUpNormalView()
    }
}

Let’s continue to focus on the the first section, which sets placeholders for two text fields. (Reminder: the names textField1 and textField2 aren’t descriptive because I anonymized a section of real code.) Our goal is to move this logic into the view model.

Last time, we made progress by moving the strings from the view controller into the view model. We did this so that the view model could have everything it needs to manage the conditional logic.

Take a few moments to think about it: How would you move this in small steps? (Hint: Do one field at a time.)

Separating the entanglement

This is the logic we want to move. Notice how each clause of the if-else statement is changing both text fields.

if viewModel.isAlternateStyle {
    textField1.placeholder = strings.field1Hint
    textField2.placeholder = strings.field2Hint
} else {
    textField1.placeholder = strings.field1Placeholder
    textField2.placeholder = strings.field2Placeholder
}

Remember my hint above? “Do one field at a time.” This is harder to do when they’re entangled with each other.

So, separate them. Duplicate the entire block of code above, so that we have two. Remove lines with textField2 from the first block. Remove lines with textField1 from the second block.

if viewModel.isAlternateStyle {
    textField1.placeholder = strings.field1Hint
} else {
    textField1.placeholder = strings.field1Placeholder
}

if viewModel.isAlternateStyle {
    textField2.placeholder = strings.field2Hint
} else {
    textField2.placeholder = strings.field2Placeholder
}

Run tests to confirm. (Remember, don’t start refactoring a section of code until you give it full test coverage.)

This illustrates a common refactoring goal: Move related things closer together. Move unrelated things further apart.

Refactoring: Making a mess to clean a mess

At this point, you may be frowning a little. We started with 7 lines. Now that’s grown to 11 lines. Wasn’t refactoring supposed to make things more expressive? Did we make things messier?

This is like organizing a cluttered closet. Everything is hidden when the closet door is closed. But inside, it’s a mess. In fact, that mess is slowing you down. So one day, you decide to invest time into organizing it. But it’s hard to tell what’s what, so you pull everything out. Now your closet is empty (and ready to receive things in an orderly manner). But everything that used to be in there now lies scattered across the floor, and on top of your bed. What a mess!

This is a typical phase during a series of refactoring steps. Don’t panic! All we’re doing is exposing the mess that was hidden before. Like the stuff in the closet, the code is now easier to deal with, one thing at a time.

Separating calculation from assignment

Now we can work on the first text field, by itself:

if viewModel.isAlternateStyle {
    textField1.placeholder = strings.field1Hint
} else {
    textField1.placeholder = strings.field1Placeholder
}

Ultimately, this will become some kind of function in the view model. I’m planning to do an Extract Function followed by a Move Function. But we don’t want to extract the text field; we want to leave it behind. Let’s separate “figure out which string we want” from “assign it to the text field” by introducing a temporary variable:

let placeholder1: String
if viewModel.isAlternateStyle {
    placeholder1 = viewModel.strings.field1Hint
} else {
    placeholder1 = viewModel.strings.field1Placeholder
}
textField1.placeholder = placeholder1

Again, it feels like our formerly compact code is bloating. But remember, we’re making a mess to clean a mess. This is all part of teasing the code apart into moveable chunks.

Applying “Extract Function”

Now we can proceed with the Extract Function refactoring. I select the first 6 lines, which set the value of the temporary variable. Now in AppCode, we can use the Extract Method command, and it works. Sadly, Xcode 10.1 grays out automated refactoring for this selection. (For my next post, I’m planning to show AppCode and Xcode side-by-side.)

Whether automated or manual, the result of the extraction is a new method.

private func placeholder1() -> String {
    let placeholder1: String
    if viewModel.isAlternateStyle {
        placeholder1 = viewModel.strings.field1Hint
    } else {
        placeholder1 = viewModel.strings.field1Placeholder
    }
    return placeholder1
}

At the call site, we now have:

let placeholder1 = placeholder1()
textField1.placeholder = placeholder1

Run tests to confirm.

Now applying the Inline Variable refactoring, we get:

textField1.placeholder = placeholder1()

Run tests to confirm.

Applying “Move Function”

Now that we extracted a method inside the view controller, we can use Move Function to move it to the view model. The “Mechanics” section of Move Function includes this step:

Copy the code from the source method to the target. Adjust the method to make it work in its new home.

Disclosure: The book links below are affiliate links. If you buy anything, I earn a commission, at no extra cost to you. I’m still using my copy of the 1st edition. Martin Fowler has probably changed the wording in the newly-released 2nd edition. I’m quite eager to get it!

So I copy the placeholder1() function, and paste it into the view model. Since this new method is inside the ViewModel struct, delete its references to the viewModel property.

struct ViewModel {
    let isAlternateStyle: Bool
    let strings: ScreenStrings

    private func placeholder1() -> String {
        let placeholder1: String
        if isAlternateStyle {
            placeholder1 = strings.field1Hint
        } else {
            placeholder1 = strings.field1Placeholder
        }
        return placeholder1
    }
}

Build the target to confirm. (There’s no need to run tests, because nothing is calling this new method yet. The old method is still there, and everything is still calling it.)

Here’s the next small step to take:

Turn the source method into a delegating method.

To access the new method from the old method, we need to remove private from its declaration. Then we can call it from the old method:

private func placeholder1() -> String {
    return viewModel.placeholder1()
}

Run tests to confirm.

Finally, we can take this old method and perform the Inline Function refactoring. This deletes the old method and changes the call site to:

textField1.placeholder = viewModel.placeholder1()

Run tests to confirm. …And that concludes our Move Function refactoring!

Conclusion

There are a few things left to do that I won’t show in this post:

  • Simplify the new method.
  • Since it has no arguments, change it from a function to a calculated property.
  • Repeat everything for the second text field.

But hopefully I’ve given you enough to see a few patterns:

  • Each move is small.
  • Each move is automatically verified.
  • Lean on automated refactoring when it’s available.
  • Larger refactorings are composed from smaller ones.
  • Teasing apart functionality seems to make a bigger mess, at first. You end up with more chunks, but each chunk is easier to move.

Do you have any questions about this refactoring? Can you break down your next change into smaller steps, verifying each change? Please share in the comments below.

About the Author Jon Reid

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 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, hoping we can all reach greater effectiveness and joy.

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

>