Code Review / Alternative Designs
private struct ItemViewModel {
var item: Item?
var title: String? { item?.title }
var image: UIImage? { item?.image }
var currentPrice: String? { (item?.currentPrice).map { "€\($0)" } }
var strikethroughPrice: NSAttributedString? {
guard let item = item else { return nil }
return isOnSale() ? AttributedText.strikethroughText("€\(item.listPrice)") : nil
}
var isStrikethroughPriceHidden: Bool { !isOnSale() }
private func isOnSale() -> Bool {
guard let item = item else { return false }
return item.savingsRate > 0
}
}
Moving the View Model into Its Own File
- failing tests, or
- broken code that doesn’t compile.
We want to minimize or eliminate the time we’re in red.
Now, we’re going to mess around with ItemViewModel here. The first thing I want to do is pull it into its own file. Unfortunately, there’s no automated support for this in AppCode, at least for Swift. So, we’re going to do it manually.
Improve your test writing “Flow.”
Sign up to get my test-oriented code snippets.
The view model can’t stay private if it’s going to move to a different file. So let’s take off the private and compile. This is to make sure that nothing else complains about another ItemViewModel elsewhere.
Then I create a new file, and cut and paste the struct. This gives us an error because it doesn’t know what UIImage is, so let’s import UIKit. And this should pass tests. Now we can play in here.
Adding an Explicit Initializer
We want to extract a protocol from this view model, but that’s hard to do in one step. Yes, that is the final goal. But we can get there while minimizing breaks by working in smaller steps.
Looking at where we call this view model, it’s created once and mutated in another place. Because this is a value object, when we mutate it, it gets swapped out for a different instance. Let’s make this explicit by creating new instances, either with or without an item.
So let’s add an explicit initializer. It doesn’t build, so we want to get out of code not building as fast as we can. When the app starts, the item is initially nil. Let’s confirm this by running tests.
Where it implicitly creates a new item view model, let’s make this explicit. Instead of changing the item, we are going to set it to a new ItemViewModel passing in the new item. We run tests to see if this passes, and it does. So this is a step forward.
I do encourage you to get the code and try it yourself using Xcode or AppCode or whatever you like.
Making a Factory Function
So now we instantiate ItemViewModels in two places. To create one thing or another thing we need a factory method. So instead of instantiating an ItemViewModel, we’re going to make a PresentItemViewModel or a NullItemViewModel.
We can start with a static function. It should take the same arguments as the initializer, and return an ItemViewModel. And at first, it’s simply going to return the same thing. This should compile.
Now one at a time, let’s replace the direct call to the initializer with the call to the static function. I run tests after each change.
The factory function gives us a way to do other things, to create one thing or another. So let’s declare those things. The easiest way to move forward would be to make a subclass, but you can’t subclass a struct. So as a temporary measure, let’s change this to a class. This happens often in refactoring: to reach your desired destination, you have to make a temporary change, then revisit it later to set it back.
Now that it’s a class, we can define new subclasses: PresentItemViewModel and NullItemViewModel. These are subclasses that do nothing other than existing. And inside the factory function, we’re going to make one or the other.
class PresentItemViewModel: ItemViewModel {}
class NullItemViewModel: ItemViewModel {}Let’s do this in very small steps. I want to see if the item is nil and if so, unwrap it. So let’s add an if-else statement, where each clause is empty. This immediately breaks the build because now we need an explicit return statement.
if let item = item {
} else {
}
return ItemViewModel(item: item)Now I can duplicate the return statement and put one into each side of the if-else. This is a provable refactoring called “Move line into all if/else branches” which doesn’t change any logic. I’m still making the same thing—one in the if and another in the else.
if let item = item {
return ItemViewModel(item: item)
} else {
return ItemViewModel(item: item)
}With this conditional, we can replace one instance with the PresentItemViewModel subclass. Run tests. In the nil case, we can return a NullItemViewModel subclass, and run tests again.
if let item = item {
return PresentItemViewModel(item: item)
} else {
return NullItemViewModel(item: item)
}Let’s pull this static function out of the class. When the view model becomes a protocol, we can’t have a static function inside of the protocol. Instead of just yanking it out and fixing up the places, we can do this in small, verified steps. Remember, we want to minimize the time where we can’t compile the code. So I start by copying the code, pasting it, and removing the static declaration. This should compile.
Then we want the static function to forward to the new standalone function. But we can’t use the same name, because that would be recursive. To break this cycle, let’s temporarily change the name of the standalone function to make2. Run tests.
Now we can go to each caller and switch them from the old function to the new function. I confirm each change by running tests. Once the old function is no longer called, we can get rid of it and run tests. Then we can rename the new function from make2 to makeItemViewModel. Always use your IDE’s “Rename” support. Once again, we confirm this change by running tests.
Duplicate Types Conforming to a Protocol
There’s no longer any need for the item property in ItemViewModel to be mutable. Let’s change it from a var to a let. While we’re at it, let’s see if we can make it private. We want to restrict information as much as possible.
Now let’s copy the entire thing and paste it into one of the subclasses. This gives us a few build errors. We can fix most by applying the suggestion that the methods should override their superclass. After that, one last bit is that the initializer needs to call super.init. With the compile problems out of the way, run tests to confirm that everything still works.
Then I copy everything and paste the same thing inside of the NullItemViewModel, with no changes. So now we have two copies of the original code, but inside of the subclasses.
It may be possible now to switch from subclassing to using a protocol. To do that, let’s define a protocol. I do this by copying a computed property, pasting, removing the body, and fixing up the declaration to be read-only—it’s get not set.
I can now declare that the base class ItemViewModel conforms to this new protocol. When I do, I get immediate feedback in AppCode that the title computed property is implementing a protocol definition, and that it’s overridden elsewhere. So that seems to be the way forward.
Let’s grab the other computed properties and do the same thing. Now we should have those little “implementor” indicators by everything except for the private function. Run tests to see that they still pass.
Now, with this protocol in place, we may be able to change the subclassing into a straight implementation of a protocol. But when I try, a bunch of stuff fails—all these overrides no longer make sense. I try removing them all in one shot, by doing a replace-all inside the selected area. And we don’t need super anymore because there is no superclass. And will this work? It doesn’t work. Why? Because the factory function is trying to return the old type, which no longer matches.
(All book links below are affiliate links. If you buy anything, I earn a commission, at no extra cost to you.)
At this point, the safest thing to do is revert, going back to green. I run tests again to confirm that we’re back in safe territory. Then we can make the preparatory changes we need to switch away from subclassing. This is another refactoring meta-pattern. It’s a small application of The Mikado Method.
We want this factory method to return an instance of the protocol, not the base class. Let’s see if this builds. It doesn’t, because the view model itself also needs to be the same type. So let’s fix that forward and see if this builds and passes tests. Now I can go back and try the subclass removal again. I change PresentItemViewModel from subclassing ItemViewModel to conforming to ItemViewModelProtocol. Then I get rid of the call to super and replace override-space, and run tests to see if this works. It does.
Now I can do the same with NullItemViewModel, copying and pasting the entire body to match. Now we have PresentItemViewModel and NullItemViewModel both implementing the same protocol. And they both have the same bodies.
A quick check in the IDE shows that nothing remains which calls the original ItemViewModel. I delete it and run tests. We distributed its body successfully into two new types. These types can now become structs again, instead of classes. And we can rename the protocol to ItemViewModel. (Remember, don’t rename things manually.)
Could you have done this in fewer steps by just ripping the code apart? Yes, but you would have been broken this whole time. Notice that even as I was doing these small steps, when I would run into a situation where things didn’t compile, I have a choice:
- Fix forward, if I can do it in one step.
- Or undo, and try making the required change first.
I’m trying to stay in green as much as possible.
Removing Unused Code to Simplify Each Type
struct NullItemViewModel: ItemViewModel {
var title: String? { nil }
var image: UIImage? { nil }
var currentPrice: String? { nil }
var strikethroughPrice: NSAttributedString? { nil }
var isStrikethroughPriceHidden: Bool { true }
}Does this mean I can change the item property to be non-optional? As soon as we do that, we have some things break. Let’s see if we can fix these. This is going to be the longest time I’m going to be in a broken state. Mostly, I apply the fix-it suggestions to remove optional chaining. For the map call, I can grab the subject and paste it over the closure argument. I eliminate the guard clauses. I’m careful to run tests after each of these changes.
About Design Patterns
All Articles in this series
