Refactoring code is when you restructure existing code without changing its external behavior. Basically your aim is to make \u201cbad\u201d code better without changing the underlying functionality, and one of the key ways you can do this is by making sure human beings can understand it. In this article, we\u2019ll look at some of the refactoring techniques you can use to make sure your code is clear, human-readable, and simpler to maintain.<\/p>\n\n
It should be noted that there are cases when the time spent refactoring would be better spent rewriting the code from scratch. Minor tweaks and refactors can improve maintainable code, but gutting over 50% of your code may mean a rewrite is more cost-effective. Refactoring incrementally over time avoids crossing this threshold and makes that part of the code easier to maintain in the future. A previous version of this article was written by Gilbert Pellegrom and published in 2016. This version largely retains his original code examples.<\/p>\n\n
There are a few principles you\u2019ll hear about again and again when it comes to refactoring. The YAGNI (“You Aren’t Gonna Need It”) principle suggests focusing on implementing only current requirements without anticipating hypothetical future needs that may never materialize. Closely related is KISS (“Keep It Simple, Stupid”), which emphasizes designing systems simply and intuitively. These are handy principles, but we\u2019ll start by keeping things DRY.<\/p>\n\n
Probably the<\/em> most popular programming principle you\u2019re likely to hear is \u201cDon\u2019t Repeat Yourself\u201d (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.<\/p>\n\n A good example of this is when different but similar classes require some of the same functionality. The example below defines two classes, Instead of duplicating the code across two different classes, you could create a parent or abstract class<\/a> which holds the common functionality, and then make the First, we declare an abstract Complex functions and methods can also lead to technical debt and hard-to-read code. One way to deal with this is to break up complicated functions into smaller chunks that are easy to understand (and test).<\/p>\n\n Here\u2019s an example of a complex function, a complete method for uploading a file from a WordPress site to S3 and handling pre-upload conditions, post-upload operations, and potential error situations.<\/p>\n\n This function is quite long. The code would be cleaner, more readable, and easier to maintain if we refactored to take a more object-oriented approach where the functionalities are abstracted into methods of a class. We can make the primary function, This is much easier to read and understand. It\u2019s amazing the difference simply splitting up large, complex bits of code can make to a codebase.\nAnother thing to note here is that you should use descriptive function names when splitting up complex functions like this. Remember, the aim is human readability. Trying to be too concise with your function names can actually make your code harder to understand. For example:<\/p>\n\n Is harder to understand than:<\/p>\n\n Have you ever seen big conditionals that look something like:<\/p>\n\n 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:<\/p>\n\n 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 known as declarative programming.<\/p>\n\n Another way to refactor complex conditionals is to use guard clauses. Guard clauses 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. Let\u2019s take a look at the complex function below, created without using guard clauses.<\/p>\n\n Here you can see how you could quickly end up in \u201cconditional hell\u201d if the method got any more complex. However, when we refactor to use guard clauses, it looks like this:<\/p>\n\n Now even if the method gets more complex, it\u2019s not going to be a maintenance headache down the road.<\/p>\n\n This is a slightly more advanced type of refactoring that is used more heavily in functional programming languages and libraries such as JavaScript, but can also be applied to PHP. Functions like The examples below were inspired by Adam Wathan\u2019s great screencast<\/a> on the subject and is based on using Laravel Collections. However, the example has been adapted to work with standard PHP functions. Speaking of Adam, his book \u201cRefactoring to Collections<\/a>\u201d is a great guide to writing clean and maintainable PHP.<\/p>\n\n Let\u2019s look at two common scenarios and see how they can be improved using functional methods. The example below fetches a bunch of The first thing we can improve is replacing the The second thing we can do is break down the big But we can actually go one step further here and use PHP\u2019s array_reduce<\/a> function to calculate the Putting it all together we now have:<\/p>\n\n Much better. No loops or conditionals in sight. You could imagine how, if more complexity was required to find This article just scrapes the surface of refactoring, concentrating on the human readability aspects. For a deeper dive, check out PHP The Right Way<\/a> or SourceMaking Refactoring guide<\/a>.<\/p>\n\n How highly do you value the human readability aspects of your code? Let us know in the comments.<\/p>\n","protected":false},"excerpt":{"rendered":"AwesomeAddon<\/code> and EvenMoreAwesomeAddon<\/code>, with the code duplicated in each class.<\/p>\n\n<?php\n\nclass AwesomeAddon {\n\n private $settings;\n\n public function __construct( $settings ) {\n $this->set_settings( $settings );\n }\n\n protected function set_settings( $settings ) {\n if ( ! is_array( $settings ) ) {\n throw new \\Exception( 'Invalid settings' );\n }\n\n $this->settings = $settings;\n }\n\n protected function do_something_awesome() {\n \/\/...\n }\n\n}\n\nclass EvenMoreAwesomeAddon {\n\n private $settings;\n\n public function __construct( $settings ) {\n $this->set_settings( $settings );\n }\n\n protected function set_settings( $settings ) {\n if ( ! is_array( $settings ) ) {\n throw new \\Exception( 'Invalid settings' );\n }\n\n $this->settings = $settings;\n }\n\n protected function do_something_even_more_awesome() {\n \/\/...\n }\n\n}\n<\/code><\/pre>\n\nAwesomeAddon<\/code> and EvenMoreAwesomeAddon<\/code> classes to inherit from it:<\/p>\n\n<?php\n\nabstract class Addon {\n\n protected $settings;\n\n protected function set_settings( $settings ) {\n if ( ! is_array( $settings ) ) {\n throw new \\Exception( 'Invalid settings' );\n }\n\n $this->settings = $settings;\n }\n\n}\n\nclass AwesomeAddon extends Addon {\n\n public function __construct( $settings ) {\n $this->set_settings( $settings );\n }\n\n protected function do_something_awesome() {\n \/\/...\n }\n\n}\n\nclass EvenMoreAwesomeAddon extends Addon {\n\n public function __construct( $settings ) {\n $this->set_settings( $settings );\n }\n\n protected function do_something_even_more_awesome() {\n \/\/...\n }\n\n}\n<\/code><\/pre>\n\nAddon<\/code> class, which captures the common code for $settings<\/code> in AwesomeAddon<\/code> and EvenMoreAwesomeAddon<\/code>. Next, we added array<\/code> type before $settings<\/code> in __construct<\/code> and set_settings<\/code> methods, and void<\/code> return type in setSettings<\/code> method. This ensures we pass an array of settings when we create an instance and that function doesn’t return anything. We\u2019ve also removed the exception for when $settings<\/code> is not an array, because the array type hint on set_settings<\/code> method will throw an exception automatically now if anything other than an array is passed to it.<\/p>\n\nSplitting up Complex Functions<\/h2>\n\n
\u201cPrograms must be written for people to read, and only incidentally for machines to execute.\u201d – Harold Abelson<\/blockquote>\n\n
<?php\nfunction upload_attachment_to_s3( $post_id, $data = null, $file_path = null, $force_new_s3_client = false, $remove_local_files = true ) {\n $return_metadata = null;\n if ( is_null( $data ) ) {\n $data = wp_get_attachment_metadata( $post_id, true );\n } else {\n \/\/ As we have passed in the meta, return it later\n $return_metadata = $data;\n }\n\n if ( is_wp_error( $data ) ) {\n return $data;\n }\n\n \/\/ Allow S3 upload to be hijacked\/canceled for any reason\n $pre = apply_filters( 'as3cf_pre_upload_attachment', false, $post_id, $data );\n if ( false !== $pre ) {\n if ( ! is_null( $return_metadata ) ) {\n \/\/ If the attachment metadata is supplied, return it\n return $data;\n }\n\n $error_msg = is_string( $pre ) ? $pre : __( 'Upload aborted by filter \\'as3cf_pre_upload_attachment\\'', 'amazon-s3-and-cloudfront' );\n\n return $this->return_upload_error( $error_msg );\n }\n\n if ( is_null( $file_path ) ) {\n $file_path = get_attached_file( $post_id, true );\n }\n\n \/\/ Check file exists locally before attempting upload\n if ( ! file_exists( $file_path ) ) {\n $error_msg = sprintf( __( 'File %s does not exist', 'amazon-s3-and-cloudfront' ), $file_path );\n\n return $this->return_upload_error( $error_msg, $return_metadata );\n }\n\n $file_name = basename( $file_path );\n $type = get_post_mime_type( $post_id );\n $allowed_types = $this->get_allowed_mime_types();\n\n \/\/ Check mime type of file is in allowed S3 mime types\n if ( ! in_array( $type, $allowed_types ) ) {\n $error_msg = sprintf( __( 'Mime type %s is not allowed', 'amazon-s3-and-cloudfront' ), $type );\n\n return $this->return_upload_error( $error_msg, $return_metadata );\n }\n\n $acl = self::DEFAULT_ACL;\n\n \/\/ Check the attachment already exists in S3, eg., edit or restore image\n if ( ( $old_s3object = $this->get_attachment_s3_info( $post_id ) ) ) {\n \/\/ use existing non default ACL if attachment already exists\n if ( isset( $old_s3object['acl'] ) ) {\n $acl = $old_s3object['acl'];\n }\n \/\/ Use existing prefix\n $prefix = dirname( $old_s3object['key'] );\n $prefix = ( '.' === $prefix ) ? '' : $prefix . '\/';\n \/\/ Use existing bucket\n $bucket = $old_s3object['bucket'];\n \/\/ Get existing region\n if ( isset( $old_s3object['region'] ) ) {\n $region = $old_s3object['region'];\n };\n } else {\n \/\/ Derive prefix from various settings\n if ( isset( $data['file'] ) ) {\n $time = $this->get_folder_time_from_url( $data['file'] );\n } else {\n $time = $this->get_attachment_folder_time( $post_id );\n $time = date( 'Y\/m', $time );\n }\n\n $prefix = $this->get_file_prefix( $time );\n\n \/\/ Use bucket from settings\n $bucket = $this->get_setting( 'bucket' );\n $region = $this->get_setting( 'region' );\n if ( is_wp_error( $region ) ) {\n $region = '';\n }\n }\n\n $acl = apply_filters( 'as3cf_upload_acl', $acl, $data, $post_id );\n\n $s3object = array(\n 'bucket' => $bucket,\n 'key' => $prefix . $file_name,\n 'region' => $region,\n );\n\n \/\/ Store acl if not default\n if ( $acl != self::DEFAULT_ACL ) {\n $s3object['acl'] = $acl;\n }\n\n $s3client = $this->get_s3client( $region, $force_new_s3_client );\n\n $args = array(\n 'Bucket' => $bucket,\n 'Key' => $prefix . $file_name,\n 'SourceFile' => $file_path,\n 'ACL' => $acl,\n 'ContentType' => $type,\n 'CacheControl' => 'max-age=31536000',\n 'Expires' => date( 'D, d M Y H:i:s O', time() + 31536000 ),\n );\n\n $args = apply_filters( 'as3cf_object_meta', $args, $post_id );\n\n $files_to_remove = array();\n\n if ( file_exists( $file_path ) ) {\n $files_to_remove[] = $file_path;\n try {\n $s3client->putObject( $args );\n } catch ( Exception $e ) {\n $error_msg = sprintf( __( 'Error uploading %s to S3: %s', 'amazon-s3-and-cloudfront' ), $file_path, $e->getMessage() );\n\n return $this->return_upload_error( $error_msg, $return_metadata );\n }\n }\n\n delete_post_meta( $post_id, 'amazonS3_info' );\n\n add_post_meta( $post_id, 'amazonS3_info', $s3object );\n\n $file_paths = $this->get_attachment_file_paths( $post_id, true, $data );\n $additional_images = array();\n\n $filesize_total = 0;\n $remove_local_files_setting = $this->get_setting( 'remove-local-file' );\n\n if ( $remove_local_files_setting ) {\n $bytes = filesize( $file_path );\n if ( false !== $bytes ) {\n \/\/ Store in the attachment metadata for use by WP\n $data['filesize'] = $bytes;\n\n if ( is_null( $return_metadata ) ) {\n \/\/ Update metadata with filesize\n update_post_meta( $post_id, '_wp_attachment_metadata', $data );\n }\n\n \/\/ Add to the file size total\n $filesize_total += $bytes;\n }\n }\n\n foreach ( $file_paths as $file_path ) {\n if ( ! in_array( $file_path, $files_to_remove ) ) {\n $additional_images[] = array(\n 'Key' => $prefix . basename( $file_path ),\n 'SourceFile' => $file_path,\n );\n\n $files_to_remove[] = $file_path;\n\n if ( $remove_local_files_setting ) {\n \/\/ Record the file size for the additional image\n $bytes = filesize( $file_path );\n if ( false !== $bytes ) {\n $filesize_total += $bytes;\n }\n }\n }\n }\n\n if ( $remove_local_files ) {\n if ( $remove_local_files_setting ) {\n \/\/ Allow other functions to remove files after they have processed\n $files_to_remove = apply_filters( 'as3cf_upload_attachment_local_files_to_remove', $files_to_remove, $post_id, $file_path );\n \/\/ Remove duplicates\n $files_to_remove = array_unique( $files_to_remove );\n\n \/\/ Delete the files\n $this->remove_local_files( $files_to_remove );\n }\n }\n\n \/\/ Store the file size in the attachment meta if we are removing local file\n if ( $remove_local_files_setting ) {\n if ( $filesize_total > 0 ) {\n \/\/ Add the total file size for all image sizes\n update_post_meta( $post_id, 'wpos3_filesize_total', $filesize_total );\n }\n } else {\n if ( isset( $data['filesize'] ) ) {\n \/\/ Make sure we don't have a cached file sizes in the meta\n unset( $data['filesize'] );\n\n if ( is_null( $return_metadata ) ) {\n \/\/ Remove the filesize from the metadata\n update_post_meta( $post_id, '_wp_attachment_metadata', $data );\n }\n\n delete_post_meta( $post_id, 'wpos3_filesize_total' );\n }\n }\n\n if ( ! is_null( $return_metadata ) ) {\n \/\/ If the attachment metadata is supplied, return it\n return $data;\n }\n\n return $s3object;\n}\n<\/code><\/pre>\n\nupload_attachment_to_s3<\/code>, much easier to read by abstracting every component of the task into its own method:<\/p>\n\n<?php\nfunction upload_attachment_to_s3( $post_id, $data = null, $file_path = null, $force_new_s3_client = false, $remove_local_files = true ) {\n $return_metadata = $this->get_attachment_metadata( $post_id );\n if ( is_wp_error( $return_metadata ) ) {\n return $return_metadata;\n }\n\n \/\/ Allow S3 upload to be hijacked\/canceled for any reason\n $pre = apply_filters( 'as3cf_pre_upload_attachment', false, $post_id, $data );\n if ( $this->upload_should_be_cancelled( $pre ) ) {\n return $pre;\n }\n\n \/\/ Check file exists locally before attempting upload\n if ( ! $this->local_file_exists() ) {\n $error_msg = sprintf( __( 'File %s does not exist', 'amazon-s3-and-cloudfront' ), $file_path );\n\n return $this->return_upload_error( $error_msg, $return_metadata );\n }\n\n \/\/ Check mime type of file is in allowed S3 mime types\n if ( ! $this->is_valid_mime_type() ) {\n $error_msg = sprintf( __( 'Mime type %s is not allowed', 'amazon-s3-and-cloudfront' ), $type );\n\n return $this->return_upload_error( $error_msg, $return_metadata );\n }\n\n $s3object = $this->get_attachment_s3_info( $post_id );\n $acl = $this->get_s3object_acl( $s3object );\n\n $s3client = $this->get_s3client( $region, $force_new_s3_client );\n\n $args = array(\n 'Bucket' => $s3object['bucket'],\n 'Key' => $s3object['key'],\n 'SourceFile' => $s3object['source_file'],\n 'ACL' => $acl,\n 'ContentType' => $s3object['mime_type'],\n 'CacheControl' => 'max-age=31536000',\n 'Expires' => date( 'D, d M Y H:i:s O', time() + 31536000 ),\n );\n\n $s3client->putObject( $args );\n\n $this->maybe_remove_files( $args, $s3object );\n\n return $s3object;\n}\n<\/code><\/pre>\n\n\n$this->get_att_inf( $post_id );\n<\/code><\/pre>\n\n$this->get_attachment_s3_info( $post_id );\n<\/code><\/pre>\n\nSplitting Up Complex Conditionals<\/h2>\n\n
<?php\n\nif ( isset( $settings['wp-uploads'] ) && $settings['wp-uploads'] && in_array( $key, array( 'copy-to-s3', 'serve-from-s3' ) ) ) {\n return '1';\n}\n<\/code><\/pre>\n\n<?php\n\nif ( upload_is_valid( $settings, $key ) ) {\n return '1';\n}\n\nfunction upload_is_valid( $settings, $key ) {\n return isset( $settings['wp-uploads'] ) && $settings['wp-uploads'] && in_array( $key, array( 'copy-to-s3', 'serve-from-s3' ) );\n}\n<\/code><\/pre>\n\nReplacing Nested Conditionals with Guard Clauses<\/h2>\n\n
<?php\n\nfunction get_setting( $key, $default = '' ) {\n $settings = $this->get_settings();\n\n \/\/ If legacy setting set, migrate settings\n if ( isset( $settings['wp-uploads'] ) && $settings['wp-uploads'] && in_array( $key, array( 'copy-to-s3', 'serve-from-s3' ) ) ) {\n return $default;\n } else {\n \/\/ Turn on object versioning by default\n if ( 'object-versioning' == $key && ! isset( $settings['object-versioning'] ) ) {\n return $default;\n } else {\n \/\/ Default object prefix\n if ( 'object-prefix' == $key && ! isset( $settings['object-prefix'] ) ) {\n return $this->get_default_object_prefix();\n } else {\n if ( 'use-yearmonth-folders' == $key && ! isset( $settings['use-yearmonth-folders'] ) ) {\n return get_option( 'uploads_use_yearmonth_folders' );\n } else {\n $value = parent::get_setting( $key, $default );\n return apply_filters( 'as3cf_setting_' . $key, $value );\n }\n }\n }\n }\n\n return $default;\n}\n<\/code><\/pre>\n\n<?php\n\nfunction get_setting( $key, $default = '' ) {\n $settings = $this->get_settings();\n\n \/\/ If legacy setting set, migrate settings\n if ( isset( $settings['wp-uploads'] ) && $settings['wp-uploads'] && in_array( $key, array( 'copy-to-s3', 'serve-from-s3' ) ) ) {\n return $default;\n }\n\n \/\/ Turn on object versioning by default\n if ( 'object-versioning' == $key && ! isset( $settings['object-versioning'] ) ) {\n return $default;\n }\n\n \/\/ Default object prefix\n if ( 'object-prefix' == $key && ! isset( $settings['object-prefix'] ) ) {\n return $this->get_default_object_prefix();\n }\n\n \/\/ Default use year and month folders\n if ( 'use-yearmonth-folders' == $key && ! isset( $settings['use-yearmonth-folders'] ) ) {\n return get_option( 'uploads_use_yearmonth_folders' );\n }\n\n $value = parent::get_setting( $key, $default );\n\n return apply_filters( 'as3cf_setting_' . $key, $value );\n}\n<\/code><\/pre>\n\nRefactoring Loops and Conditionals Using Functional Methods<\/h2>\n\n
map<\/code> and reduce<\/code> can dramatically improve the readability of your code.<\/p>\n\n$events<\/code> from an API and calculates a score based on the event type:<\/p>\n\n<?php\n\n$events = file_get_contents( 'https:\/\/someapi.com\/events' );\n\n$types = array();\nforeach ( $events as $event ) {\n $types[] = $event->type;\n}\n\n$score = 0;\n\nforeach ( $types as $type ) {\n switch ( $type ) {\n case 'type1':\n $score += 2;\n break;\n case 'type2':\n $score += 5;\n break;\n case 'type3':\n $score += 10;\n break;\n default:\n $score += 1;\n break;\n }\n}\n<\/code><\/pre>\n\nforeach<\/code> loop with a map<\/code> function. A map function can be used when you want to create a new array from an existing array. In our example we\u2019re creating a $types<\/code> array from the $events<\/code> array. PHP has an array_map<\/a> function that will let us write the first foreach<\/code> in the above code like this:<\/p>\n\n<?php\n\n$types = array_map( function( $event ) {\n return $event->type;\n}, $events );\n<\/code><\/pre>\n\nswitch<\/code> statement so it\u2019s a bit easier to process.<\/p>\n\n<?php\n\n$scores = array(\n 'type1' => 2,\n 'type2' => 5,\n 'type3' => 10,\n);\n\n$score = 0;\n\nforeach ( $types as $type ) {\n $score += isset( $scores[$type] ) ? $scores[$type] : 1;\n}\n\n<\/code><\/pre>\n\n$score<\/code> in a single function. A reduce function takes an array of values and reduces them to a single value. We\u2019re going to do that here to calculate our $score<\/code>.<\/p>\n\n<?php\n\n$scores = array(\n 'type1' => 2,\n 'type2' => 5,\n 'type3' => 10,\n);\n\n$score = array_reduce( $types, function( $result, $type ) use ( $scores ) {\n return $result += isset( $scores[$type] ) ? $scores[$type] : 1;\n} );\n<\/code><\/pre>\n\n<?php\n\n$events = file_get_contents( 'https:\/\/someapi.com\/events' );\n\n$types = array_map( function( $event ) {\n return $event->type;\n}, $events );\n\n$scores = array(\n 'type1' => 2,\n 'type2' => 5,\n 'type3' => 10,\n);\n\n$score = array_reduce( $types, function( $result, $type ) use ( $scores ) {\n return $result += isset( $scores[$type] ) ? $scores[$type] : 1;\n} );\n<\/code><\/pre>\n\n$types<\/code> or calculate the $score<\/code> down the line, it would be easy to refactor the map<\/code> and reduce<\/code> function calls into separate methods. This is now possible because we’ve already reduced the complexity down to a function.<\/p>\n\nFurther Reading<\/h2>\n\n