In my last post, you learned what the Single Responsibility Principle is, why it is important, and its benefits. You’re now able to apply SRP at the class level.
Today, I’ll show you how to apply SRP at the method level.
By looking at some practical examples, you’ll learn how to spot potential code smells so that you can refactor your methods into shorter methods. You’ll write shorter methods with each doing just one thing. This’ll make it much easier to write unit tests because the inputs and outputs become obvious. There are also fewer edge cases per method.
Class level vs method level
The original SRP states that:
Each software module should have one and only one reason to change.
Last time, we treated software module as class. This time, we’ll treat software module as method.
So, the SRP can be restated as:
There should never be more than one reason for a method to change.
What is reason to change?
Let’s say you have a method M that does several things: A, B, C, and D. Things A and B implement feature X, while things C and D implement feature Y.
|
1 2 3 4 5 6 7 8 |
func M { A B C D } |
When feature X’s requirements change, you’ll need to modify method M. While doing so, there’s a fair chance you’ll also inadvertently alter feature Y.
Your tests for method M have to cover for all test cases for feature X, feature Y, and a full permutation of both features X and Y. This makes it difficult to write the tests in the first place. They are also more fragile and break when a change is made.
How do you feel when you update feature X and unintentionally change the behavior of Y? You lose confidence in your tests. You keep juggle around in method M, hoping it’ll work again. Eventually, it spirals into a massive method M.
And then no developer, including you who wrote it in the first place, wants to touch method M again.
That’s no way to programmer happiness.
So how can we do better? Extract things C and D into a new method N. Invoke N from M.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 |
func M { A B N() } func N { C D } |
M still does two things A and B. But it’s ok, because A and B are both necessary to implement feature X. Similarly, things C and D in N implement feature Y.
Now, you could separate A, B, C, D into four methods. But that’s an overkill.
The key is to maintain the cohesion between things that change for the same reason, and decouple the things that change for different reasons.
As Uncle Bob pointed out in his post on the Single Responsibility Principle, on the topic of cohesion vs coupling:
Gather together the things that change for the same reasons. Separate those things that change for different reasons.
A massive viewDidLoad() method
I recently ran a workshop and showed this massive viewDidLoad() method to a group of iOS developers.
|
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 |
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 } } } |
How many things does this viewDidLoad() method do?
In short, when the view finishes loading, 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.
Let’s break it down into bullet points:
- When the view finishes loading
- 5 posts
- Sort posts by most recent first
- Fetch posts over the network
- Get the currently logged in user
- Get the user’s followers
- Show a login button if logged out
This viewDidLoad() method massively violates the SRP.
If you want to see how I break down this massive viewDidLoad() method into shorter methods, you can subscribe below to find out the details of future workshops.
Spotting keywords in feature spec for multiple responsibilities
When you look at a feature requirement, it is often useful to keep an eye out for some keywords that can give you clues into some potential code smells.
It should XXX and/or XXX.
- The
fetchPosts()method should check the login status and fetch posts and parse JSON data into post objects.
It should XXX when XXX.
- The
fetchPosts()method should parse JSON data into post objects when it fetches posts.
If XXX, it should do XXX.
- If the user is logged in, the
fetchPosts()method should fetch posts. If it successfully fetches posts, it should parse JSON data into posts objects.
The client can specify a feature requirement in any of these three versions. If you pay attention to the keywords before you begin to write the code, it’ll help you see multiple responsibilities from the beginning so you don’t end up writing massive methods.
Potential code smells to watch out for refactoring opportunities
For existing code, you can also detect potential code smells by noticing specific language constructs and how they are used.
Let’s consider the following accelerate() method for a Car.
|
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 |
class Car { func accelerate() { guard engine.state == .Start && battery.hasCharge else { return } switch accelerator.state { case .Pressed: while accelerator.duration > 0.0 { if engine.state == .Start && battery.hasCharge && pumpFuel() { let surfaceMaterial = getSurfaceMaterial() let tireMaterial = getTireMaterial() let friction = calculateFriction(surfaceMaterial, tireMaterial, steering) let acceleration = accelerator.level * (weight - friction) / (weight / gravity) let distance = currentVelocity * duration + (acceleration * duration * duration) / 2 let nextPosition = calculateNextPosition(currentPosition, distance, acceleration) moveToPosition(nextPosition) } accelerator.duration -= duration } case .Released: break } } } |
The following language constructs are very common, and aren’t necessarily bad by themselves. What you need to focus on is the context in which they are used.
&&, ||
This should be obvious. If you AND or OR multiple things together, you could be doing more than one unrelated things. If they aren’t related, break them down into separate methods.
|
1 2 3 4 5 6 7 8 9 |
func accelerate() { // ... if engine.state == .Start && battery.hasCharge && pumpFuel() { // ... } // ... } |
Here, checking the engine state and if the battery has charge are unrelated. They are also too implementation dependent. (We’ll look at pumping fuel next.) It can be improved to:
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 |
func accelerate() { // ... if engineStarted() && batteryHasCharge() && pumpFuel() { // ... } // ... } func engineStarted() -> Bool { return engine.state == .Start } func batteryHasCharge() -> Bool { return battery.hasCharge } |
Now, if the determination of engine state and calculation of battery charge has to change, you only need to change in one place, engineStarted() and batteryHasCharge().
|
1 2 3 4 5 6 7 8 9 10 |
func engineStarted() -> Bool { return engine.passCheck && engine.wellOiled && engine.state == .Start } func batteryHasCharge() -> Bool { return battery.connected && battery.health > 0.3 && battery.charge > 0.10 } |
Now, you’re glad you’ve refactored it to separate methods. There’s just one place to change and it’s well tested.
if
If a condition is true, then do something. This is probably the most used statement in any application.
The problem arises when the condition becomes yet another thing. If do-first-thing-that-returns-bool, then do-second-thing.
If your condition is more than just checking state, you may be doing too many things. If that’s the case, refactor the first thing into a separate method that returns a Bool. The new method can then be reused and invoked in other places.
Another problem occurs when you nest too many if-then-else. It usually means you are doing one too many things at each nesting. Refactor the nested if into its own method.
|
1 2 3 4 5 6 7 8 9 |
func accelerate() { // ... if engineStarted() && batteryHasCharge() && pumpFuel() { // ... } // ... } |
Pumping fuel from the fuel tank through the fuel pump into the engine compartment is not merely checking state. It is doing something essential. It’s already refactored into its own pumpFuel() method. But we can do even better by moving it out of the condition and into the body.
|
1 2 3 4 5 6 7 8 9 10 |
func accelerate() { // ... if engineStarted() && batteryHasCharge() { pumpFuel() // ... } // ... } |
In Swift, you use if let for optional binding, this is simply syntactical, not code smell.
switch
Swift makes switch very powerful. It can match all kinds of things such as numbers, strings, intervals, and even multiple values in tuples. It can also perform pattern matching and value binding for optionals. Finally, you can use where to check for conditions.
To quote the Spider-Man,
With great power comes great responsibility.
You have to be careful not to mix too many things together in a switch statement. Prefer cleanness over coolness. Complex pattern matching is hard to read and understand.
If you are doing too many things in a matched case, refactor it to its own method with a descriptive name for reusability and testability.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 |
func accelerate() { // ... switch accelerator.state { case .Pressed: while accelerator.duration > 0.0 { if engine.state == .Start && battery.hasCharge && pumpFuel() { let surfaceMaterial = getSurfaceMaterial() let tireMaterial = getTireMaterial() let friction = calculateFriction(surfaceMaterial, tireMaterial, steering) let acceleration = accelerator.level * (weight - friction) / (weight / gravity) let distance = currentVelocity * duration + (acceleration * duration * duration) / 2 let nextPosition = calculateNextPosition(currentPosition, distance, acceleration) moveToPosition(nextPosition) } accelerator.duration -= duration } case .Released: break } } |
The whole acceleration algorithm is embedded in the accelerate() method. These nitty gritty details of physics and geometry should be refactored to a lower level to be individually tested.
|
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 |
func accelerate() { // ... switch accelerator.state { case .Pressed: accelerateCar() case .Released: break } } func accelerateCar() { while accelerator.duration > 0.0 { if engine.state == .Start && battery.hasCharge && pumpFuel() { let surfaceMaterial = getSurfaceMaterial() let tireMaterial = getTireMaterial() let friction = calculateFriction(surfaceMaterial, tireMaterial, steering) let acceleration = accelerator.level * (weight - friction) / (weight / gravity) let distance = currentVelocity * duration + (acceleration * duration * duration) / 2 let nextPosition = calculateNextPosition(currentPosition, distance, acceleration) moveToPosition(nextPosition) } accelerator.duration -= duration } } |
Even better, refactor the switch statement to its own method and give the method a meaningful name for readability and cleanness.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 |
func accelerate() { // ... if canAccelerate() { accelerateCar() } } func canAccelerate() -> Bool { switch accelerator.state { case .Pressed: return true case .Released: return false } } |
When you use switch for pattern matching, try to keep the cases simple and use where judiciously. The pattern matching algorithm should be refactored into its own method with a meaningful name such as findMakerAndModelFromVIN().
for, while, repeat
Loops can smell similarly to if and switch. You have a condition to determine if the loop should repeat. The condition is usually pretty simple and not a problem. But if you do too many things inside the loop, you may want to take a closer look.
|
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 |
func accelerate() { // ... while accelerator.duration > 0.0 { if engine.state == .Start && battery.hasCharge && pumpFuel() { let surfaceMaterial = getSurfaceMaterial() let tireMaterial = getTireMaterial() let friction = calculateFriction(surfaceMaterial, tireMaterial, steering) let acceleration = accelerator.level * (weight - friction) / (weight / gravity) let distance = currentVelocity * duration + (acceleration * duration * duration) / 2 let nextPosition = calculateNextPosition(currentPosition, distance, acceleration) moveToPosition(nextPosition) } accelerator.duration -= duration } // ... } |
In the while loop of the accelerate() method, we are doing way too many things. For every time step,
- Get the surface and tire materials
- Calculate the friction of the road on the car
- Calculate the acceleration of the car
- Calculate the distance to travel
- Figure out the next position of the car
- Move the car to the new position
Do all these physics and geometry need to coexist in the accelerate() method? Not really. So let’s refactor it.
|
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 |
func accelerate() { // ... while accelerator.duration > 0.0 { if engine.state == .Start && battery.hasCharge && pumpFuel() { let distance = calculateDistanceToTravel() let nextPosition = calculateNextPosition(currentPosition, distance) moveToPosition(nextPosition) } } // ... } func calculateFriction() -> Double { let surfaceMaterial = getSurfaceMaterial() let tireMaterial = getTireMaterial() let friction = calculateFriction(surfaceMaterial, tireMaterial, steering) return friction } func calculateAcceleration(friction: Double) -> Double { let acceleration = accelerator.level * (weight - friction) / (weight / gravity) return acceleration } func calculateDistance(acceleration: Double) -> Double { let distance = currentVelocity * duration + (acceleration * duration * duration) / 2 return distance } func calculateDistanceToTravel() -> Double { let friction = calculateFriction() let acceleration = calculateAcceleration(friction) let distance = calculateDistance(acceleration) return distance } func calculateNextPosition(currentPosition: Point, _ distance: Double) -> Point { return Point(x: currentPosition.x + distance, y: currentPosition.y) } |
The original accelerate() method becomes much simpler and shows only the steps at the high level. The physics and geometry lower level details are extracted. You can write unit tests to make sure all the complexity is fully tested and verified to work. If you need to make a change later, you have a test suite to rely upon.
guard
guard is great to ensure some requirements are met before the method continues. Just make sure you don’t specify too many requirements. If that’s the case, refactor these requirements into separate methods.
Alternatively, you can extract the whole guard statement into its own method that return a Bool. They can then be individually tested and reused elsewhere.
|
1 2 3 4 5 6 |
func accelerate() { guard engine.state == .Start && battery.hasCharge else { return } // ... } |
Checking if the car can move is likely a very useful assertion elsewhere, so it makes sense to refactor it out.
|
1 2 3 4 5 6 7 8 9 10 11 |
func accelerate() { guard canMove() else { return } // ... } func canMove() -> Bool { return engine.state == .Start && battery.hasCharge } |
Again, this increases reusability and testability for the canMove() method.
a ? b : c
The ternary conditional operator:
|
1 2 |
a ? b : c |
is a shorthand for:
|
1 2 3 4 5 6 |
if a { b } else { c } |
Since it’s a shorthand, it is supposed to be short. If you make it too complex, or even nest them, it kinds of defeat the purpose.
Some people love these tertiary operations because they are concise. But use with caution. Make sure your condition and branches are simple. If you have to && or || multiple conditions, execute more than one statement in a branch, or calling multiple methods, they are almost certainly too complex. They can greatly reduce readability and are hard to understand.
Number of lines of code
While this isn’t syntactical, but it’s still a factor to consider. When your method starts to grow and consists of many lines of code, it’s time to consider if you are doing too many things.
Method size may indicate you are specifying unrelated things to happen after one another.
On one hand, it’s fine if a task requires multiple lines of code to initialize, configure, execute, and return results. That’s just the verboseness of the language or class design.
On the other hand, if you find yourself having more than one algorithms in a single method, you may want to refactor them into multiple methods and invoke them from your original method.
Great post, Raymond!
I feel you experience when I read it. Sometimes these ideas come into mind but lazy can kill it. So lazy is evil. Now I know that do small methods with SPR it a right way, and you need to force yourself to do that.
Thanks.
Nice work, but I have a quibble: I think your first example isn’t quite complete. You posit A & B implement feature X while C & D implement feature Y. Then you factor out C/D into a separate method. in order to keep with SRP.
But in doing so you change the level of abstraction of the original method (M). Now it directly implements feature X but only indirectly implements feature Y. To use the abstraction level terminology from Fowler, method M now is at the Implementation level of abstraction for feature X (does the actual work) but it’s at the Specification level for feature Y (calls something else to do the work). This means M would change if the Specification for Y changed, or when the Implementation of X changed.
A slightly more complete solution, I think, would have been to extract both features out to the Specification level. Then method M would be at the same abstraction level for both features. M’s reason to change then would be if the feature specifications changed, while N and O would each change only if the implementation of their feature changed.
None of this is affects your overall idea; it just keeps the solution consistent.
Hi Arlen,
I completely agree with your argument. I was just doing step 1 of the refactoring. If this is real code, I would absolutely move A and B to a new method O in the second step.
Thank you for taking the example a step further, and the excellent explanation with regard to the level of abstraction.