Whenever it comes to writing code – be it starting a new project or working with an existing system – there are a lot of developers that aim to write clean, maintainable code.

It’s a challenge for sure, but it’s not impossible. Even if you’re responsible for simply introducing a new function into an existing system, there’s an opportunity to leave the place a little better than you found it.

Granted, trying to refactor a larger system too much can have serious unintended consequences, so I don’t necessarily advocate doing that, but if you have the opportunity to break a one larger function into several smaller, more focused functions, then I think the opportunity should be taken.

I recently had a chance to do a bit of WordPress refactoring in the context of a commissioned plugin I am working on, and wanted to share the process that I followed for doing so.

What The Function Does

In short, the function is working with a Project custom post type and is responsible for doing the following:

  • If the post is being published and the Project Owner hasn’t been emailed, then notify the Project Owner that their Project is published
  • Otherwise, email the project owner that the Project has been updated

Note that I’ve written a class that wraps the wp_mail function to add some additional functionality. In the code that you’re about to see, this is is referenced by the `$mailer` variable.

Here’s the code – I’ll discuss it after the block:

public function notify_project_owner( $post_id ) {

	if( 'project' == get_post_type() ) {

		// First, check to see if we're publishing this post and that it's not been previously emailed
		if( 'publish' == get_post_status( $post_id ) ) {

			// If the post doesn't yet exist, then we're creating it so we need to update the Project Owner
			if( 'true' != get_post_meta( $post_id, 'project_owner_emailed', true ) ) {

				$this->mailer->set_subject( __( 'Your Project Has Been Published!', 'cj' ) );
				$this->mailer->set_message( __( 'Need filler text.', 'cj' ) );
				$this->mailer->send_mail( $this->temp_email );

				// Mark that we've notified the project owner
				update_post_meta( $post_id, 'project_owner_emailed', 'true' );

			} // end if

		} elseif( 'inherit' == get_post_status( $post_id ) && ! isset( $_POST['project_is_completed'] ) && 'trash' != $_GET['action'] ) {

			$this->mailer->set_subject( __( 'Your Project Has Been Updated.', 'cj' ) );
			$this->mailer->set_message( __( 'Need filler text.', 'cj' ) );
			$this->mailer->send_mail( $this->temp_email );

		} // end if

	} // end if

} // end notify_project_owner

Though the comments should provide enough context, I’ll explain in detail here.

This function is attached to a save_post. If a Project is being saved, if first checks to see if the post is being published, and, if so, will then check the post’s meta data to see if the project_owner_email is set to true.

If it’s not set to true, then the email is sent and the post meta data is set.

Otherwise, if the project is being updated and has not been marked as completed (another set of post meta data), then the Project Owner should be notified that the Project has been updated.

Opportunities For Refactoring

There are two primary conditions that are too complex.

  1. The conditions are too complex.
  2. Each body of the conditional can be encapsulated into its own function.

Refactor The Conditional Evaluations

I generally try to make sure that my code reads as close to English as possible. It’s tough – I mean, it’s code – but that’s no excuse for not trying, right?

So first, I stub out the first conditional to read something that’s easier to read:

if( $this->post_is_published( $post_id ) ) {
	// Coming soon...
} // end if

Instead of:

if( 'true' != get_post_meta( $post_id, 'project_owner_emailed', true ) ) {
	// Snip Snip!
} // end if

And then I create a helper function that will allow me to encapsulate the evaluating into a single function:

private function post_is_published( $post_id ) {

	return
		'publish' == get_post_status( $post_id ) &&
		'true' != get_post_meta( $post_id, 'project_owner_emailed', true );

} // end post_is_published

Similarly, I needed to do the same for the conditional that was responsible for updating the user when the Project was updated.

So I took the following conditional:

if( 'inherit' == get_post_status( $post_id ) && ! isset( $_POST['project_is_completed'] ) && 'trash' != $_GET['action'] ) {
  // Snip Snip
}

Changed the call to this:

if( $this->post_is_updated( $post_id ) ) {
	// Coming soon...
} // end if

And introduced a new helper function:

private function post_is_updated( $post_id ) {

	 return
	 	'true' != get_post_meta( $post_id, 'project_owner_emailed', true ) &&
	 	'inherit' == get_post_status( $post_id ) &&
	 	! isset( $_POST['project_is_completed'] ) &&
	 	'trash' != $_GET['action'];

} // end post_is_updated

Refactor The Conditionals

Next, we can simplify the body of the conditional so that it’s in its own function thus making the evaluation of the conditionals even easier to read.

First, we start with the case when the user is first publishing a Project. We take the following block of code:

$this->mailer->set_subject( __( 'Your Project Has Been Published!', 'cj' ) );
$this->mailer->set_message( __( 'Need filler text.', 'cj' ) );
$this->mailer->send_mail( $this->temp_email );

// Mark that we've notified the project owner
update_post_meta( $post_id, 'project_owner_emailed', 'true' );

Then we move to code to a new function:

private function send_published_email( $post_id ) {

	$this->mailer->set_subject( __( 'Your Project Has Been Published!', 'cj' ) );
	$this->mailer->set_message( __( 'Need filler text.', 'cj' ) );
	$this->mailer->send_mail( $this->temp_email );

	// Mark that we've notified the project owner
	update_post_meta( $post_id, 'project_owner_emailed', 'true' );

 } // end send_published_email

Next, we do the same for the code that’s responsible for emailing users when the Project has been updated:

$this->mailer->set_subject( __( 'Your Project Has Been Updated.', 'cj' ) );
$this->mailer->set_message( __( 'Need filler text.', 'cj' ) );
$this->mailer->send_mail( $this->temp_email );

And then move it to its own function:

private function send_updated_email() {

	$this->mailer->set_subject( __( 'Your Project Has Been Updated.', 'cj' ) );
	$this->mailer->set_message( __( 'Need filler text.', 'cj' ) );
	$this->mailer->send_mail( $this->temp_email );

} // end send_updated_email

The End Result

At this point, we’re left with four new functions and one, smaller easier-to-read original function. Don’t mistake the number of functions for increasingly complexity, either.

Thanks to the helper functions mentioned above, the notify_project_owner function has been reduced to this:

public function notify_project_owner( $post_id ) {

	// We only care to notify the project owner if we're working with Projects
	if( 'project' == get_post_type() ) {

		// If the post is published, send the notification email
		if( $this->post_is_published( $post_id ) ) {
			$this->send_published_email( $post_id );
		} // end if

		// If the post is being updated, send the appropriate email
		if( $this->post_is_updated( $post_id ) ) {
			$this->send_updated_email();
		} // end if

	} // end if

} // end notify_project_owner

It’s significantly easier to read, isn’t it?

Despite actually adding code, we’ve actually simplified the process by giving each function a unique purpose. This makes it easier to read, easier to trace (or debug), and easier to maintain.

Sure, we’d love to be able to refactor every class and or set of functions that we come into contact with, but that’s not always a possibility, but I think that we can at least do the best with the time that we have available and the code that we have in front of us.

Truth is, the above could probably be refined even more. I just didn’t have the time :).