Last Update: June 9, 2017
To new designers or seasoned developers starting to think more about design, the SOLID principles can seem too academic and can be hard to remember. They apply mostly at the class level, whereas object-oriented programming usually occurs at the method level. So, some of the principles can be hard to think about during development.
Glenn Vanderburg gave a talk about this in 2008 titled Tactical Design. In it, he proposes another set of five rules - well, two rules and three principles. These rules are easier to remember and put into practice as they are simpler and focus mostly on method-level concerns.
The five rules of tactical design are:
- Few Lines Per Method
- Few Methods Per Class
- Single Responsibility Principle (SRP)
- Don’t Repeat Yourself (DRY)
- Uniform Level of Abstraction (ULA)
I’m going to explain the rules, show some code that violates them, and start refactoring it into a better design.
1. Few Lines Per Method
We’ve all seen methods with over 100 lines. Long methods are hard to understand and hard to change. Keeping your methods short can help facilitate a better design as it will force you to think about how to separate your code. How long is too long? Setting an arbitrary maximum length (five or ten lines) can help get you started.
2. Few Methods Per Class
Stepping back one level, we’ve also seen classes with too many methods. They get labeled as “God Objects” because they appear to do everything. Most systems end up with at least one god object. These classes are hard to understand because we don’t know what their intended use is. Also, like a religion that’s been around for thousands of years, they are very resistant to change. Keeping our classes small can lead to better design.
3. Single Responsibility Principle (SRP)
The SRP is the first of the SOLID principles and arguably the easiest to understand. It means that a class or method should “do one thing” or “have one reason to change”. Whether or not a class or method adheres to the principle depends on how you define its responsibility. For example, a class for managing an application’s configuration does one thing (manages the configuration!), but it may have multiple reasons to change: switching from XML to YAML, adding a new configurable setting, etc. Or, maybe that means it violates the principle!
4. Don’t Repeat Yourself (DRY)
The DRY principle is the third of Kent Beck’s Four Rules of Simple Design. You probably first encountered it when learning about inheritance - moving code shared by multiple types of objects into a parent class. It’s easiest to put into practice by thinking of it as “no duplication”. It is easier to change code that isn’t repeated multiple times throughout a system. When you identify duplicated code, you’re forced to think about the right place to put it.
5. Uniform Level of Abstraction (ULA)
This last rule may sound academic, but it’s rather simple. Take the following grocery list for example: chips, pretzels, 8 oz. container of Kraft brand cream cheese. The first two items are generic. The last one is specific. The list does not read well. The same thing can happen with your code. Try to keep each operation within a method at the same level of specificity.
Those are the rules. Now, let’s look at some code that violates them. WordPress plugins are soft targets for finding poorly-designed code. Here’s one of many methods from the Getty Images plugin:
<?php
class Getty_Images {
function ajax_download() {
$this->ajax_check();
if( !current_user_can( self::capability ) ) {
$this->ajax_error( __( "User can not download images", 'getty-images' ) );
}
if( !isset( $_POST['url'] ) ) {
$this->ajax_error( __( "Missing image URL", 'getty-images' ) );
}
$url = sanitize_url( $_POST['url'] );
if( empty( $url ) ) {
$this->ajax_error( __( "Invalid image URL", 'getty-images' ) );
}
if( !isset( $_POST['meta'] ) ) {
$this->ajax_error( __( "Missing image meta", 'getty-images' ) );
}
$meta = $_POST['meta'];
if( !is_array( $_POST['meta'] ) || !isset( $_POST['meta']['ImageId'] ) ) {
$this->ajax_error( __( "Invalid image meta", 'getty-images' ) );
}
$tmp = download_url( $url );
if( is_wp_error( $tmp ) ) {
$this->ajax_error( __( "Failed to download image", 'getty-images' ) );
}
if( strpos( $url, 'http://delivery.gettyimages.com/' ) !== 0 ) {
$this->ajax_error( "Invalid URL" );
}
preg_match( '/[^?]+\.(jpe?g|jpe|gif|png)\b/i', $url, $matches );
if( empty( $matches ) ) {
$this->ajax_error( __( "Invalid filename", 'getty-images' ) );
}
$file_array['name'] = basename( $matches[0] );
$file_array['tmp_name'] = $tmp;
$attachment_id = media_handle_sideload( $file_array, 0 );
if( is_wp_error( $attachment_id ) ) {
$this->ajax_error( __( "Failed to sideload image", 'getty-images' ) );
}
$attachment = get_post( $attachment_id );
if( !$attachment ) {
$this->ajax_error( __( "Attachment not found", 'getty-images' ) );
}
$post_parent = isset( $_POST['post_id'] ) ? (int) $_POST['post_id'] : 0;
wp_update_post( array(
'ID' => $attachment->ID,
'post_content' => '',
'post_excerpt' => $attachment->post_content,
'post_parent' => $post_parent
) );
$getty_id = sanitize_text_field( $_POST['meta']['ImageId'] );
$existing_image_ids = get_posts( array(
'post_type' => 'attachment',
'post_status' => 'any',
'meta_key' => self::getty_details_meta_key,
'meta_value' => $getty_id,
'fields' => 'ids'
) );
foreach( $existing_image_ids as $existing_image_id ) {
wp_delete_post( $existing_image_id );
}
update_post_meta( $attachment->ID, self::getty_details_meta_key, array_map( 'sanitize_text_field', array_filter( $_POST['meta'], 'is_string' ) ) );
update_post_meta( $attachment->ID, self::getty_imageid_meta_key, sanitize_text_field( $_POST['meta']['ImageId'] ) );
$this->ajax_success( __( "Image downloaded", 'getty-images' ), wp_prepare_attachment_for_js( $attachment_id ) );
}
What rules does this code violate? For starters, it’s a long method. What responsibilities does it have? It validates the request, downloads an image, turns it into a wordpress attachment, deletes existing attachments of the same image, and updates the meta info for the attachment. It also deals with ajax requets. That’s obviously more than one responsibility.
The first line calls ajax_check(). Subsequent lines inspect individual pieces of the post request array. The code is not at a uniform level of abstraction.
There are multiple calls to ajax_error with ‘getty-images’ passed as one of the arguments each time. That duplication can be abstracted into a separate method.
This method is in a class with multiple other methods that do all sorts of things. So, it looks like it breaks all five of the rules of tactical design. Let’s start refactoring it.
Because the ajax_download method is already in a large class with other responsibilities, I’m going to extract a separate class to handle the download. Because I don’t want the new class to be concerned about ajax requests, I’m going to replace the ajax_error calls with exceptions. I’m also going to separate the user authorization and image downloading into separate methods. Now, ajax_download is at a uniform level of abstraction:
<?php
function ajax_download() {
$this->ajax_check();
$this->authorize_user();
$this->download_and_attach_image();
}
function authorize_user() {
if( !current_user_can( self::capability ) )
$this->ajax_error( __( "User can not download images", 'getty-images' ) );
}
function download_and_attach_image() {
try {
$attacher = new GettyImageAttacher();
$attachment = $attacher->process();
$this->ajax_success( __( "Image downloaded", 'getty-images' ), wp_prepare_attachment_for_js($attachment->ID));
} catch(GettyImageException $e) {
$this->ajax_error( __( $e->getMessage(), 'getty-images' ) );
}
}
Now, onto the new GettyImageAttacher class. I placed all of the code we removed from ajax_download into a process method that returns the attachment. I replaced the ajax_error calls with code that throws GettyImageExceptions (class not shown).
<?php
class GettyImageAttacher {
function process() {
if( !isset( $_POST['url'] ) ) {
throw new GettyImageException('Missing image URL');
}
$url = sanitize_url( $_POST['url'] );
if( empty( $url ) ) {
throw new GettyImageException('Missing image URL');
}
if( !isset( $_POST['meta'] ) ) {
throw new GettyImageException('Missing image meta');
}
$meta = $_POST['meta'];
if( !is_array( $_POST['meta'] ) || !isset( $_POST['meta']['ImageId'] ) ) {
throw new GettyImageException('Invalid image meta');
}
$tmp = download_url( $url );
if( is_wp_error( $tmp ) ) {
throw new GettyImageException('Failed to download image');
}
if( strpos( $url, 'http://delivery.gettyimages.com/' ) !== 0 ) {
throw new GettyImageException('Invalid URL');
}
preg_match( '/[^?]+\.(jpe?g|jpe|gif|png)\b/i', $url, $matches );
if( empty( $matches ) ) {
throw new GettyImageException('Invalid filename');
}
$file_array['name'] = basename( $matches[0] );
$file_array['tmp_name'] = $tmp;
$attachment_id = media_handle_sideload( $file_array, 0 );
if( is_wp_error( $attachment_id ) ) {
throw new GettyImageException('Failed to sideload image');
}
$attachment = get_post( $attachment_id );
if( !$attachment ) {
throw new GettyImageException('Attachment not found');
}
$post_parent = isset( $_POST['post_id'] ) ? (int) $_POST['post_id'] : 0;
wp_update_post( array(
'ID' => $attachment->ID,
'post_content' => '',
'post_excerpt' => $attachment->post_content,
'post_parent' => $post_parent
) );
$getty_id = sanitize_text_field( $_POST['meta']['ImageId'] );
$existing_image_ids = get_posts( array(
'post_type' => 'attachment',
'post_status' => 'any',
'meta_key' => Getty_Images::getty_details_meta_key,
'meta_value' => $getty_id,
'fields' => 'ids'
) );
foreach( $existing_image_ids as $existing_image_id ) {
wp_delete_post( $existing_image_id );
}
update_post_meta( $attachment->ID, Getty_Images::getty_details_meta_key, array_map( 'sanitize_text_field', array_filter( $_POST['meta'], 'is_string' ) ) );
update_post_meta( $attachment->ID, Getty_Images::getty_imageid_meta_key, sanitize_text_field( $_POST['meta']['ImageId'] ) );
return $attachment;
}
}
Now, GettyImageAttacher handles the image download process and knows nothing about ajax. We can start cleaning up the class by extracting separate methods for each resonsibility. We already identified the separate responsibilities when analyzing the original code. Let’s break them up.
<?php
class GettyImageAttacher {
function process() {
$this->parseRequest();
$this->downloadFile();
$this->createAttachment();
$this->deleteExistingAttachments();
$this->updateAttachmentMeta();
return $this->attachment;
}
}
With a uniform level of abstraction, the process method reads much nicer.
The parseRequest method checks the values of $_POST and stores them in instance variables for use by the other methods.
<?php
protected function parseRequest() {
$url = isset($_POST['url']) ? sanitize_url($_POST['url']) : '';
if(empty($url)) throw new GettyImageException('Missing image URL');
if(strpos($url, 'http://delivery.gettyimages.com/') !== 0)
throw new GettyImageException('Invalid URL');
if(!isset($_POST['meta']) || !is_array($_POST['meta']) || !isset($_POST['meta']['ImageId']))
throw new GettyImageException('Missing image meta');
if(!is_array($_POST['meta']) || !isset($_POST['meta']['ImageId']))
throw new GettyImageException('Invalid image meta');
preg_match('/[^?]+\.(jpe?g|jpe|gif|png)\b/i', $url, $matches);
if(empty($matches)) throw new GettyImageException('Invalid filename');
$this->url = $url;
$this->fileName = basename($matches[0]);
$this->gettyId = sanitize_text_field($_POST['meta']['ImageId']);
$this->parentId = isset($_POST['post_id']) ? (int) $_POST['post_id'] : 0;
$this->meta = $_POST['meta'];
}
The downloadFile method downloads the file and stores the temporary name in an instance variable.
<?php
protected function downloadFile() {
$tmp = download_url($url);
if(is_wp_error($tmp))
throw new GettyImageException('Failed to download image');
$this->tmpFileName = $tmp;
}
The createAttachment method creates a media attachment in WordPress and associates it with the parent post.
<?php
protected function createAttachment() {
$attachment_id = media_handle_sideload(
array('name' => $this->fileName, 'tmp_name' => $this->tmpFileName),
0
);
if(is_wp_error($attachment_id)) throw new GettyImageException('Failed to sideload image');
$attachment = get_post( $attachment_id );
if( !$attachment ) throw new GettyImageException('Attachment not found');
wp_update_post(array(
'ID' => $attachment->ID,
'post_content' => '',
'post_excerpt' => $attachment->post_content,
'post_parent' => $this->parentId
));
$this->attachment = $attachment;
}
The deleteExistingAttachments method finds and deletes attachments with the given Getty Image ID.
<?php
protected function deleteExistingAttachments() {
$existing_image_ids = get_posts(array(
'post_type' => 'attachment',
'post_status' => 'any',
'meta_key' => Getty_Images::getty_details_meta_key,
'meta_value' => $this->gettyId,
'fields' => 'ids'
));
foreach($existing_image_ids as $existing_image_id) {
wp_delete_post($existing_image_id);
}
}
Finally, the updateAttachmentMeta method stores the meta info and Getty Image ID for the new attachment.
<?php
protected function updateAttachmentMeta() {
update_post_meta(
$this->attachment->ID,
Getty_Images::getty_details_meta_key,
array_map('sanitize_text_field', array_filter($this->meta, 'is_string'))
);
update_post_meta(
$this->attachment->ID,
Getty_Images::getty_imageid_meta_key,
$this->gettyId
);
}
That’s a good start. Some of the methods in GettyImageAttacher are still a bit long. The class and its methods arguably have more than one responsibility and can be split up even further. However, we’re leaving the code better than we found it. A design is emerging from the changes we made.
Try applying these simple rules in your day-to-day programming work. It may be difficult to identify the changes at first, but eventually it will become more natural. Be careful to stay practical and not over-architect your code into something too complex.
You can watch Glenn’s talk here https://www.youtube.com/watch?v=LeZq6WoVzgY.
Need help cleaning up your code? Reach out to me on Twitter at @elegantbrew if you have any questions.