[Workshop - 1] What is wrong with this viewDidLoad() method?

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

Take a look at this viewDidLoad() method.

What is wrong with it? Why is it so bad? Can you tell what it does?

class ViewController: UITableViewController
{
  override func viewDidLoad()
  {
    super.viewDidLoad()
    let currentUser = userManager.loggedInUser()
    if let currentUser = currentUser {
      var followerPosts = [Post]()
      let followers = userManager.followersForUser(currentUser)
      for follower in followers {
        postManager.fetchPostsForUser(follower) { (posts: [Post]?, error: ErrorType?) -> () in
          if let error = error {
            self.showAlert(error)
          } else if let posts = posts {
            followerPosts += posts
          }
          if follower == followers.last! {
            self.recentPosts = Array(followerPosts.sort { $0 > $1 }[0...4])
            self.tableView.reloadData()
            self.loginButton.hidden = true
          }
        }
      }
    } else {
      recentPosts = []
      tableView.reloadData()
      loginButton.hidden = false
    }
  }
}

class ViewController: UITableViewController

{

override func viewDidLoad()

{

super.viewDidLoad()

let currentUser = userManager.loggedInUser()

if let currentUser = currentUser {

var followerPosts = [Post]()

let followers = userManager.followersForUser(currentUser)

for follower in followers {

postManager.fetchPostsForUser(follower) { (posts: [Post]?, error: ErrorType?) -> () in

if let error = error {

self.showAlert(error)

} else if let posts = posts {

followerPosts += posts

}

if follower == followers.last! {

self.recentPosts = Array(followerPosts.sort { $0 > $1 }[0...4])

self.tableView.reloadData()

self.loginButton.hidden = true

}

} else {

recentPosts = []

tableView.reloadData()

loginButton.hidden = false

}

After the view is loaded, it tries to show the 5 most recent posts of a user’s followers. If the user isn’t logged in, it shows a login button.

How long did it take you to read and understand what it does?
Do you think you’ll remember a week from today?
If I ask you to make a change, will you know what to do without having to read it again?
Can you be sure you won’t break something when you make the change?
What do you feel about passing this along to someone else who needs to make a change?
What if you are on the receiving end of this method?
How will you go about fixing it?

We’ll be dissecting this method down to the nitty-gritty to find ways to improve it.

Hit reply and let me know your answers now.

Share this post: