This article is part of an email series I wrote to my subscribers to teach them how to refactor existing legacy code. If you want to join us in the next workshop, make sure you subscribe.
- What is wrong with this viewDidLoad() method?
- Don’t waste time writing tests for untestable code
- Breaking up a method into shorter methods with single responsibilities
- How to write clean code in a component architecture using Clean Swift
In my last email, you saw how little value tests provide if the code is not architected in the right way for it to be testable. In particular, the massive viewDidLoad() method we examined together was doing too many things. We decided to separate its many responsibilities into smaller tasks such that each can be easily tested.
Here’s my first take:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 |
class ViewController: UITableViewController { let userManager = UserManager() let postManager = PostManager() var recentPosts = [Post]() @IBOutlet weak var loginButton: UIButton! override func viewDidLoad() { super.viewDidLoad() displayFollowerPosts() } // MARK: - Steps to follow func displayFollowerPosts() { if let currentUser = userManager.loggedInUser() { let followers = userManager.followersForUser(currentUser) fetchPostsByAllFollowers(followers, completionHandler: { (posts: [Post]?, error: ErrorType?) -> () in if let error = error { self.showAlert(error) } else if let posts = posts { self.updateFollowerPosts(posts) } }) hideLoginButton() } else { showLoginButton() } } func updateFollowerPosts(posts: [Post]) { recentPosts = Array(posts.sort { $0 > $1 }[0...4]) refreshFollowerPosts() hideLoginButton() } // MARK: - Fetch posts func fetchPostsByAllFollowers(followers: [User], completionHandler: (posts: [Post]?, error: ErrorType?) -> ()) { var allFollowerPosts = [User: [Post]]() for follower in followers { fetchPostsByFollower(follower) { (posts: [Post]?, error: ErrorType?) -> () in if let error = error { completionHandler(posts: nil, error: error) } else if let posts = posts { allFollowerPosts[follower] = posts if allFollowerPosts.count == followers.count { completionHandler(posts: Array(allFollowerPosts.values.flatten()), error: nil) } } } } } func fetchPostsByFollower(follower: User, completionHandler: (posts: [Post]?, error: ErrorType?) -> ()) { postManager.fetchPostsForUser(follower) { (posts: [Post]?, error: ErrorType?) -> () in if let error = error { completionHandler(posts: nil, error: error) } else if let posts = posts { completionHandler(posts: posts, error: nil) } } } // MARK: - UI func refreshFollowerPosts() { tableView.reloadData() } func showLoginButton() { loginButton.hidden = false } func hideLoginButton() { loginButton.hidden = true } func showAlert(error: ErrorType) { // ... } } |
You can find all the supporting classes and structs for a fully compilable version here.
Each of the following functions have one clear responsibility. Their method names convey what they do.
fetchPostsByAllFollowers(_:completionHandler:)fetchPostsByFollower(_:completionHandler:)refreshFollowerPosts()showLoginButton()hideLoginButton()showAlert(_:)
Each function does only one thing and it’s very clear what that thing is. The method bodies are also very short.
The two fetch methods are longer because of the asynchronous nature. The handling of the success and failure cases in the closures also contribute to the length. But as we’ve seen earlier, number of lines of code is just a vanity metric.
The if-let optional binding for error and posts are pretty standard. It simply invokes the completion handler. The caller decides what to do with the result.
Because the method bodies are short, they are very easy to test. I have already written many posts about testing on my blog, so I’m not going to elaborate on it any further.
You may wonder how did I transform the original massive viewDidLoad() method into this better version. I looked at the original method and tried to figure out what it does, divvy up the responsibilities, and refactor it. Nothing special.
However, imagine if you had started with shorter, single responsibility methods, it would have been much easier to write than the original method AND the refactoring.
Although the displayFollowerPosts() and updateFollowerPosts(_:) methods are shorter, they still do more than one thing. They are certainly easier to read because of the descriptive method names. You no longer concern yourself with the logic and the syntax at the same time.
But it still feels like something isn’t quite right.
These two methods are what I called the coordinator methods.
You can think of them as managers in a corporation. They tell their employees to do the actual work. But they don’t do the work themselves. Rather, they coordinate the tasks performed by each employee.
This coordination holds or breaks the application logic. The behavior of the smaller, single responsibility methods are easy to test. Not so much with the coordinator methods.
In a single-class refactoring of this viewDidLoad() method, it’s really difficult, if not impossible, to get rid of them.
Although this new version is an improvement, we can still do better by componentizing the app to remove the risks presented by these coordinator methods. I’ll show you how to do that using the Clean Swift architecture in the next email.
Stay tuned.
BONUS POINT: Did you find out what the logic error is with the original viewDidLoad() method? Here’s the answer.
The follower == followers.last! check isn’t sufficient. The postManager.fetchPostsForUser() is asynchronous, so there’s no guarantee the server responses will be in the same order as the requests. You could very well receive the response for the last follower before you receive all the previous ones.
A better check is allFollowerPosts.count == followers.count. So you don’t invoke the completion handler before you receive all the responses for all the followers.
In a massive method, something like that is easy to slip through.