This is a follow-up to my post on why you should unit test Cocoa and iPhone applications. One reason I think Matt is against unit tests, at least given his particular examples, is that the tests themselves are quite large and confusing. They also use category smashing to override methods and inject mock objects and use runtime trickery to gain access to private instance variables.

These are all code smells that something isn’t right. Remember, TDD is about tests driving development; however you must remember to listen to the tests when they are crying out in pain. I’m going to take Matt’s Mac and iPhone projects with unit tests and re-work them so that they are much more manageable and cleaner.

Duplicated Code

This first smell I want to address is in the duplicated code between the Mac and iPhone applications. Both apps have a CLLoationManager delegate that listens for updates and formats the location into HTML for the web view, strings labels for the buttons, and a Google Maps URL. The code between these two apps is identical and is a good example of a violation of the Don’t Repeat Yourself, or DRY, Principle.

So how do we remove this duplicated code? Part of what makes this difficult is that it is highly coupled to the UI. In the Mac app, this code lives in an NSWindowController subclass, and in the iPhone app it lives in a UIViewController subclass. Thus we can’t just share an existing class as-is between projects because you can’t use an NSWindowController in an iPhone app and you can’t use a UIViewController in a Mac app. The answer is to extract this code into a new class that can be used from both applications.

Because the primary function of this new class is to take core location updates and format them to various strings, I’m calling the class MyCoreLocationFormatter. There may be a better name, since using the word formatter may imply an NSFormatter subclass, but let’s stick with it for now.

The sticking point is that we somehow need this new class to update the UI, irrespective of Cocoa vs. Cocoa Touch. We need to break the direct dependency on Cocoa and Cocoa Touch, and I’ve chosen to use a delegate with a single method as a bit of dependency inversion:

@protocol MyCoreLocationFormatterDelegate <NSObject>

- (void)locationFormatter:(MyCoreLocationFormatter *)formatter
 didUpdateFormattedString:(NSString *)formattedString
            locationLabel:(NSString *)locationLabel
           accuractyLabel:(NSString *)accuracyLabel;

@end

Thus, when the location changes, it formats the new location into appropriate strings and sends this message to its delegate. Both WhereIsMyMacWindowController and WhereIsMyPhoneViewController implement this protocol, and when they receive the message, the update their UI accordingly.

There are other ways to achieve similar decoupling. We could use an NSNotification to send out the update with the strings in the user info dictionary. Or we could have properties for the three strings and allow interested parties to monitor their updates using key-value observing. On the Mac side, this enables you to use Cocoa bindings to wire up the UI. Or we could return a dictionary with the three strings. Or we could create a new value object and return that. Any of these alternatives are viable, however I think using a delegate makes the coupling a bit more explicit and easier to follow. What’s important is that the direct dependency to the UI layer is broken.

Here’s the full interface of MyCoreLocationFormatter:

@interface MyCoreLocationFormatter : NSObject <CLLocationManagerDelegate>
{
    id<MyCoreLocationFormatterDelegate> _delegate;
    NSString * _formatString;
}

@property (nonatomic, assign, readwrite) id<MyCoreLocationFormatterDelegate> delegate;
@property (nonatomic, copy, readonly) NSString * formatString;

- (id)initWithDelegate:(id<MyCoreLocationFormatterDelegate>)delegate
          formatString:(NSString *)htmlFormatString;

- (NSURL *)googleMapsUrlForLocation:(CLLocation *)currentLocation;

- (void)locationManager:(CLLocationManager *)manager
    didUpdateToLocation:(CLLocation *)newLocation
           fromLocation:(CLLocation *)oldLocation;

- (void)locationManager:(CLLocationManager *)manager
       didFailWithError:(NSError *)error;

@end

Ignoring the CLLocationManager delegate methods, there are only two methods. The format string is used as the HTML template that gets loaded from the application’s bundle. The -googleMapsUrlForLocation: method is used to open the location up in a browser.

To test this class, we use a mock object to ensure the delegate is being called properly. We use instance variables and our fixture methods to setup an instance of MyCoreLocationFormatter and the mock delegate:

- (void)setUp
{
	// Setup
	_mockDelegate = [OCMockObject mockForProtocol:@protocol(MyCoreLocationFormatterDelegate)];
	_formatter = [[MyCoreLocationFormatter alloc] initWithDelegate:_mockDelegate
													formatString:@"ll=%f,%f spn=%f,%f"];
}

- (void)tearDown
{
	// Verify
	[_mockDelegate verify];
	
	// Teardown
	[_formatter release]; _formatter = nil;
}

With these fixtures in place, writing our tests are fairly straight forward:

- (void)testUpdateToNewLocationSendsUpdateToDelegate
{
    // Setup
    CLLocation * location = [self makeLocationWithLatitude:-37.80996889 longitude:144.96326388];
    [[_mockDelegate expect] locationFormatter:_formatter
                     didUpdateFormattedString:@"ll=-37.809969,144.963264 spn=-0.000018,-0.000014"
                                locationLabel:@"-37.809969, 144.963264"
                               accuractyLabel:[NSString stringWithFormat:@"%f", kCLLocationAccuracyBest]];
    
    // Execute
    [_formatter locationManager:nil didUpdateToLocation:location fromLocation:nil];
}

- (void)testUpdateToSameLocationDoesNotSendUpdateToDelegate
{
    // Setup
    CLLocation * location = [self makeLocationWithLatitude:-37.80996889 longitude:144.96326388];
    
    // Execute
    [_formatter locationManager:nil didUpdateToLocation:location fromLocation:location];
}

- (void)testFailedUpdateSendsUpdateToDelegate
{
    // Setup
    NSError * error = [self makeFakeErrorWithDescription:@"Some error description"];
    [[_mockDelegate expect] locationFormatter:_formatter
                     didUpdateFormattedString:@"Location manager failed with error: Some error description"
                                locationLabel:@""
                               accuractyLabel:@""];
    

    // Execute
    [_formatter locationManager:nil didFailWithError:error];
}

In order to keep the tests short and concise, I’ve moved the lengthy code to create a CLLocation and an NSError out of the tests and into helper methods. Remember, you’ve got to keep test code clean and maintainable, too. All three of these tests are only about 35 lines of code.

In contrast, Matt’s two original test methods were over 200 lines of code. The problem is that Matt’s code is testing the string formatting by asserting the web view’s HTML and text field’s strings. Separating the responsibility of formatting the strings from updating the UI not only improves the design, but makes testing much simpler.

So we’ve got on almost order of magnitude less code in our test methods that’s cleaner with the same code coverage. As a bonus, this class can be used in both the Mac and iPhone applications, so we can re-use the core logic. From an MVC perspective, the new MyCoreLocationFormatter class would be considered the model that is used with different views. Pushing logic out of the controller layer and into a model pays homage to the skinny controller, fat model design guideline.

Removing Testing Hacks

With this class complete, we can now move on to testing the WhereIsMyMacWindowController class. Matt’s test code uses a couple of hacks in order to enable testing that I consider code smells.

First, it uses runtime trickery, namely object_getInstanceVariable and object_setInstanceVariable, to gain access to the outlet instance variables. I think this is bad form and prefer to add public accessors for the outlets using properties:

@property (assign) IBOutlet WebView *webView;
@property (assign) IBOutlet NSTextField *locationLabel;
@property (assign) IBOutlet NSTextField *accuracyLabel;
@property (assign) IBOutlet NSButton *openInBrowserButton;

Using IBOutlet on a property means the NIB loading code will go through this public interface as well. Since these properties are already settable by the NIB loading code, I don’t see a big downside to making them public.

Second, the tests use category smashing to inject mock objects of CLLocationManager and NSWorkspace for testing. Again, I think this is bad form and prefer explicit dependency injection as a cleaner way to do this. I’m going to use constructor injection by adding these two initializers to WhereIsMyMacWindowController:

- (id)init;

// Designated Initializer
- (id)initWithLocationManager:(CLLocationManager *)locationManager
            locationFormatter:(MyCoreLocationFormatter *)locationFormatter
                    workspace:(NSWorkspace *)workspace;

The no-argument initializer calls the designated initializer with a new location manager, new location formatter, and the shared workspace. This allows production code to use the no-argument initializer and allows test code to use the designated initializer in order to inject test doubles, such as mock objects.

Some may find it odd to pass in an instance of NSWorkspace. I agree, it is a bit odd, but it’s necessary because it’s a singleton and hard to stub out for testing. There are other ways to achieve this, such as the category smashing Matt uses, or using a custom factory class that can be overridden by unit tests. However, eliminating the use of a singleton in the logic and using dependency injection is far more flexible. What if, for example, we wanted to run tests concurrently, like GHUnit allows? Now we’ve got to deal with thread safety issues when overriding the shared global instance. Remember, a singleton is a global in sheep’s clothing.

With these two hacks addressed, our tests in WhereIsMyMacWindowControllerTests become simpler. Again, I update the fixture to create mock objects for the various dependencies:

- (void)setUp
{
    // Setup
    _mockLocationManager = [OCMockObject mockForClass:[CLLocationManager class]];
    _mockLocationFormatter = [OCMockObject mockForClass:[MyCoreLocationFormatter class]];
    _mockWorkspace = [OCMockObject mockForClass:[NSWorkspace class]];
    _windowController = [[WhereIsMyMacWindowController alloc]
                         initWithLocationManager:_mockLocationManager
                         locationFormatter:_mockLocationFormatter
                         workspace:_mockWorkspace];
}

- (void)tearDown
{
    // Verify
    [_mockLocationManager verify];
    [_mockLocationFormatter verify];
    [_mockWorkspace verify];
    
    // Teardown
    [_windowController release]; _windowController = nil;
}

Here’s a few of the tests to show what a big difference these changes make:

- (void)testWindowDidLoadStartsLocationManager
{
    // Setup
    [[_mockLocationManager expect] setDelegate:_mockLocationFormatter];
    [[_mockLocationManager expect] startUpdatingLocation];

    // Execute
    [_windowController windowDidLoad];
}

- (void)testOpenInDefaultBrowserActionOpensGoogleMapsUrlInWorkspace
{
    // Setup
    [[[_mockLocationManager stub] andReturn:nil] location];
    NSURL * dummyUrl = [NSURL URLWithString:@"http://example.com/"];
    [[[_mockLocationFormatter stub] andReturn:dummyUrl] googleMapsUrlForLocation:nil];
    [[_mockWorkspace expect] openURL:dummyUrl];
    
    // Execute
    [_windowController openInDefaultBrowser:nil];
}

- (void)testCloseStopsLocationManager
{
    // Setup
    [[_mockLocationManager expect] stopUpdatingLocation];

    // Execute
    [_windowController close];
}

Note that I also moved the call to -stopUpdatingLocation from -dealloc to -close. I try to use -dealloc only for cleaning up memory related resources, which is especially important in a garbage collected environment. This also means we don’t have to stub out the call to -stopUpdatingLocation everywhere, making our tests simpler, too.

The test to ensure that the location formatter delegate updates the web view and test fields is still a bit lengthy:

- (void)testLocationFormatterDelegateUpdatesUI
{
    // Setup
    id mockWebView = [OCMockObject mockForClass:[WebView class]];
    id mockWebFrame = [OCMockObject mockForClass:[WebFrame class]];
    id mockLocationLabel = [OCMockObject mockForClass:[NSTextField class]];
    id mockAccuracyLabel = [OCMockObject mockForClass:[NSTextField class]];
    
    _windowController.webView = mockWebView;
    _windowController.locationLabel = mockLocationLabel;
    _windowController.accuracyLabel = mockAccuracyLabel;
    
    [[[mockWebView stub] andReturn:mockWebFrame] mainFrame];
    [[mockWebFrame expect] loadHTMLString:@"html string" baseURL:nil];
    [[mockLocationLabel expect] setStringValue:@"location"];
    [[mockAccuracyLabel expect] setStringValue:@"accuracy"];
    
    // Execute
    [_windowController locationFormatter:_mockLocationFormatter
                didUpdateFormattedString:@"html string"
                           locationLabel:@"location"
                           accuracyLabel:@"accuracy"];
    
    // Verify
    [mockWebFrame verify];
    [mockLocationLabel verify];
    [mockAccuracyLabel verify];
}

Personally, I’m not a big fan of these kinds tests, as well as tests that ensure outlets are setup properly after the NIB loads. If you stick with the skinny controller, fat model principle, the controllers become so simple they’re almost not worth testing. Just as you don’t test simple accessors because there’s little benefit to doing so, I don’t know that it’s necessarily worth it to get 100% code coverage on your controller classes. You still get a big benefit if the bulk of your code is in testable model classes, so this isn’t much of a loss.

There’s more changes I’ve made to the Mac application, and I’ve given the same treatment to the iPhone application, but I don’t want to make this post any longer than it already is. View the code I put up on Bitbucket, or download a tarball, to see the final result.