Code Review Part 2: Improving Readability and Performance of the FilterWPQuery Class

In the last article, I walked you through the problem of letting “no go” conditions flow through your code. I showed you how to refactor the code to solve the problem and make the code more readable. In this article, you and I will continue doing a code review and refactoring of the FilterWPQuery in Josh Pollock’s plugin.

In Part 3 of Advanced OOP for WordPress, Josh walks you through how he refactored the FilterWPQuery class to make it more testable. Let’s use his latest version of the class for our code review process.

<?php
namespace CalderaLearn\RestSearch;
/**
 * Class FilterWPQuery
 *
 * Changes WP_Query object during REST API requests
 *
 * @package CalderaLearn\RestSearch
 */
class FilterWPQuery implements FiltersPreWPQuery
{
	/**
	 * Priority for filter
	 *
	 * @var int
	 */
	protected static $filterPriority = 10;
	/**
	 * Demonstrates how to use a different way to set the posts that WP_Query returns
	 *
	 * @uses "posts_pre_query"
	 *
	 * @param $postsOrNull
	 * @return \WP_Post[]
	 */
	public static function callback($postsOrNull)
	{
		//Only run during WordPress API requests
		if (static::shouldFilter()) {
			//Prevent recursions
			//Don't run if posts are already sent
			if (is_null($postsOrNull)) {
				//Get mock data
				$postsOrNull = static::getPosts();
			}
		}
		//Always return something, even if its unchanged
		return $postsOrNull;
	}
	/** @inheritdoc */
	public static function shouldFilter() :bool
	{
		return did_action('rest_api_init');
	}
	/** @inheritdoc */
	public static function addFilter() : bool
	{
		return add_filter('posts_pre_query', [FilterWPQuery::class, 'callback'], 10);
	}
	/** @inheritdoc */
	public static function removeFilter() : bool
	{
		return remove_filter('posts_pre_query', [FilterWPQuery::class, 'callback'], 10);
	}
	/** @inheritdoc */
	public static function getFilterPriority() : int
	{
		return static::$filterPriority;
	}
	/** @inheritdoc */
	public static function getPosts() : array
	{
		//Create 4 mock posts with different titles
		$mockPosts = [];
		for ($i = 0; $i <= 3; $i++) {
			$post = new \WP_Post((new \stdClass()));
			$post->post_title = "Mock Post $i";
			$post->filter = 'raw';
			$mockPosts[$i] = $post;
		}
		//Return a mock array of mock posts
		return $mockPosts;
	}
}

To get us started, I’ll quickly refactor the opening conditionals by applying the same “return early” strategy that you and I did in the last article:

public static function callback($postsOrNull)
{
	// Bail out if not a WordPress REST Request.
	if ( ! static::shouldFilter()) {
		return $postsOrNull;
	}
	
	// Bail out if posts were already sent.
	if ( ! is_null($postsOrNull)) {
		return $postsOrNull;
	}

	// Get mock data
	$postsOrNull = static::getPosts();

	return $postsOrNull;
}

Now we can start our code review.

Make shouldFilter Decide If We Should Filter or Not

Notice that Josh created a new method called shouldFilter(). This method’s name tells us that it decides whether the callback should filter the pre-query or not.

When reviewing the callback’s code, that decision should be based on two conditions:

WordPress is currently processing a RESTful request.
The incoming value is null, meaning we need to go get the posts.

There’s a problem. In the current design, the shouldFilter() method is not deciding whether to filter or not. Rather, it’s only doing one of the checks.

How can we fix this problem? We can move the null checker into the method.

Let’s walk through it together step-by-step.

Step 1: Move the Null Checker

The first step is to relocate the null checker from the callback() method and put it into the shouldFilter() method. That’s easy enough to cut it from one method and paste it into another.

But hold on, the null checker is dependent upon the given input. That means we declare $postsOrNull as a method’s parameter.

public static function shouldFilter($postsOrNull) :bool
{
	// REST request checker.
	if ( ! did_action('rest_api_init')) {
		return false;
	}

	// Null checker.
	if ( ! is_null($postsOrNull)) {
		return false;
	}

	return true;
}

Step 2: Flip the Checker Order to Improve Performance

Let me ask you a question. What happens if the method receives anything other than null? Look at the code. What happens?

Yes, it returns false back and does its job. But look at the control flow. First, it has to go through the REST request checker.

Think about the order of the checkers. Should we do the REST request check before checking what we received?

The answer to that question depends upon the complexity of the code. In this case, it is more performant (faster) to flip the order and do the null checker first.

Why? Look at the code. The PHP function is_null is very quick, whereas the the WordPress function did_action() has more code that needs to be processed.

Flipping the order allows us to do the faster check first. Then if it fails, the code will more quickly bail out and return early.

public static function shouldFilter($postsOrNull) :bool
{
	// Null checker.
	if ( ! is_null($postsOrNull)) {
		return false;
	}

	// REST request checker.
	if ( ! did_action('rest_api_init')) {
		return false;
	}

	return true;
}

Step 3: Abstract the REST Checker to Improve Readability

Right now the REST checker requires an inline comment for us to quickly understand its intent (i.e. what it’s doing). Let’s remove that comment and I’ll ask you a question.

if (did_action('rest_api_init')) {}

Can you quickly understand what the intent of this check? No. I can’t. Do you agree? Then we need to refactor this code to make it tell us.

Code Tip: Make it Tell Us. Make it Human Readable.

I want to stop right here for a moment and share a code tip with you.

Code should be expressive and highly human readable. It should tell us what’s happening so that we can quickly get our work done.

When code needs a comment, that’s typically a clue. The way you know if it needs to be refactored is by reading the code without the comment and then asking yourself: “Can I quickly understand what’s going on?” If you answer no, then refactor.

”Clean code is simple and direct. Clean code reads like well-written prose. Clean code never obscures the designer’s intent but rather is full of crisp abstractions and straightforward lines of control.”

Grady Booch, Clean Code: A Handbook of Agile Software Craftsmanship

Back to Refactoring

Let’s refactor this checker by creating a private method:

public static function shouldFilter($postsOrNull) :bool
{
	if ( ! is_null($postsOrNull)) {
		return false;
	}

	if ( ! static::doingREST()) {
		return false;
	}

	return true;
}

/**
 * Checks if WordPress is doing a REST request.
 *
 * @return bool
 */
private static function doingREST() : bool
{
	return did_action('rest_api_init');
}

Notice that the method’s name describes the behavior of this checker. We no longer need the inline comments. When you come back to this code next week or next year, the code will convey its message to you more quickly.

Remember, readability is a major cornerstone of quality code. Make it tell. Make it human readable.

Step 4: Just Return the Conditional’s Decision

I often see this code pattern:

if ( ! static::doingREST()) {
  return false;
}

return true;

Why do we need to explicitly return true or false? Think about it.

The conditional is already returning its decision in a boolean format. That means the decision has been made. Why not simply return its decision?

return static::doingREST();

Why is this strategy better?

It’s less code to read and maintain.
It’s more performant (faster) because there is only one line of code for PHP to process.

Here is our refactored code to this point:

public static function callback($postsOrNull)
{
	if ( ! static::shouldFilter($postsOrNull)) {
		return $postsOrNull;
	}

	// Get mock data
	$postsOrNull = static::getPosts();

	return $postsOrNull;
}

public static function shouldFilter($postsOrNull) :bool
{
	if ( ! is_null($postsOrNull)) {
		return false;
	}

	return static::doingREST();
}

/**
 * Checks if WordPress is doing a REST request.
 *
 * @return bool
 */
private static function doingREST() : bool
{
	return did_action('rest_api_init');
}

Step 5: Update the Interface

The last step is to update the method in the interface to declare the incoming input:

/**
 * Checks if the request should be filtered or not.
 *
 * @param array|null $postsOrNull Array of WP_Posts or null.
 * @return bool
 */
public static function shouldFilter($postsOrNull) :bool;

Now the interface and implementation match.

Skip the Assignment and Just Return

Let’s look at these couple lines of code:

// Get mock data
$postsOrNull = static::getPosts();

return $postsOrNull;

What do you notice? Think about what these lines of code are doing.

The returned array of posts from the getPosts() method is being assigned to a variable first before being returned to the filter event. Why? It’s not used anywhere.

This code is an example of an unnecessary assignment. Here, we can just return whatever is returned from getPosts().

public static function callback($postsOrNull)
{
	if ( ! static::shouldFilter($postsOrNull)) {
		return $postsOrNull;
	}

	return static::getPosts();
}

This refactor is better because:

It’s less code to read and maintain.
It’s more performant (faster) because:
1. PHP does not have to create the variable in its symbols table.
2. It does not have to bind that variable to the array’s memory location.
3. It avoids a variable lookup before returning.
4. There is only one line of code for PHP to process.

You can learn more about how PHP manages its memory by reading the PHP Internals Book.

Callback is Too Generic. Make it Tell Us What It Does.

I noted early how important it is to make code tell us what’s going on. That starts with how we name our functions and methods. These are our workers. They do stuff. Therefore, they should start with a verb and then be descriptive and expressive about its behavior.

The name “callback” is generic. That word means that the method is bound to something that will invoke it. But it doesn’t tell us anything about what will happen.

What does this method do?

It’s a filter, which changes the given input for the ’posts_pre_query’ event.
It handles or manages getting the array of posts to be used by the query.

Therefore, in essence, it’s a filter that changes the pre-query. Let’s rename it to filterPreQuery. What do you think?

In making this name change, we’ll have to change it in the interface and each of the tests that Josh built. That’s easy enough with a global search and replace.

Use the Priority Level Property

Take a look at the addFilter() and removeFilter() methods. Notice that the priority level is hard-coded. Why? Stick with me as I explain the thought process.

The class has a property that holds the value of the priority level. There’s a method to get that property.

Imagine that you wanted to change it from 10 to say 99. In order to make that change, how many places in the class do you have to remember to change? 3.

What happens if you forget to replace one of them? A bug might occur.

Using the DRY principle, we strive to eliminate redundant code. One of the reasons is to eliminate the problem I just explained.

How do we fix this one? We can use the property when adding or removing the filter hook:

/** @inheritdoc */
public static function addFilter() : bool
{
	return add_filter('posts_pre_query', [FilterWPQuery::class, 'filterPreQuery'], static::$filterPriority);
}

/** @inheritdoc */
public static function removeFilter() : bool
{
	return remove_filter('posts_pre_query', [FilterWPQuery::class, 'filterPreQuery'], static::$filterPriority);
}

Let’s Review

I covered a lot in this article. Together, you and I walked through the code review process to improve the FilterWPQuery class. Though this class is small, there were quite a few code quality improvements.

Let’s look at what we did to improve readability and performance:

The improvement	Readability	Performance
Made the `shouldFilter` method decide whether the class should filter or not.	✓	✓
Flipped the order of the checkers.		✓
Created a new private method to tell us the intent of checking if WordPress is processing a RESTful request.	✓
Returned the decision of the conditional.	✓	✓
Skipped the variable assignment to simply return the array from `getPosts()`.	✓	✓
Gave the callback method a more expressive name to tell us what it does. We made that name start with a verb, as method’s do work.	✓

In addition, we made the code more maintainable:

Used the priority level’s property to replace the hard-coded integers in the add and remove methods.

All of these improvements are designed to increase the class’ code quality.

What’s Next?

How about the posts generator code in the getPosts() method? I think we’ve done enough in this article. Don’t you? Let’s continue the code review and improvement process in Part 3.

The final code and each refactoring step is documented in the Pull Request on GitHub. I invite you to explore it.

Let’s Discuss It

What do you think? Do each of these improvements make sense to you? No, really, I want to hear what you think.

From the step-by-step walkthrough, do you see how to implement each of these strategies in your own code?

I look forward to discussing this review and refactor process with you. Feel free to ask me any questions and share your opinions in the comments below.

There are 2 comments

May 2, 2018 #

Caspar

I particularly like your abstraction of the did_action() check to a doingREST() method. A case might be made that did_action() is already descriptive, but for someone new to this code, this saves having to look up “What’s the ‘rest_api_init` action?” and then figuring out why we should care. doingREST() immediately makes sense.
- May 2, 2018 #
  
  Tonya Mork
  
  Excellent. Thanks for sharing, Caspar. When I first read it, the intent was not clear to me, especially without the inline comment. That’s what caught my eye, as it was screaming “Refactor me.”
  
  When we strive to write code that clearly communicates intent, then our code is quickly understandable. That helps all of us to pop into the project and get to work. Win. Win.