Refactoring remains a much-misunderstood discipline. Even when folks do have unit tests to back their changes, how long does the code stay broken? Let’s return to the idea that refactoring happens in very small steps.
No, even smaller than that.
Improve your test writing “Flow.”
Sign up to get my test-oriented code snippets.
Motivating Example
Remember this code from the code smells exercise? I made the names generic because I anonymized a section of real code.
override func viewDidLoad() {
super.viewDidLoad()
if viewModel.isAlternateStyle {
textField1.placeholder = strings.field1Hint
textField2.placeholder = strings.field2Hint
} else {
textField1.placeholder = strings.field1Placeholder
textField2.placeholder = strings.field2Placeholder
}
if viewModel.isAlternateStyle {
setUpAlternateView()
} else {
setUpNormalView()
}
}Let’s focus our attention for now on the first section, which sets placeholders for two text fields. When I asked for code smells, Josh Brown commented:
The View Controller should just ask the View Model for the placeholder text, rather than having a conditional statement. That conditional belongs in the View Model.
This will be our goal. How do we get there?
It’s tempting to start ripping things apart, tossing all the pieces in the air. But the problem with making changes here & there is that we’ve broken the code—and we don’t know how long it will remain broken.
That’s not the Refactoring way. Disclosure: The book links below are affiliate links. If you buy anything, I earn a commission, at no extra cost to you.
Take Smaller Steps
If we were doing this in one of my workshops, I’d let you wrestle with a few ideas. And I’d keep replying, “Good. Now how could you take an even smaller step?”
So take a few moments right now to think about it. How would you shift things to the View Model?
Now, can you take that step, and break it down even further?
(And remember, the first step of any refactoring is to guarantee the desired behavior with unit tests. We’ve already done that.)
Move Field Refactoring
To have the View Model provide the placeholder text, the View Model will need those strings. So let’s do a Move Field refactoring. The motivation for this move as described in the Refactoring book is
A field is, or will be, used by another class more than the class on which it is defined.
Currently, the strings are in the View Controller. For this example, I’m going to use a view controller that’s created programmatically. This simplifies Dependency Injection. So it has an initializer with the following signature:
init(strings: ScreenStrings, viewModel: ViewModel)And I’ve reduced the View Model to its barest form:
struct ViewModel {
let isAlternateStyle: Bool
}What we want to do is pass the strings to the View Model, instead of passing them to the view controller.
Reducing the Amount of Change
Adding or removing initializer parameters causes us to update the call sites. In the production code, there’s probably one place that sets things up and creates the view controller. But it’s easy to have duplication in the test code. I have several tests which start with these lines:
let viewModel = ViewModel(isAlternateStyle: false)
let sut = ViewController(strings: strings, viewModel: viewModel)A test case manages multiple objects. There’s the thing being tested, any inputs, and any outputs. A standard term for “the thing being tested” is System Under Test.
If you use the name sut to identify the System Under Test, it will be easy to find across your test code.
The smallest change I can envision is to add the strings to the view model. We won’t even remove them from the view controller yet. But adding a field will break every test that starts with these lines. That’s painful.
Let’s reduce the amount of change needed. The pain we’re feeling comes from duplicate code. Instead of setting up the System Under Test repeatedly, let’s extract that creation to a Test Helper.
Even to make this Test Helper, I want to do that in small steps. And I want to lean on automated refactoring support as much as possible. Ready? Here we go!
A Sequence of Very Small Steps
Step 1
We can’t just Extract Method because some tests need to specify isAlternateStyle with false, and some with true. The method we extract needs to take that as a parameter. So the first step is to perform Extract Variable on literal value.
I place my cursor in the false argument of
let viewModel = ViewModel(isAlternateStyle: false)Using AppCode, I do an Extract Variable refactoring with ⌥⌘V. (Strangely, while Xcode has “Extract Variable” in its Refactoring menu, none of the choices are enabled when false is selected.) This gives us:
let isAlternateStyle = false
let viewModel = ViewModel(isAlternateStyle: isAlternateStyle)
let sut = ViewController(strings: strings, viewModel: viewModel)Run tests to confirm.
Step 2
Now I select the bottom 2 lines, and do an Extract Method refactoring with ⌥⌘M. I specify the name of the new method, and this makes:
private func makeViewController(isAlternateStyle: Bool) -> ViewController {
let viewModel = ViewModel(isAlternateStyle: isAlternateStyle)
let sut = ViewController(strings: strings, viewModel: viewModel)
return sut
}Run tests to confirm.
Step 3
We can combine the last two lines in the new method by performing an Inline Variable refactoring. AppCode provides Inline for Objective-C, but hasn't brought it to Swift yet. So I do this refactoring by hand:
private func makeViewController(isAlternateStyle: Bool) -> ViewController {
let viewModel = ViewModel(isAlternateStyle: isAlternateStyle)
return ViewController(strings: strings, viewModel: viewModel)
}Run tests to confirm.
Step 4
Now the original test from which we extracted the method has these lines:
let isAlternateStyle = false
let sut = makeViewController(isAlternateStyle: isAlternateStyle)This temporary variable has served its purpose, helping us perform the extraction. Let's Inline Variable to eliminate the variable:
let sut = makeViewController(isAlternateStyle: false)Run tests to confirm.
To extract a parameterized method from code which has the value hard-coded:
- Perform “Extract Variable” to pull the value out.
- Perform “Extract Method” to pull the code out.
- Perform “Inline Variable” to use the value directly in the new call.
Step 5
Now for every test that sets up the view controller with isAlternateStyle: false, I can use this new line. I do “Replace all” in file.
Run tests to confirm.
Step 6
Now I edit the replacement strings, changing false to true so it will transform the isAlternateStyle: true cases. I do “Replace all” in file.
Run tests to confirm.
Step 7
Now every test calls the makeViewController() helper. This will make it easier to add strings to the ViewModel. I add a new property, like this:
struct ViewModel {
let isAlternateStyle: Bool
let strings: ScreenStrings
}This is the first change that breaks the build. But we’ve reduced the amount of change needed. I fix up the production code, and I change the test helper:
private func makeViewController(isAlternateStyle: Bool) -> ViewController {
let viewModel = ViewModel(isAlternateStyle: isAlternateStyle,
strings: strings)
return ViewController(strings: strings, viewModel: viewModel)
}Run tests to confirm.
Step 8
Now both ViewController and ViewModel have a strings property. Enabling Column Selection Mode in AppCode (or using Select Column in Xcode), I change the first clause of the if statement in one shot:
if viewModel.isAlternateStyle {
textField1.placeholder = viewModel.strings.field1Hint
textField2.placeholder = viewModel.strings.field2Hint
} else {
textField1.placeholder = strings.field1Placeholder
textField2.placeholder = strings.field2Placeholder
}Run tests to confirm.
Step 9
I do the same thing in the else clause, selecting a column so I can type “viewModel.” to edit two lines at once:
if viewModel.isAlternateStyle {
textField1.placeholder = viewModel.strings.field1Hint
textField2.placeholder = viewModel.strings.field2Hint
} else {
textField1.placeholder = viewModel.strings.field1Placeholder
textField2.placeholder = viewModel.strings.field2Placeholder
}Run tests to confirm.
Step 10
In real life, there would probably be more places in the view controller that continue to use the strings property. But in this example, those were all the references. So I can now remove the property, and the line that initializes it in init(). (The initializer still has a strings parameter, but it’s now unused.)
Run tests to confirm.
Step 11
Finally, I can remove the strings parameter from the view controller initializer. This is the second change that breaks the build. I fix up the production code, and I change the test helper:
private func makeViewController(isAlternateStyle: Bool) -> ViewController {
let viewModel = ViewModel(isAlternateStyle: isAlternateStyle,
strings: strings)
return ViewController(viewModel: viewModel)
}Run tests to confirm.
This completes the Move Field refactoring! The strings have a new home. This will let us move responsibility for the placeholder text from the view controller to the view model—as more refactoring, of course.
Conclusions
Don’t just make a bunch of changes and hope for the best. You can do real refactoring instead. Refactoring is moving in small steps, with each step verified by unit tests.
Refactoring is moving in small steps, with each step verified by unit tests.
A few observations:
- A single refactoring like Move Field is composed of smaller steps, often using other refactorings.
- Each small step is really quick, and includes a test run. So it's important to have quick turnaround time for building and running tests.
- Add the new thing. Shift each use from the old thing to the new thing. Then remove the old thing. …This is a common pattern in refactoring.
- The only times we couldn't commit our changes to master were when things were broken. And we minimized those times.
- Since I wrote this, AppCode added automated refactoring to Change Signature. This reduces the build breaks even more, by updating all call sites.
If you'd like to see a demonstration of refactoring, watch Refactoring Demo: Is It More than Just Changing Stuff? It's 12 minutes long, and may help give you a visual “feel” for what refactoring looks like.
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
Good post. It’s very important to remember to always move in tiny steps when refactoring. Sure it’s tempting to re-structure the whole file at once, but that’s just as good as rewriting it (and that’s usually not good).
The number of small steps may feel daunting when written out like this when in reality it happens very quickly.
It’s something I still need to practice more to get the most out of refactoring and its increased reliability.
Thanks, Aleksi. I’m frequently tempted to take larger steps, “to save time.” But at some point, I’m usually bit by doing so, which reminds me to take it smaller.