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.
Improve your test writing “Flow.”
Sign up to get my test-oriented code snippets.
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 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 goal: Move related things closer together. Move unrelated things further apart.
Refactoring: Making a Mess to Clear Up 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 = placeholder1Again, it feels like our formerly compact code is bloating. But remember, we’re making a mess to clear up a mess. This is all part of teasing the code apart into moveable chunks.
Refactoring seems to make a bigger mess, at first. You end up with more chunks, but each chunk is easier to move.
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 11.2.1 grays out automated refactoring for Extract to Method. (In a follow-up 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 = placeholder1Run 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 function to the target context. Adjust it to fit 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.
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 function into a delegating function.
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 of 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.
All Articles in this series
