While reading Josh Pollock’s first article in his Advanced OOP for WordPress series, I noticed opportunities to improve his code’s quality and performance. So I reached out to him. As a fellow educator, he suggested that I do a code review and then publish my insights in a series of articles here on Torque as a companion to his series.
In this Code Quality Review series, I will present one or more code quality opportunities and provide details and insights to help you improve your code.
Let’s start with the posts_pre_query method in his FilterWPQuery class:
<?php
class FilterWPQuery
{
public static function posts_pre_query( $postsOrNull, $query ) {
//Only run during WordPress API requests
if ( defined( 'REST_REQUEST' ) && REST_REQUEST ) {
//Prevent recursions
remove_filter( 'posts_pre_query', [ FilterWPQuery::class, 'posts_pre_query' ], 10 );
//Don't run if posts are already sent
if ( is_null( $postsOrNull ) ) {
//Create 4 mock posts with different titles
$mockPosts = [];
for ( $i = 0; $i post_title = "Mock Post $i";
$mockPosts[ $i ]->filter = "raw";
}
//Return a mock array of mock posts
return $mockPosts;
}
//Always return something, even if its unchanged
return $postsOrNull;
}
}
}
When you look at the structure of this method, what do you notice? Focus on the indentation. Notice that there are multiple nested conditionals. Where is the business logic, i.e. the main intent of this method? It is wrapped inside of these conditionals.
This design is a bad coding practice and a flawed design.
Let’s explore why in the rest of the article. Then you and I will refactor his code together, step-by-step. Finally, I’ll give you an implementation strategy to identify and refactor these patterns in your code.
Exploring the Design Flaw
To illustrate my points, I will borrow the phrase “go/no go” from manufacturing days to describe the result of a conditional. A “go” state means the check passes and the process can continue. A “no go” state means the check fails and the process should stop.
Refer back to Josh’s code above and look specifically at the conditionals. When a conditional passes, that is a “go” as it is saying: “If this conditional passes my check, then run the code inside of my control block.”
How about when it fails, meaning that it finds a “no go?” Look at the code. What happens? The business logic does not run because the conditional is guarding it. That’s the behavior we want. But how about the “no go?” It continues running all the way to the end of the method. That is a design flaw. Why?
The conditional identifies a “no go” condition. This “no go” is a stopping point, meaning that when you find one, you don’t want any of the code below it to run.
This flowchart will help you to visualize the paths through the code:
Notice that a “no go” skips over the normal control flow (i.e. the business logic), bypassing it, and flowing the end of the method. In other words, the “no go” continues processing, whereas the normal control flow does not. That is the design flaw.
Wait, isn’t that the opposite of what we want? When we find a “no go,” we don’t want it to continue processing and flowing; rather, we want it to stop the method’s processing.
The effect of this flawed design is the “no go” exit point shifts from where the conditional finds it to the end of the method.
A Better Strategy – Return Early on a “No Go”
A better design is to stop the method’s execution and “return early” at the point where the code finds a “no go”. Refer to this flowchart:
Implementing this design clearly communicates your intent to stop, thereby, reducing the shifts in control flow and immediately improving your code’s quality.
This strategy is not new, in fact, Kent Beck first presented the idea of guard clauses in 1997 in his book, Smalltalk Best Practice Patterns.
The Approach is the Problem
Let’s start with understanding the problem. The problem is the approach.
When designing a function, you may think about the conditions that determine if the next lines of code should run. Right? In your mind, you are thinking “if this and that are true, then go do this work.” That thinking drives you to wrap the business logic:
If this and that are true {
Go do the work.
}
return
I want you to invert your approach. Change your thinking to: “if this or that are not true, then bail out.” Using this inverted model, the code follows this design:
If this or that are not true {
return // bail out
}
Go do the work.
Notice that the emphasis shifts to determining if the function should continue. If no, then the function stops executing and returns early at the point where the conditional determines it should stop and exit.
Why is This Approach Better?
This approach solves the problems listed above:
- The “no go” immediately stops the processing.
- The intent is clearly communicated.
- The code is more readable.
The code is clearly telling you “Hey, I’m stopping right here.” There are no misunderstandings about what will happen. Your code is then more readable because the intent is clear.
”Multiple returns can simplify the formatting of code, particularly conditionals. What’s more, the multiple return version of a method is often a more direct expression of the programmer’s intent.”
Kent Beck, Smalltalk Best Practice Patterns
Wait, there’s more. Using this strategy, you are unwrapping the method’s business logic, shifting it back inline to the left so that as you read the code, as it flows vertically down the page.
Why is this better? It takes less brain power to process what is happening in the code as you read and trace its control flow.
As the business logic grows, the problem exponentially grows. The more code that is wrapped and nested, the more complex it becomes and harder it is to read and comprehend what is happening and why.
Readability directly impacts code quality. The more readable it is, the less time it takes to comprehend, test, reuse, extend, and maintain it.
Step-by-Step Refactor: Invert the Design
Let’s invert the above code and walk through the refactoring process together.
Step 1: Invert the First Conditional
The first conditional is a “go/no go” checker. It decides if the function will continue running or not. Therefore, you can invert it, converting it from a “go” checker to a “no go” checker.
The current design communications: “If this is a REST request, then flow into this control block.” Invert that thinking: “If this is not a REST request, then nothing should happen so bail out.”
// Bail out if not a WordPress REST Request.
if ( !defined( 'REST_REQUEST' ) || !REST_REQUEST ) {
return $postsOrNull
}
Notice how this design puts emphasis on (1) identifying a “no go” condition and (2) exiting immediately when one occurs. This design clearly communicates its intent of why it exists in the code.
The new design is a guard clause, as it guards the method from a “no go” and returns early when one is found.
Step 2: Invert the Next Conditional
Let’s repeat that process for the next conditional expression.
The current design communications: “If the incoming value is null, then it flows into the control block.” Invert the approach by shifting emphasis to finding a “no go”: “Hey, if the incoming value is not null, then it’s already been sent out. There’s nothing to do. So I’m bailing out.”
// Bail out if posts were already sent.
if (!is_null($postsOrNull)) {
return $postsOrNull;
}
Notice how this design puts the emphasis squarely on identifying a “no go” state. Like the first step, this design is clearly communicating its intent and reason to exist in the code.
The new design is also a guard clause.
Step 3: Unwrapping the Primary Code
The next step is to move the method’s business logic to the bottom of the method, after the guard clauses.
Let’s Review
Here is the code from our refactoring steps above:
public static function posts_pre_query( $postsOrNull, $query ) {
// Bail out if not a WordPress REST Request.
if ( ! defined( 'REST_REQUEST' ) || ! REST_REQUEST ) {
return $postsOrNull;
}
// Unhook to prevent recursions.
remove_filter( 'posts_pre_query', [ FilterWPQuery::class, 'posts_pre_query' ], 10 );
// Bail out if posts were already sent.
if ( ! is_null( $postsOrNull ) ) {
return $postsOrNull;
}
// Mock getting posts by creating 4 mock posts with different titles.
$mockPosts = [];
for ( $i = 0; $i post_title = "Mock Post $i";
$mockPosts[ $i ]->filter = "raw";
}
return $mockPosts;
}
Go ahead and read it.
- It has guard clauses, each of which guard the method and stop its execution when either finds a “no go”.
- Reading the code, the “return early” intent is clearly expressed even without the inline comment.
- The method’s business logic is unwrapped and flows vertically down the page. It is easy to identify it.
- The code is more readable.
While there is more we can refactor to continue improving the quality of this code, let’s save those topics for the next article.
How to Implement in Your Code
How can you implement this strategy in your code? Look for the clues:
- Look for nested control blocks. Jeff Atwood calls this anti-pattern “arrow code”. When your code looks like an arrow, it’s a big clue to refactor.
- If a big section of code is wrapped inside of a control block, that’s a clue.
- An inline comment can be a clue. Read the comment and consider why it exists. It might be that the code is not telling you what is happening and, therefore, a comment is needed to clear it up.
When designing your code, ask yourself if the remaining code should not run if this check fails. If the answer is yes, then stop right here and immediately bail out.
Move all pre-conditions to the top of your function, meaning validate the incoming data to determine if the function should work or not. If no, use the guard clause strategy to protect your code.
For potential stopping points in the middle or near the end of your code, use the “return early” strategy.
Above all else, make sure your code clearly communicates its intent. Make it readable. When you do, you will immediately improve its code quality.
Does This Strategy Work For You?
What do you think? Do you see the value of returning early? Do you agree (or not) that it better communicates intent while protecting the function’s normal control flow?
I want to hear your opinion. Share it below.
Also, feel free to ask me any questions about the implementation, inversion model, or clues. I’m here to help.
Tonya Mork
Tonya is a technology leader, engineer, educator, mentor, and author. With over 3 decades of high-tech, enterprise experience, she’s on a mission to develop professional WordPress developers and engineers at Know the Code, unlocking each person’s potential and empowering each to excel, innovate, and prosper.
The post Code Review Part 1: Fixing Design Flaw with the “Return Early” Strategy appeared first on Torque.
