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.

Simplify the simple

Simple things should be simple. That’s easier said than done in a language like Objective-C. Sometimes things that are in fact simple look difficult just because of the syntax.

##Problem

I often prefer to override my getters to lazy load objects the moment they are actually required as opposed to setting up everything in init* methods. This also cleans up any init* methods by removing a chunk of boiler plate nothingness. This can get a bit hairy, consider (read: skim) the following example - the content here is really not important

- (NSCalendar *)calendar;
{
  if (!_calendar) {
    _calendar = [NSCalendar currentCalendar];
  }
  return _calendar;
}

- (NSTimeZone *)timeZone;
{
  if (!_timeZone) {
    _timeZone = [[NSTimeZone alloc] initWithName:@"GMT"];
  }
  return _timeZone;
}

- (NSDate *)minimumDate;
{
  if (!_minimumDate) {
    _minimumDate = [[NSDate alloc] initWithTimeIntervalSince1970:0];
  }
  return _minimumDate;
}

- (NSDate *)maximumDate;
{
  if (!_maximumDate) {
    _maximumDate = [[NSDate alloc] initWithTimeIntervalSince1970:MAXFLOAT];
  }
  return _maximumDate;
}

The above code looks like it is doing more than it actually is because of the if statements and all of the vertical space it consumes. This kind of code generally causes me to do a quick double take to skim the code to make sure it really is only lazy loading objects. When I have to do a double take it generally means I will make a mistake at some point later on so I prefer to avoid them if at all possible. It is also quite likely that a double take can knock me off my train of thought by taking unnecessary detours - I can only hold so much in my brain at once.

##Solution

I tend to hide this all at the bottom of a class below a #pragma mark - Properties. That’s enough to flip the bit in my head that says expect this kind of boiler plate code.

##Is that enough?

I don’t think it is, the code is still big bulky and annoying to look at. Granted I could not be so stubborn and transform the if statements into one liners but that would break my coding conventions and cause more double takes. I like to keep my if statements consistent and always use curly braces to avoid any pain later on. This also helps readability as things are always how I expect them to be. If something can be more succinctly written with ternary syntax then I will favour that - if not it’s multi-line, curly brace goodness for me.

##Experimentation

So the current solution I am experimenting with on these simple cases is to use the ternary syntax to make the simple getters as simple as possible. With this the above is transformed into the slightly more compact and easy to read

- (NSCalendar *)calendar;
{
  return _calendar = _calendar ?: [NSCalendar currentCalendar];
}

- (NSTimeZone *)timeZone;
{
  return _timeZone = _timeZone ?: [[NSTimeZone alloc] initWithName:@"GMT"];
}

- (NSDate *)minimumDate;
{
  return _minimumDate = _minimumDate ?: [[NSDate alloc] initWithTimeIntervalSince1970:0];
}

- (NSDate *)maximumDate;
{
  return _maximumDate = _maximumDate ?: [[NSDate alloc] initWithTimeIntervalSince1970:MAXFLOAT];
}

The syntax seems like a familiar old friend for a Ruby fan and is pretty concise without much compromise. I have even managed to put this class on a diet and trim 12 lines in just this simple example. (Here’s the Ruby example)

def minimum_date
  @minimum_date ||= NSDate.alloc.initWithTimeIntervalSince1970 0
end

This doesn’t help me in cases where I want to lazy load and configure an object at the same time, but my goal here is to make simple things simple. Returning a simple object is pretty simple but configuring one is not. For objects that need configuration I can go back to the warmth of those cosy curly braces, I also get the added advantage of being able to determine which methods return simple objects and which methods do a little more heavy lifting with just a glance.

// Object instantiated and needs some additional configuration
- (NSDateFormatter *)dateFormatter;
{
  if (!_dateFormatter) {
    _dateFormatter = [[NSDateFormatter alloc] init];
    _dateFormatter.dateFormat = @"yyyy-mm-DD";
  }
  return _dateFormatter;
}

// Simple object instantiated and returned
- (NSCalendar *)calendar;
{
  return _calendar = _calendar ?: [NSCalendar currentCalendar];
}

Here I can push the simple getter further down the class to emphasise even more it’s status in the class.

##Conclusion

I am still only really toying with this syntax but I currently like the additional semantic information I can get by just glancing at the code. I can quickly see which getters lazy load a simple default value and which getters lazy load objects and involve a little more setup work. I can imagine that this syntax may present an initial double take to anyone who was working on my code base and had never come across this before but it has a considerably lower barrier to entry than design patterns that are employed all of the time in large projects.

Experimentation with build numbering

Build numbering is not the most interesting subject but during my investigations I found some cool ideas to use in my own scheme.

##tl;dr

MacRuby is cool and I over engineered a solution for fun.

#Starting point

I started out with some StackOverflow questions 1, 2 and then ended up with this blog post over at cimgf.com. I’ve not used pearl and as stated in some of the comments it’s got some hairy regex’s in there which ideally I’d rather not have to deal with.

I’m a Ruby fan and as such that would be my preferred language to use for this. Scrolling down to the comments on that blog there are examples in python and ruby so there’s a good starting off point.

##Issue 1

The example in the blog uses the commit hash as the build number.

In accordance with Apple’s Information Property List Key Reference the commit hash can not be used as the build number because this this property should be:

… a monotonically increased string, comprised of one or more period-separated integers

A couple of examples on StackOverflow and in comments of the original blog post suggest using

git rev-list HEAD | wc -l

This first command git rev-list HEAD prints the hash of each commit on a new line

paul$ git rev-list HEAD
ced64f89f841d46b2a4262a2987ca363be704d49
ff538caa32f704fb09295d41a20fe2f8488ca6ae
72847eb10e3c3731a696c84e3ae4179640d28ed2

This is then piped into wc -l which counts the number of lines given to it, in this case from standard input. Here the result would be 3.

I quite like this idea for numbering as it works a lot better when the code base is shared among a team. If you was simply incrementing a counter you have to deal with issues such as where is it stored and how do you keep it consistent. It should also serve as a rough guide to how many stable builds it took to get the app to it’s current state.

##Issue 2

I quite like the thought of having the commit hash

I’d like to keep the commit hash details so that I can easily track down what exact commit this came from. A quick scan of the Apple’s Information Property List Key Reference confirms that you are allowed to add any keys/values to the info plist.

##Additional requirements

• I also want to update the settings bundle
• I don’t want any regex’s
• I want to play with doing this fairly natively

#Let’s build

So I started out by wrapping up those build paths that come from the ENV variable

##Helpers

class Environment
  %w(BUILT_PRODUCTS_DIR INFOPLIST_PATH WRAPPER_NAME).each { |const| const_set(const, const) }
  
  def initialize env
    @env = env
  end
  
  def build_info_plist_path
    "#{@env[BUILT_PRODUCTS_DIR]}/#{@env[INFOPLIST_PATH]}"
  end
  
  def build_settings_plist_path
    "#{@env[BUILT_PRODUCTS_DIR]}/#{@env[WRAPPER_NAME]}/Settings.bundle/Root.plist"
  end
end

I only need two paths but they need to be constructed and I would rather not have this construction logic scattered about. I’m also preferring to use constants for keys as opposed to strings so I’m dynamically generating them first.

This next bit may look especially odd for people who don’t know Ruby, but do know Objective-C

class NSMutableDictionary
  def self.plist_open path
    plist = self.alloc.initWithContentsOfFile path
    if block_given?
      yield plist
      plist.writeToFile path, atomically: true
    end
    plist
  end
end

The cat is out of the bag - I’m playing around with MacRuby. I can get the native handling of plists from Objective-C but the ease of Ruby. Here I am opening up the NSMutableDictionary class and adding my own helper method to open a file, allow me to make changes and then save them back out. I added this as a convenience for later on.

##Actual logic

The constructor for the main class looks like this

class XcodeVersioner  
  %w(DefaultValue PreferenceSpecifiers CFBundleShortVersionString CFBundleVersion PSGitCommit Key).each { |const| const_set(const, const) }
  
  attr_reader :bundle_version, :build_number, :git_commit, :env
  
  def initialize env
    @env            = env
    @build_number   = `git rev-list HEAD | wc -l`.to_s.strip
    @git_commit     = `git rev-parse HEAD`.to_s.strip
    @bundle_version = ''
  end

...
  

I start out by declaring constants again to avoid magic strings being littered everywhere. Then I add the attr_reader’s for ivars. The Environment helper class is passed into the constructor and then the basic information is retrieved from git.

Next up is the method that gets called to start the real work

def run
  version_info_plist
  version_settings
  puts "Bundle Version => #{bundle_version} (#{build_number}) with commit #{git_commit}"
end

In this method we just call two more methods where the nitty gritty details are hidden and then print out the result (this will show up in the build logs).

If you’ve not used blocks much in Objective-C then you really should. You can make some really cool API’s, of course this is Ruby but they work pretty much the same. Using the helper method I added to NSMutableDictionary I get something like this for editing the info plist

def version_info_plist
  NSMutableDictionary.plist_open(env.build_info_plist_path) do |plist|
    @bundle_version        = plist[CFBundleShortVersionString]
    plist[CFBundleVersion] = build_number
    plist[PSGitCommit]    = git_commit
  end
end

Let’s break this one down line by line

1. Method defintion
2. Call the helper method with a path to the file. The NSMutableDictionary opens the file and then yield’s it back into the block as the variable plist
3. I grab the bundle version from the info plist file and store it into an ivar
4. I set the CFBundleVersion version to the build_number retrieved in the constructor
5. I set the PSGitCommit to the git_commit retrieved in the constructor
6. The end of the block
7. The end of the method

The code between the do and matching end are the block of code that gets yield‘d to by the plist_open method.

The method for working with the settings bundle is very similar in concept, but it’s slightly more awkward to work with due to the structure of the plist.

The structure of the settings bundle plist looks like this

{
  "PreferenceSpecifiers" => [
    {"Type"=>"PSGroupSpecifier", "FooterText"=>"Some Footer Text", "Title"=>"Info"}, 
    {"DefaultValue"=>"", "Key"=>"CFBundleShortVersionString", "Type"=>"PSTitleValueSpecifier", "Title"=>"Version"},
    {"DefaultValue"=>"", "Key"=>"PSGitCommit", "Type"=>"PSTitleValueSpecifier", "Title"=>"Debug"},
  ], 
  "StringsTable"=>"Root"
}

The bit’s we care about are the dictionaries that are held within an array. The array is contained in a dictionary under the key PreferenceSpecifiers.

What this means for me is that I have to loop through each item in the array and then use a case statement to figure out if it is a preference specifier that I want to edit. This all ends up looking like:

def version_settings
  NSMutableDictionary.plist_open(env.build_settings_plist_path) do |plist|
    plist[PreferenceSpecifiers].each do |preference_specifier| 
      case preference_specifier[Key]
      when CFBundleShortVersionString
        preference_specifier[DefaultValue] = "#{bundle_version} (#{build_number})"
      when PSGitCommit
        preference_specifier[DefaultValue] = git_commit[0...10]
      end
    end
  end
end

##Conclusion

This probably looks completely over engineered when compared to the original blog post’s code. I would have to agree with that statement, but I do believe that I’ve had some fun experimentation with MacRuby, made a useful tool for myself and have abstracted the logic out nicely for easier maintenance. Before I did this I never thought of the power at my fingertips with the Cocoa framework and Ruby combined - this example is only taking advantage of NSMutableDictionary’s ability to natively handle the plist format but I am sure better examples could be found.