[Workshop – 2] Don’t waste time writing tests for untestable code

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.

  1. What is wrong with this viewDidLoad() method?
  2. Don’t waste time writing tests for untestable code
  3. Breaking up a method into shorter methods with single responsibilities
  4. How to write clean code in a component architecture using Clean Swift

Last week, I sent out a massive viewDidLoad() method and asked you how you would fix it. Many of you said you would write some tests and refactor it.

But have you tried to actually write the tests for it? Why not? Is it easy to do?

The method was untestable. I would not even bother writing any test for it. Here’s why:

  • It’ll be super difficult to write the tests.
  • I’m not even sure if I can cover all the original intentions.
  • I’m not even sure if the original author properly expressed all his intentions.
  • After refactoring, those tests would be completely useless and are thrown out anyway.

The tests would not be very valuable if it’s written for some bad code.

That’s why I think architecture comes before unit testing. It is only meaningful to write tests for a well architected code base.

One of the reasons why the method is untestable is because it violates the Single Responsibility Principle (SRP). It unfortunately follows the “Ten Responsibilities Principle”:

  1. Get the currently logged in user.
  2. Find the user’s followers.
  3. Fetch posts for each of the followers.
  4. Aggregate all followers’ posts.
  5. Check for the last follower.
  6. Sort the posts.
  7. Limit to the first 5 posts.
  8. Refresh the table view.
  9. Show/hide the login button.
  10. Show an alert when there is an error.

The SRP states that a class should have only one reason to change.

We can apply the same SRP to methods. The viewDidLoad() method has 10 responsibilities, and they become coupled. If you need to make a change to one responsibility, it unnecessarily affects other responsibilities.

For example, assume the asynchronous postManager.fetchPostsForUser() method returns a Response struct containing success/failure and the payload instead of separate [Post] and ErrorType. You would have to change a lot of code.

You actually have to re-read the whole method and manually test it many times to make sure it still works for all the edge cases.

So the first step I would take is to separate these responsibilities into multiple much shorter methods. I can then test these single responsibility methods much more easily. Better yet, these shorter methods and tests don’t have to be thrown away. They’ll be part of the final refactored code.

In my next email, I am going to show you how to do just that.

In the meantime, for bonus point, there is actually a logical error in this viewDidLoad() method. Can you spot it?