This is a continuation of the blog post 12 Principles of Good Software Design.
1. An idiom should make your code simpler
It is a shame that this first principle even has to be said, since it is so self-evidently true. But all too often we see people employ design patterns or architectural redesigns in the name of clarity or out of a desire for better code organization which results in making the code more complex. The theory is that eventually we’ll need the features so adding complexity now will help us in the future.
But notice what happens: the future never comes, the code is handed over to someone who is confused by the new design pattern, and they add code rather than fix what is there, because it’s better to leave something that mostly works alone than reorganize what you don’t understand.
Besides, there are only so many hours in the day, and it’s Friday, and we’ve got tickets to the latest Star Trek movie…
There is a flip side of this principle that can also be violated. Quoting Albert Einstein: “Make everything as simple as possible, but not simpler.”
Sometimes people attempt to make the code simpler than it can be made. And in the process they wind up with extra implementation details that they then “hide” using tricks or obscure language features which make it impossible to know what the software is doing.
Ironically we can see both of these (hiding features and added complexity) in the following example.
For the following example, written in Objective C for iOS, we have a single view controller. The view controller shows a balance that is stored on a remote server, and the interface to the server can be used to both obtain the current balance and update the balance by subtracting from it. Our balance cannot go below zero.
Our interface looks like:
@interface ServerModel : NSObject + (ServerModel *)shared; - (void)obtainBalance:(void (^)(NSInteger bal))callback; - (void)updateBalance:(NSInteger)update callback:(void (^)(NSInteger bal))callback; @end
The callback method provided is called asynchronously; after all, the value has to be pulled from a server. To keep things simple for this example, we do not pass an error to the user. (In real life, of course, you would pass the error and handle network issues appropriately.)
Now when our view controller contains two controls: a label which is used to show the balance, and a text field which is used to enter a change in the balance. The return key is used to update our balance.
Our view controller looks like the following:
#import "ViewController.h"
#import "ServerModel.h"
@interface ViewController () <UITextFieldDelegate>
@property (weak, nonatomic) IBOutlet UILabel *balance;
@property (weak, nonatomic) IBOutlet UITextField *change;
@property (assign, nonatomic) double curBalance;
@end
@implementation ViewController
- (void)viewDidLoad
{
[super viewDidLoad];
/*
* Load the contents from our server
*/
self.balance.text = @"";
self.change.text = @"";
[[ServerModel shared] obtainBalance:^(NSInteger bal) {
// Received balance; update contents
double val = bal/100.0;
self.curBalance = val;
self.balance.text = [NSString stringWithFormat:@"%.2f",val];
}];
}
- (BOOL)textFieldShouldReturn:(UITextField *)textField
{
double change = self.change.text.doubleValue;
/*
* Check to make sure our balance has been properly adjusted
*/
if (self.curBalance - change < 0) {
/*
* This would draw to zero. Alert the user rather than make the
* round trip to the server
*/
UIAlertController *c = [UIAlertController alertControllerWithTitle:@"Illegal value" message:@"You may not reset the balance below zero" preferredStyle:UIAlertControllerStyleAlert];
UIAlertAction *action = [UIAlertAction actionWithTitle:@"Cancel" style:UIAlertActionStyleCancel handler:nil];
[c addAction:action];
[self presentViewController:c animated:YES completion:nil];
return NO;
}
self.change.text = @""; // Clear the value we entered.
/*
* Sanity check against balance.
*/
[[ServerModel shared] updateBalance:(int)(change * 100 + 0.5) callback:^(NSInteger bal) {
double val = bal/100.0;
self.balance.text = [NSString stringWithFormat:@"%.2f",val];
}];
return NO;
}
@end
When the view loads, the balance is retrieved; some time later, we then display the balance on our display. When the user enters a value, we check to make sure the balance has not gone below zero; if it does, we warn the user. Otherwise, we update the balance and (when the updated balance returns), we update the displayed balance.
Of course there are some missing features. Our balance may be updated from a number of sources, and periodically we may want to ping the server to see if our balance changed. Our protocol may even notify us if the balance has changed unilaterally. We may want to display a “wait” spinner as we are loading the contents. We of course need to handle network errors.
Our example is 66 lines long, consisting of two methods, though one of the methods is rather long as it handles the business logic of validating the inputs locally.
Now let’s suppose we wish to refactor our code to use MVVM.
There are two motivations for MVVM. First, it presumes that the model in our applications are simplistic, and we need to push the business logic in our view controller (the actions of pulling and pushing data from the server, validating our inputs) to a separate chunk of code from our “view” (the view controller), which is solely responsible for managing the display.
The original motivation for MVVM comes from the C# world, where tools exist to help construct a view controller and tie fields of that view controller to a model, and where tools exist to build models as collections of data objects. In that world, when the tools are building our view controller and other tools are building our model, naturally the business logic needs to go somewhere else, right?
The additional motivations for MVVM in the iOS world is to help isolate the business logic in a View Controller to something that can be run in a unit test without involving the user interface itself.
So let’s do that.
First, we need a view model which handles the logic of obtaining and saving our data, as well as the business logic of validating our balance. Because this object ideally has no user interface logic, we need to alert the view model when there is an error.
Our final ViewModel object has the following declaration:
#import <Foundation/Foundation.h> // Possible updateBalance return codes #define UPDATEERROR_OK 0 #define UPDATEERROR_ILLEGAL 1 @interface ViewModel : NSObject @property (nonatomic, assign) double balance; - (void)reload; // Reload balance - (NSInteger)updateBalance:(double)update; @end
Note we expose the reload method, though reload is implicitly called when our object is first constructed. We also provide an updateBalance method which returns an error code; these errors are only errors detected during pre-flight; server errors would need to be handled separately.
Our ViewModel code looks like:
#import "ViewModel.h"
#import "ServerModel.h"
@implementation ViewModel
- (id)init
{
if (nil != (self = [super init])) {
[self reload];
}
return self;
}
- (void)reload
{
[[ServerModel shared] obtainBalance:^(NSInteger bal) {
self.balance = bal / 100.0;
}];
}
- (NSInteger)updateBalance:(double)update
{
/* Preflight errors */
if (self.balance - update < 0) {
return UPDATEERROR_ILLEGAL;
}
/* Update our balance */
[[ServerModel shared] updateBalance:(NSInteger)(update*100 + 0.5) callback:^(NSInteger bal) {
self.balance = bal / 100.0;
}];
return UPDATEERROR_OK;
}
@end
This seems relatively straight forward. Our ViewModel object communicates with our server to maintain the balance internally.
And if we refactor our original ViewController to use our ViewModel class, notice that things appear simpler.
#import "ViewController.h"
#import "ViewModel.h"
@interface ViewController () <UITextFieldDelegate>
@property (weak, nonatomic) IBOutlet UILabel *balance;
@property (weak, nonatomic) IBOutlet UITextField *change;
@property (strong, nonatomic) ViewModel *viewModel;
@end
@implementation ViewController
- (void)viewDidLoad
{
[super viewDidLoad];
/*
* Load the contents from our server
*/
self.balance.text = @"";
self.change.text = @"";
self.viewModel = [[ViewModel alloc] init];
}
- (BOOL)textFieldShouldReturn:(UITextField *)textField
{
double change = self.change.text.doubleValue;
/*
* Check to make sure our balance has been properly adjusted
*/
NSInteger err = [self.viewModel updateBalance:change];
if (err == UPDATEERROR_ILLEGAL) {
/*
* This would draw to zero. Alert the user rather than make the
* round trip to the server
*/
UIAlertController *c = [UIAlertController alertControllerWithTitle:@"Illegal value" message:@"You may not reset the balance below zero" preferredStyle:UIAlertControllerStyleAlert];
UIAlertAction *action = [UIAlertAction actionWithTitle:@"Cancel" style:UIAlertActionStyleCancel handler:nil];
[c addAction:action];
[self presentViewController:c animated:YES completion:nil];
} else if (err == UPDATEERROR_OK) {
self.change.text = @"";
}
return NO;
}
@end
However, we have a problem, and that is the balance is not reflected in the view controller’s balance label.
For this, we use KVO to observe changes in the ViewModel’s balance field. (In the Microsoft .Net environment, this binding is handled for you by a collection of built-in tools.)
There are plenty of KVO libraries out there (such as ReactiveCocoa) which provide a variety of classes and methods which aid in this, but let’s go ahead–as this is such as simple example–and handle it ourselves.
In order to simplify the binding operation, we create a class called “Binder”. Our class is simple: it handles all the bindings and issues a call to a block when the value our binder is bound to changes. The binder releases all of the observers on the object when it is disposed of, so we only need to create the binder in our ViewController, and bind against the values whose values we are interested in monitoring.
Our binder class has the following declaration:
#import <Foundation/Foundation.h> @interface Binder : NSObject - (void)bindKeyPath:(NSString *)path ofObject:(NSObject *)object withCallback:(void (^)(void))callback; @end
And the declaration of our binder class is:
#import "Binder.h"
#import "BinderKey.h"
// Simple binder utility aids in KVO observation
@interface Binder ()
@property (strong, nonatomic) NSMutableDictionary *map;
@end
@implementation Binder
- (id)init
{
if (nil != (self = [super init])) {
self.map = [[NSMutableDictionary alloc] init];
}
return self;
}
- (void)dealloc
{
NSArray *a = [self.map allKeys];
for (BinderKey *b in a) {
[b.object removeObserver:self forKeyPath:b.keyPath];
}
}
- (void)bindKeyPath:(NSString *)path ofObject:(NSObject *)object withCallback:(void (^)(void))callback;
{
BinderKey *k = [[BinderKey alloc] initWithObject:object keyPath:path];
void (^copyCallback)(void) = [callback copy];
self.map[k] = copyCallback;
[object addObserver:self forKeyPath:path options:NSKeyValueObservingOptionNew context:nil];
}
- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context
{
BinderKey *k = [[BinderKey alloc] initWithObject:object keyPath:keyPath];
void (^callback)(void) = self.map[k];
if (callback) callback();
}
@end
Note our class uses a dictionary to rapidly look up a particular callback block given an object and the keypath being observed. This uses a custom dictionary key, whose declaration is:
#import <Foundation/Foundation.h> // A key used internally for my NSDictionary by my binder. This tracks // a specific object (by pointer) and a specific key @interface BinderKey : NSObject <NSCopying> - (id)initWithObject:(NSObject *)obj keyPath:(NSString *)key; - (NSObject *)object; - (NSString *)keyPath; @end
And the body is:
#import "BinderKey.h"
@interface BinderKey()
@property (weak, nonatomic) NSObject *object;
@property (copy, nonatomic) NSString *keyPath;
@end
@implementation BinderKey
- (id)initWithObject:(NSObject *)obj keyPath:(NSString *)key;
{
if (nil != (self = [super init])) {
self.object = obj;
self.keyPath = key;
}
return self;
}
- (id)copyWithZone:(NSZone *)zone
{
// We are a constant object so we just return ourselves.
return self;
}
- (NSUInteger)hash
{
NSUInteger ptr = (NSUInteger)(self.object); // object pointer equality
return [self.keyPath hash] ^ ptr; // silly hash code
}
- (BOOL)isEqual:(id)object
{
if (object == nil) return NO;
if (![object isKindOfClass:[BinderKey class]]) return NO;
BinderKey *b = (BinderKey *)object;
return ((b.object == self.object) && [b.keyPath isEqual:self.keyPath]);
}
@end
Now that we have our binder, we can then bind our ViewModel to our view label in our ViewController:
- (void)viewDidLoad
{
[super viewDidLoad];
/*
* Load the contents from our server
*/
self.balance.text = @"";
self.change.text = @"";
self.viewModel = [[ViewModel alloc] init];
self.binder = [[Binder alloc] init];
[self.binder bindKeyPath:@"balance" ofObject:self.viewModel withCallback:^{
double val = self.viewModel.balance;
self.balance.text = [NSString stringWithFormat:@"%.2f",val];
}];
}
Our new overall ViewController then becomes:
#import "ViewController.h"
#import "ViewModel.h"
#import "Binder.h"
@interface ViewController () <UITextFieldDelegate>
@property (weak, nonatomic) IBOutlet UILabel *balance;
@property (weak, nonatomic) IBOutlet UITextField *change;
@property (strong, nonatomic) ViewModel *viewModel;
@property (strong, nonatomic) Binder *binder;
@end
@implementation ViewController
- (void)viewDidLoad
{
[super viewDidLoad];
/*
* Load the contents from our server
*/
self.balance.text = @"";
self.change.text = @"";
self.viewModel = [[ViewModel alloc] init];
self.binder = [[Binder alloc] init];
[self.binder bindKeyPath:@"balance" ofObject:self.viewModel withCallback:^{
double val = self.viewModel.balance;
self.balance.text = [NSString stringWithFormat:@"%.2f",val];
}];
}
- (BOOL)textFieldShouldReturn:(UITextField *)textField
{
double change = self.change.text.doubleValue;
/*
* Check to make sure our balance has been properly adjusted
*/
NSInteger err = [self.viewModel updateBalance:change];
if (err == UPDATEERROR_ILLEGAL) {
/*
* This would draw to zero. Alert the user rather than make the
* round trip to the server
*/
UIAlertController *c = [UIAlertController alertControllerWithTitle:@"Illegal value" message:@"You may not reset the balance below zero" preferredStyle:UIAlertControllerStyleAlert];
UIAlertAction *action = [UIAlertAction actionWithTitle:@"Cancel" style:UIAlertActionStyleCancel handler:nil];
[c addAction:action];
[self presentViewController:c animated:YES completion:nil];
} else if (err == UPDATEERROR_OK) {
self.change.text = @"";
}
return NO;
}
@end
And if we compile all of this, it works correctly.
Question: is this simpler?
Empirically our ViewController is doing nearly as much work. The only two things we have removed from the ViewController is one instance of loading the balance label from the -textFieldShouldReturn: message, and the business logic of validating prior to a network call of making sure our balance does not go below zero.
We’ve also added a new Binder object which is handling binding fields with values in our view model–a binder object which takes a text string to describe the field within our view model. (This is inherently fragile; if we were to change the name of the field in our ViewModel from ‘balance’ to something else, we would have no indications at compile time that something went wrong.)
We did push the logic for validating the inputs for -updateBalance: to the ViewModel, thus removing business logic from our ViewController. But note that we could have pushed the same logic into the ServerModel class by doing the following:
First, alter the signature of ServerModel so that -updateBalance:callback: returns a preflight error:
#import <Foundation/Foundation.h> // Possible updateBalance return codes #define PREFLIGHTERROR_OK 0 #define PREFLIGHTERROR_ILLEGAL 1 @interface ServerModel : NSObject + (ServerModel *)shared; - (void)obtainBalance:(void (^)(NSInteger bal))callback; - (NSInteger)updateBalance:(NSInteger)update callback:(void (^)(NSInteger bal))callback; @end
Second, we change our server model class by adding the preflight checking to the server call:
- (NSInteger)updateBalance:(NSInteger)update callback:(void (^)(NSInteger bal))callback
{
// Check against our cached value
if (self.balance - update < 0) {
return PREFLIGHTERROR_ILLEGAL;
}
// Server magic goes here
return PREFLIGHTERROR_OK;
}
After all, the original papers describing the Model-View-Controller paradigm suggested the Model was not just simple database access code or simple network access code, but would also contain all of the business logic and data integrity checking logic as well. (That way, a second screen which needs to update the balance doesn’t need to duplicate business logic, as MVVM would have us do, and as our (admittedly) poorly written original ViewController class was doing.)
And note that our duplicate code that loads the balance label in the original example could have refactored that into a single method. No point in writing the same code twice, right?
So question: is our MVVM example simpler?
Well, we had to create two new classes for binding, but presumably they could be reused throughout our code. However, the binding code separates the functionality of refreshing the contents of our label from the process of asking for our label to be refreshed. This creates a sort of “spaghetti code” flow, where the result of an action is located in a completely different place from the logic which requested the action.
Our ViewController was not really made any simpler. (Sure, the business logic was located into a separate ViewModel, but given the MVVM model presumes one ViewModel per ViewController, we still have the problem that business logic can be shotgunned throughout our code.)
Our ViewModel certainly separated the concerns of data management for a specific view from the concern of managing the user interface–but then, as I argue above, that logic would better belong in a model which (as originally conceived in the 1980’s) contains all the integrity checks and doesn’t just contain data.
And by the simple metric of line count, we expanded one class which contained 66 lines of code to 96 total lines of code, expanding our source kit by 45%, as well as hiding the logic of tying in our view to a separate binder class. (And that 45% growth does not count our binding support classes.)
I do not intend to rag on MVVM. Every tool in the toolkit is useful, and it’s important to know about those tools, know how to use them, and know their limitations.
And you really should know MVVM, because it has become all the rage. Software architects may demand that you maintain MVVM code or convert code to MVVM, and knowing how to do that is important.
But in the above case, I have to say the answer to our question “is it simpler”, in all honestly, is no:
- We increased the line count.
- We altered the flow of code so that the response to an action (updating the balance label) is divorced from the process that requests the action.
- We have not solved the problem of business logic shot-gunned through our code. All we’ve done is moved business logic into a ViewModel. Note MVVM suggests a one-to-one association between the View (ViewController) and the ViewModel classes. This means by definition business logic in a ViewModel class will necessarily be duplicated across multiple screens rather than placed into a single location.
- And we haven’t even really solved the problem of unit tests completely, as one major source of problems (the constant string used to specify the field name in ViewModel) is in code (the ViewController) which would not be tested by hooking a unit test to our ViewModel.
I know I may be attacking a sacred cow here. After all, there are plenty of people who are convinced that MVVM is “worth it” even if it makes code more complicated.
And again, I’m not attacking MVVM; it’s just an easy example. There are plenty of other examples involving “code refactoring” which attempts to “generalize” a design pattern without considering other possible ways of approaching the problem. For example, see my essay on factorial code written in Java.
But do we need to make things more complicated in the name of an architectural ideal when, in a specific case, it is not called for?
Isn’t the world already full of too much complicated stuff to add to it ourselves?
Development Chaos Theory 