Polishing turds

There’s the old adage “You can’t polish a turd”, which just sounds like a challenge to me (this was of course proved wrong by the Mythbusters).

If I see some particularly unsightly code I’ll either fix it there and then or if it’s a tricky one I’ll make a note to stew on it and come back for a fix.

The aim of this post is not to point and make fun of some code but to examine what kind of steps can be taken to polish a fresh small dung, before it’s start to rot and make a real mess in your code base.

#Starting point

Here is todays example from a real project, the participants will not be named to save their blushes (technically I have done work on this project but Git blame has gotten me off the hook for this snippet).

UITextField *textField = (UITextField *)[self.scrollView viewWithTag:kATag]; textField.text = @"A"; textField.backgroundColor = kWhiteColor;
textField = (UITextField *)[self.scrollView viewWithTag:kBTag]; textField.text = @"B"; textField.backgroundColor = kWhiteColor;
textField = (UITextField *)[self.scrollView viewWithTag:kCTag]; textField.text = @"C"; textField.backgroundColor = kWhiteColor;
textField = (UITextField *)[self.scrollView viewWithTag:kDTag]; textField.text = @"D"; textField.backgroundColor = kWhiteColor;
textField = (UITextField *)[self.scrollView viewWithTag:kETag]; textField.text = @"E"; textField.backgroundColor = kWhiteColor;

The code essentially fills out some text boxes and then changes their background colour to indicate that they are filled out. Currently there are multiple statements per line and there is a lot of repetition.

There are many angles I can come at this dung with my polishing cloth but here is how I actually did tackle it.

#Step 1

The first change I would like to make is to lie to the compiler (just a little bit). The method -[UIView viewWithTag:] has a return type of UIView *, which means to stop the compiler moaning in this case the return value has to be cast to a UITextField *. By casting the value I am essentially saying

“Hey compiler I know you are really clever and all but on this occasion I know better and I’m telling you this is going to be a UITextField *”

(it’s perfectly normal to have a conversation with the compiler whilst you code right?).

So I start to think - seems as I am already stepping in and telling the compiler that I know better, why not go the whole way and save some pixels by using id instead of UITextField *. That saves me some space and asterisks (which tend to draw my attention for no reason) and looks like this

UITextField *textField = (id)[self.scrollView viewWithTag:kATag]; textField.text = @"A"; textField.backgroundColor = kWhiteColor;
textField = (id)[self.scrollView viewWithTag:kBTag]; textField.text = @"B"; textField.backgroundColor = kWhiteColor;
textField = (id)[self.scrollView viewWithTag:kCTag]; textField.text = @"C"; textField.backgroundColor = kWhiteColor;
textField = (id)[self.scrollView viewWithTag:kDTag]; textField.text = @"D"; textField.backgroundColor = kWhiteColor;
textField = (id)[self.scrollView viewWithTag:kETag]; textField.text = @"E"; textField.backgroundColor = kWhiteColor;

#Step 2

The next thing I want to get rid of is the kWhiteColor. It’s name is too specific and I don’t like the use of #define to make it. This colour is also used throughout the project so I want it to be globally accessible and easy to change in one place. For this I’ll use a category.

I make sure to prefix the method to avoid collisions and in the process I have given the method a more appropriate name for what the colour actually represents in the problem domain as opposed to it’s actual colour.

UIColor+AppStyling.h

@interface UIColor (AppStyling)

+ (UIColor *)ps_completedFieldBackgroundColor;

@end

UIColor+AppStyling.m

#import "UIColor+AppStyling.h"

@implementation UIColor (AppStyling)

+ (UIColor *)ps_completedFieldBackgroundColor;
{
  return [UIColor whiteColor];
}

@end

I now need to import this anywhere it is needed - I’ll just make it available throughout my project by importing it into my pre compiled header.

Prefix.pch

...

#ifdef __OBJC__
    #import <Foundation/Foundation.h>
    #import <UIKit/UIKit.h>
    #import "UIColor+AppStyling.h"
#endif

...

As a result of this change the code is now slightly longer again as the old kWhiteColor is much shorted than the new [UIColor ps_completedFieldBackgroundColor]. I can live with this for now as I’ve gotten rid of the #define, given the colour a more appropriate domain specific name and made it into a normal method. This is how we stand now:

UITextField *textField = (id)[self.scrollView viewWithTag:kATag]; textField.text = @"A"; textField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];
textField = (id)[self.scrollView viewWithTag:kBTag]; textField.text = @"B"; textField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];
textField = (id)[self.scrollView viewWithTag:kCTag]; textField.text = @"C"; textField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];
textField = (id)[self.scrollView viewWithTag:kDTag]; textField.text = @"D"; textField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];
textField = (id)[self.scrollView viewWithTag:kETag]; textField.text = @"E"; textField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];

#Step 3

The next thing I want to tackle is the tags. I’m not fond of tags especially when you have to maintain them in two places (code and xibs - I’d quite like to see some imaginary construct like IBConstant which makes a constant available in IB, but that still wouldn’t help the problem much).

These views are in fact created in a xib so it’s a simple case of making some outlets and hooking them up.

@property (nonatomic, weak) IBOutlet UITextField *aTextField;
@property (nonatomic, weak) IBOutlet UITextField *bTextField;
@property (nonatomic, weak) IBOutlet UITextField *cTextField;
@property (nonatomic, weak) IBOutlet UITextField *dTextField;
@property (nonatomic, weak) IBOutlet UITextField *eTextField;

And with that the code can now be simplified to something like this

self.aTextField.text = @"A"; self.aTextField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];
self.bTextField.text = @"B"; self.bTextField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];
self.cTextField.text = @"C"; self.cTextField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];
self.dTextField.text = @"D"; self.dTextField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];
self.eTextField.text = @"E"; self.eTextField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];

Notes

• The textFields are not actually named like this in the real project this is for demonstration purposes.
• I only take a weak pointer as the containing view owns the textFields not my class.
• Hindsight is always 20/20 and I could have skipped step 1 and come straight here but it’s better to take baby steps than to try and get clever.

#Step 4

There is a lot of repeated work with the setting of the textFields background colour, so I’ll DRY this up next using the nice new literal array syntax

self.aTextField.text = @"A"; 
self.bTextField.text = @"B"; 
self.cTextField.text = @"C"; 
self.dTextField.text = @"D"; 
self.eTextField.text = @"E"; 

for (UITextField *textField in @[ self.aTextField, self.bTextField, self.cTextField, self.dTextField, self.eTextField]) {
  textField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];
}

I’m liking the way the code is taking shape and looking less like a spreadsheet of repeated rows (cough cough ignore lines 1 .. 5).

#Step 5

This is still not as DRY as I would like as the textField ivars are mentioned twice. To solve this I can take a slightly different tactic and make an intermediary mapping. Annoyingly UIView does not conform to NSCopying so it can not be used as a key in a dictionary but I can make do with the slightly backward mapping of using the word first. The final result is something like:

NSDictionary *textToFieldMappings = @{
  @"A" : self.aTextField,
  @"B" : self.bTextField,
  @"C" : self.cTextField,
  @"D" : self.dTextField,
  @"E" : self.eTextField,
};

[textToFieldMappings enumerateKeysAndObjectsUsingBlock:^(NSString *text, UITextField *textField, BOOL *stop) {
  textField.text            = text;
  textField.backgroundColor = [UIColor ps_completedFieldBackgroundColor];
}];

#Conclusion

By going step by step I’ve managed to (IMHO) clean up this snippet. I have taken a dense block off code that reused the same local variable over and over to perform multiple tasks per line and elegantly split it up into a mapping step followed by an action step. I’ve used a category so that I can use language appropriate to the problem in a normal way and I’ve made the code much DRY’r and hopefully easier to maintain.

This code takes up more vertical space than the original (it wouldn’t if the original was broken down to one statement per line) and has some new files for the category, but it’s much more self descriptive and does not repeat it’s self at any point.

I can now put my polishing cloth away and keep my eyes peeled for the next turd.