Breaking

Wednesday, January 9, 2019

Examples of Refactoring PHP Code for Better Readability by Perfect Web Solutions

Refactoring code is when you restructure existing code without changing its external behavior.Basically, your aim is to make “bad” code better without changing the underlying functionality.
There are plenty of guides out there on refactoring your code. However, I find many of them talk about the ideology of well written and structured code without actually showing you what it looks like to refactor your “bad” code into “good” code. They might talk about high-level concepts like readability, extensibility, maintainability, testability, reduced complexity etc., which are all valid aims of the refactoring process, but they often fail to show examples of what this looks like in reality.
In this article I’m not going to talk about when you should refactor (personally I believe you should do it whenever you come across bad code), neither am I going to talk about why you should refactor (it reduces technical debt). Rather I want to look at some common, practical principles you can apply when refactoring and give examples of what they look like with real code examples. For the purposes of this article I’ll be using PHP code (as WordPress is written in PHP) but these principles will apply to any programming language.

Don’t Repeat Yourself (DRY)

Probably the most popular programming principle you’re likely to hear is “Don’t Repeat Yourself” (DRY). If you find yourself duplicating the same code a couple of times, then you should probably encapsulate the functionality in its own class or function and use said class or function to avoid repetition. This means you only need to fix the code in one place when the inevitable bug arises months down the line.
A good example of this is when two different but similar classes require some of the same functionality. Instead of duplicating the code across two different classes, you could create an abstract class which holds the common functionality and then make the other two classes extend the abstract class.
For example:
<?php
class AwesomeAddon {
private $settings;
public function __construct( $settings ) {
$this->set_settings( $settings );
}
protected function set_settings( $settings ) {
if ( ! is_array( $settings ) ) {
throw new \Exception( 'Invalid settings' );
}
$this->settings = $settings;
}
protected function do_something_awesome() {
//...
}
}
class EvenMoreAwesomeAddon {
private $settings;
public function __construct( $settings ) {
$this->set_settings( $settings );
}
protected function set_settings( $settings ) {
if ( ! is_array( $settings ) ) {
throw new \Exception( 'Invalid settings' );
}
$this->settings = $settings;
}
protected function do_something_even_more_awesome() {
//...
}
}
view rawdry1.php hosted with ❤ by GitHub
Becomes:
<?php
abstract class Addon {
protected $settings;
protected function set_settings( $settings ) {
if ( ! is_array( $settings ) ) {
throw new \Exception( 'Invalid settings' );
}
$this->settings = $settings;
}
}
class AwesomeAddon extends Addon {
public function __construct( $settings ) {
$this->set_settings( $settings );
}
protected function do_something_awesome() {
//...
}
}
class EvenMoreAwesomeAddon extends Addon {
public function __construct( $settings ) {
$this->set_settings( $settings );
}
protected function do_something_even_more_awesome() {
//...
}
}
view rawdry2.php hosted with ❤ by GitHub
This is just a simple example of how you can structure your code to avoid duplication.

Splitting up Complex Functions

Another common programming problem that can lead to technical debt and hard-to-read code is complex functions or methods.
“Programs must be written for people to read, and only incidentally for machines to execute.” – Harold Abelson
Making your code easy for other people to read and understand is of utmost importance, so one way to deal with complex functions is to break them up into smaller more understandable (and testable) chunks.
Here’s an example of a complex function. Don’t worry about understanding everything it does. Just notice how complex it looks to glance at.
<?php
function upload_attachment_to_s3( $post_id, $data = null, $file_path = null, $force_new_s3_client = false, $remove_local_files = true ) {
$return_metadata = null;
if ( is_null( $data ) ) {
$data = wp_get_attachment_metadata( $post_id, true );
} else {
// As we have passed in the meta, return it later
$return_metadata = $data;
}
if ( is_wp_error( $data ) ) {
return $data;
}
// Allow S3 upload to be hijacked / cancelled for any reason
$pre = apply_filters( 'as3cf_pre_upload_attachment', false, $post_id, $data );
if ( false !== $pre ) {
if ( ! is_null( $return_metadata ) ) {
// If the attachment metadata is supplied, return it
return $data;
}
$error_msg = is_string( $pre ) ? $pre : __( 'Upload aborted by filter \'as3cf_pre_upload_attachment\'', 'amazon-s3-and-cloudfront' );
return $this->return_upload_error( $error_msg );
}
if ( is_null( $file_path ) ) {
$file_path = get_attached_file( $post_id, true );
}
// Check file exists locally before attempting upload
if ( ! file_exists( $file_path ) ) {
$error_msg = sprintf( __( 'File %s does not exist', 'amazon-s3-and-cloudfront' ), $file_path );
return $this->return_upload_error( $error_msg, $return_metadata );
}
$file_name = basename( $file_path );
$type = get_post_mime_type( $post_id );
$allowed_types = $this->get_allowed_mime_types();
// check mime type of file is in allowed S3 mime types
if ( ! in_array( $type, $allowed_types ) ) {
$error_msg = sprintf( __( 'Mime type %s is not allowed', 'amazon-s3-and-cloudfront' ), $type );
return $this->return_upload_error( $error_msg, $return_metadata );
}
$acl = self::DEFAULT_ACL;
// check the attachment already exists in S3, eg. edit or restore image
if ( ( $old_s3object = $this->get_attachment_s3_info( $post_id ) ) ) {
// use existing non default ACL if attachment already exists
if ( isset( $old_s3object['acl'] ) ) {
$acl = $old_s3object['acl'];
}
// use existing prefix
$prefix = dirname( $old_s3object['key'] );
$prefix = ( '.' === $prefix ) ? '' : $prefix . '/';
// use existing bucket
$bucket = $old_s3object['bucket'];
// get existing region
if ( isset( $old_s3object['region'] ) ) {
$region = $old_s3object['region'];
};
} else {
// derive prefix from various settings
if ( isset( $data['file'] ) ) {
$time = $this->get_folder_time_from_url( $data['file'] );
} else {
$time = $this->get_attachment_folder_time( $post_id );
$time = date( 'Y/m', $time );
}
$prefix = $this->get_file_prefix( $time );
// use bucket from settings
$bucket = $this->get_setting( 'bucket' );
$region = $this->get_setting( 'region' );
if ( is_wp_error( $region ) ) {
$region = '';
}
}
$acl = apply_filters( 'as3cf_upload_acl', $acl, $data, $post_id );
$s3object = array(
'bucket' => $bucket,
'key' => $prefix . $file_name,
'region' => $region,
);
// store acl if not default
if ( $acl != self::DEFAULT_ACL ) {
$s3object['acl'] = $acl;
}
$s3client = $this->get_s3client( $region, $force_new_s3_client );
$args = array(
'Bucket' => $bucket,
'Key' => $prefix . $file_name,
'SourceFile' => $file_path,
'ACL' => $acl,
'ContentType' => $type,
'CacheControl' => 'max-age=31536000',
'Expires' => date( 'D, d M Y H:i:s O', time() + 31536000 ),
);
$args = apply_filters( 'as3cf_object_meta', $args, $post_id );
$files_to_remove = array();
if ( file_exists( $file_path ) ) {
$files_to_remove[] = $file_path;
try {
$s3client->putObject( $args );
} catch ( Exception $e ) {
$error_msg = sprintf( __( 'Error uploading %s to S3: %s', 'amazon-s3-and-cloudfront' ), $file_path, $e->getMessage() );
return $this->return_upload_error( $error_msg, $return_metadata );
}
}
delete_post_meta( $post_id, 'amazonS3_info' );
add_post_meta( $post_id, 'amazonS3_info', $s3object );
$file_paths = $this->get_attachment_file_paths( $post_id, true, $data );
$additional_images = array();
$filesize_total = 0;
$remove_local_files_setting = $this->get_setting( 'remove-local-file' );
if ( $remove_local_files_setting ) {
$bytes = filesize( $file_path );
if ( false !== $bytes ) {
// Store in the attachment meta data for use by WP
$data['filesize'] = $bytes;
if ( is_null( $return_metadata ) ) {
// Update metadata with filesize
update_post_meta( $post_id, '_wp_attachment_metadata', $data );
}
// Add to the file size total
$filesize_total += $bytes;
}
}
foreach ( $file_paths as $file_path ) {
if ( ! in_array( $file_path, $files_to_remove ) ) {
$additional_images[] = array(
'Key' => $prefix . basename( $file_path ),
'SourceFile' => $file_path,
);
$files_to_remove[] = $file_path;
if ( $remove_local_files_setting ) {
// Record the file size for the additional image
$bytes = filesize( $file_path );
if ( false !== $bytes ) {
$filesize_total += $bytes;
}
}
}
}
if ( $remove_local_files ) {
if ( $remove_local_files_setting ) {
// Allow other functions to remove files after they have processed
$files_to_remove = apply_filters( 'as3cf_upload_attachment_local_files_to_remove', $files_to_remove, $post_id, $file_path );
// Remove duplicates
$files_to_remove = array_unique( $files_to_remove );
// Delete the files
$this->remove_local_files( $files_to_remove );
}
}
// Store the file size in the attachment meta if we are removing local file
if ( $remove_local_files_setting ) {
if ( $filesize_total > 0 ) {
// Add the total file size for all image sizes
update_post_meta( $post_id, 'wpos3_filesize_total', $filesize_total );
}
} else {
if ( isset( $data['filesize'] ) ) {
// Make sure we don't have a cached file sizes in the meta
unset( $data['filesize'] );
if ( is_null( $return_metadata ) ) {
// Remove the filesize from the metadata
update_post_meta( $post_id, '_wp_attachment_metadata', $data );
}
delete_post_meta( $post_id, 'wpos3_filesize_total' );
}
}
if ( ! is_null( $return_metadata ) ) {
// If the attachment metadata is supplied, return it
return $data;
}
return $s3object;
}
Wouldn’t it be much easier if that entire function looked like this:
<?php
function upload_attachment_to_s3( $post_id, $data = null, $file_path = null, $force_new_s3_client = false, $remove_local_files = true ) {
$return_metadata = $this->get_attachment_metadata( $post_id );
if ( is_wp_error( $return_metadata ) ) {
return $return_metadata;
}
// Allow S3 upload to be hijacked / cancelled for any reason
$pre = apply_filters( 'as3cf_pre_upload_attachment', false, $post_id, $data );
if ( $this->upload_should_be_cancelled( $pre ) ) {
return $pre;
}
// Check file exists locally before attempting upload
if ( ! $this->local_file_exists() ) {
$error_msg = sprintf( __( 'File %s does not exist', 'amazon-s3-and-cloudfront' ), $file_path );
return $this->return_upload_error( $error_msg, $return_metadata );
}
// check mime type of file is in allowed S3 mime types
if ( ! $this->is_valid_mime_type() ) {
$error_msg = sprintf( __( 'Mime type %s is not allowed', 'amazon-s3-and-cloudfront' ), $type );
return $this->return_upload_error( $error_msg, $return_metadata );
}
$s3object = $this->get_attachment_s3_info( $post_id );
$acl = $this->get_s3object_acl( $s3object );
$s3client = $this->get_s3client( $region, $force_new_s3_client );
$args = array(
'Bucket' => $s3object['bucket'],
'Key' => $s3object['key'],
'SourceFile' => $s3object['source_file'],
'ACL' => $acl,
'ContentType' => $s3object['mime_type'],
'CacheControl' => 'max-age=31536000',
'Expires' => date( 'D, d M Y H:i:s O', time() + 31536000 ),
);
$s3client->putObject( $args );
$this->maybe_remove_files( $args, $s3object );
return $s3object;
}
This is much easier to read and understand. It’s amazing the difference simply splitting up large, complex bits of code can make to a codebase.
Another thing to note here is that you shouldn’t worry too much about having long, descriptive function names when splitting up complex functions like this. Remember that the aim is human readability, so trying to be too concise with your function names can actually make your code harder to understand. For example:
$this->get_att_inf( $post_id );
Is harder to understand than:
$this->get_attachment_s3_info( $post_id );

Splitting Up Complex Conditionals

Have you ever seen big conditionals that look something like:
<?php
if ( isset( $settings['wp-uploads'] ) && $settings['wp-uploads'] && in_array( $key, array( 'copy-to-s3', 'serve-from-s3' ) ) ) {
return '1';
}
Long pieces of code with conditionals are hard to read and understand. A simple solution to this is to extract the conditional code into clearly named methods. For example:
<?php
if ( upload_is_valid( $settings, $key ) ) {
return '1';
}
function upload_is_valid( $settings, $key ) {
return isset( $settings['wp-uploads'] ) && $settings['wp-uploads'] && in_array( $key, array( 'copy-to-s3', 'serve-from-s3' ) );
}
This makes your code far easier to understand for a future maintainer. As long as the conditional method is clearly named it should be easy to understand what it does without inspecting the actual method code. This practice is also known as Declarative Programming.

Replacing Nested Conditionals with Guard Clauses

Another way to refactor complex conditionals is to use what are known as “guard clauses”. Guard clauses simply extract all conditionals that lead to calling an exception or immediate return of a value from the method and place it at the beginning of the method. For example:
<?php
function get_setting( $key, $default = '' ) {
$settings = $this->get_settings();
// If legacy setting set, migrate settings
if ( isset( $settings['wp-uploads'] ) && $settings['wp-uploads'] && in_array( $key, array( 'copy-to-s3', 'serve-from-s3' ) ) ) {
return $default;
} else {
// Turn on object versioning by default
if ( 'object-versioning' == $key && ! isset( $settings['object-versioning'] ) ) {
return $default;
} else {
// Default object prefix
if ( 'object-prefix' == $key && ! isset( $settings['object-prefix'] ) ) {
return $this->get_default_object_prefix();
} else {
if ( 'use-yearmonth-folders' == $key && ! isset( $settings['use-yearmonth-folders'] ) ) {
return get_option( 'uploads_use_yearmonth_folders' );
} else {
$value = parent::get_setting( $key, $default );
return apply_filters( 'as3cf_setting_' . $key, $value );
}
}
}
}
return $default;
}
view rawguard-clauses1.php hosted with ❤ by GitHub
Here you can see how quickly you could end up “conditional hell” if the method got any more complex. However if we refactored this method to use guard clauses it would look like this:
<?php
function get_setting( $key, $default = '' ) {
$settings = $this->get_settings();
// If legacy setting set, migrate settings
if ( isset( $settings['wp-uploads'] ) && $settings['wp-uploads'] && in_array( $key, array( 'copy-to-s3', 'serve-from-s3' ) ) ) {
return $default;
}
// Turn on object versioning by default
if ( 'object-versioning' == $key && ! isset( $settings['object-versioning'] ) ) {
return $default;
}
// Default object prefix
if ( 'object-prefix' == $key && ! isset( $settings['object-prefix'] ) ) {
return $this->get_default_object_prefix();
}
// Default use year and month folders
if ( 'use-yearmonth-folders' == $key && ! isset( $settings['use-yearmonth-folders'] ) ) {
return get_option( 'uploads_use_yearmonth_folders' );
}
$value = parent::get_setting( $key, $default );
return apply_filters( 'as3cf_setting_' . $key, $value );
}
view rawguard-clauses2.php hosted with ❤ by GitHub
Now even if the method gets more complex, it’s not going to be a maintenance headache down the road.

Refactoring Loops and Conditionals Using Functional Methods

This is a slightly more advanced type of refactoring that is used more heavily in functional programming languages and libraries (these kind of functions are used a lot in JavaScript land). You may have heard of functions like map and reduce and wondered what they are and how to use them. It turns out that these methods can dramatically improve the readability of your code.
This example is inspired by Adam Wathan’s great screencast on the subject (you should check out his upcoming book) and is based on using Laravel Collections. However, I’ve adapted the example to work with standard PHP functions.
Let’s look at two common scenarios and see how they can be improved using functional methods. Our example below fetches a bunch of $events from an API and calculates a score based on the event type:
<?php
$events = file_get_contents( 'https://someapi.com/events' );
$types = array();
foreach ( $events as $event ) {
$types[] = $event->type;
}
$score = 0;
foreach ( $types as $type ) {
switch ( $type ) {
case 'type1':
$score += 2;
break;
case 'type2':
$score += 5;
break;
case 'type3':
$score += 10;
break;
default:
$score += 1;
break;
}
}
The first thing we can improve is replacing the foreach loop with a map function. A map function can be used when you want to create a new array from an existing array. In our example we’re creating a $types array from the $events array. PHP has an array_map function that will let us write the first foreach in the above code like this:
<?php
$types = array_map( function( $event ) {
return $event->type;
}, $events );
Note: You need to be running PHP 5.3+ to be able to use anonymous functions like this.
The second thing we can do is break down the big switch statement so that it’s a bit easier to process.
<?php
$scores = array(
'type1' => 2,
'type2' => 5,
'type3' => 10,
);
$score = 0;
foreach ( $types as $type ) {
$score += isset( $scores[$type] ) ? $scores[$type] : 1;
}
But we can actually go one step further here and use PHP’s array_reduce function to calculate the $score in a single function. A reduce function takes an array of values and reduces them to a single value. We’re going to do that here to calculate our $score.
<?php
$scores = array(
'type1' => 2,
'type2' => 5,
'type3' => 10,
);
$score = array_reduce( $types, function( $result, $type ) use ( $scores ) {
return $result += isset( $scores[$type] ) ? $scores[$type] : 1;
} );
Putting it all together we now have:
<?php
$events = file_get_contents( 'https://someapi.com/events' );
$types = array_map( function( $event ) {
return $event->type;
}, $events );
$scores = array(
'type1' => 2,
'type2' => 5,
'type3' => 10,
);
$score = array_reduce( $types, function( $result, $type ) use ( $scores ) {
return $result += isset( $scores[$type] ) ? $scores[$type] : 1;
} );
Much better. No loops or conditionals in sight. You could imagine how, if more complexity was required to find $types or calculate the $score down the line, it would be easy to refactor the map and reduce function calls into separate methods. This is now possible because we’ve already reduced the complexity down to a function.

No comments:

Post a Comment