Rewrite code from Objective-C to conform with Swift power tools and concise style

Question

I started a project with Objective-C and rewrote it with Swift. The project contains two UITableViewControllers: MasterViewController and DetailViewController. MasterViewController is linked to DetailViewController with a segue.

MasterViewController has two UITableViewCells. The cell with indexPath [0, 0] has a detailTextLabel that displays a String. Each time I click on this cell, I go to the DetailViewController which, in this case, displays a list of Strings. The cell with indexPath [0, 1] has a detailTextLabel that displays an Int. Each time I click on this cell, I go to the DetailViewController, which in this case, displays a list of Ints.

Furthermore, when I select a cell in DetailViewController, it displays a checkmark and updates the MasterViewController related cell which triggered the segue.

Here is a picture of the different scenes of the project:

And here is my code:

MasterViewController

class MasterViewController: UITableViewController {

    var myString = "Yellow"
    var myInt = 16

    override func viewDidLoad() {
        super.viewDidLoad()

        NSNotificationCenter.defaultCenter().addObserver(self, selector: "attributeChangeMethod:", name: "attributeChange", object: nil)
    }

    func attributeChangeMethod(notif: NSNotification) {
        if let passedString: AnyObject = notif.userInfo?["String"] {
            myString = passedString as String
            tableView.reloadRowsAtIndexPaths([NSIndexPath(forRow: 0, inSection: 0)], withRowAnimation: .Automatic)
        }
        if let passedInt: AnyObject = notif.userInfo?["Int"] {
            myInt = passedInt as Int
            tableView.reloadRowsAtIndexPaths([NSIndexPath(forRow: 1, inSection: 0)], withRowAnimation: .Automatic)
        }
    }

    deinit {
        NSNotificationCenter.defaultCenter().removeObserver(self, name: "attributeChange", object:nil)
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
    }

    override func tableView(tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return 2
    }

    override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {
        let cell = tableView.dequeueReusableCellWithIdentifier("Cell", forIndexPath: indexPath) as UITableViewCell

        if indexPath.row == 0 {
            cell.textLabel?.text = "My string"
            cell.detailTextLabel?.text = myString
        }
        if indexPath.row == 1 {
            cell.textLabel?.text = "My int"
            cell.detailTextLabel?.text = "\(myInt)"
        }
        return cell
    }

    override func prepareForSegue(segue: UIStoryboardSegue, sender: AnyObject?) {
        let controller = segue.destinationViewController as DetailViewController

        if sender as? UITableViewCell == tableView.cellForRowAtIndexPath(NSIndexPath(forRow: 0, inSection: 0)) {
            controller.identifier = "String"
            controller.passedString = myString
        }
        if sender as? UITableViewCell == tableView.cellForRowAtIndexPath(NSIndexPath(forRow: 1, inSection: 0)) {
            controller.identifier = "Int"
            controller.passedInt = myInt
        }
    }

}

DetailViewController

class DetailViewController: UITableViewController {

    var identifier: String!
    var passedString: String!
    var passedInt: Int!
    var array: [Any]!

    override func viewDidLoad() {
        super.viewDidLoad()

        if identifier == "String" {
            title = "Select My String"
            array = ["Yellow", "Green", "Blue", "Red"]
        }
        if identifier == "Int" {
            title = "Select My Int"
            array = [8, 16, 32, 64]
        }
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
    }

    override func tableView(tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return array.count
    }

    override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {
        let cell = tableView.dequeueReusableCellWithIdentifier("Cell", forIndexPath: indexPath) as UITableViewCell

        if identifier == "String" {
            cell.textLabel?.text = array[indexPath.row] as? String
            cell.accessoryType = (array[indexPath.row] as String) == passedString ? .Checkmark : .None
        }
        if identifier == "Int" {
            cell.textLabel?.text = "\(array[indexPath.row] as Int)"
            cell.accessoryType = (array[indexPath.row] as Int) == passedInt ? .Checkmark : .None
        }
        return cell
    }

    override func tableView(tableView: UITableView, didSelectRowAtIndexPath indexPath: NSIndexPath) {
        for cell in tableView.visibleCells() as [UITableViewCell] {
            cell.accessoryType = .None
        }

        tableView.deselectRowAtIndexPath(indexPath, animated: true)
        let cell = tableView.cellForRowAtIndexPath(indexPath)
        cell!.accessoryType = .Checkmark

        //Return the new value back to MasterViewController
        var dict: [String : AnyObject]!
        if identifier == "String" {
            passedString = array[indexPath.row] as? String
            dict = ["String" : array[indexPath.row] as String]
        }
        if identifier == "Int" {
            passedInt = array[indexPath.row] as? Int
            dict = ["Int" : array[indexPath.row] as Int]
        }

        NSNotificationCenter.defaultCenter().postNotificationName("attributeChange", object: nil, userInfo: dict)
    }

}

This code works fine but I have a problem: I don't like it. It doesn't look like Swift style modern code, it looks like a copy-paste from good old Objective-C to Swift. I dislike the fact to have an identifier property in DetailViewController: I always have to check identifier in if statements in order to perform actions. I also dislike the fact to have passedString and passedInt properties with one of them being nil in half of the cases. And, worst of it, I dislike the fact that array is of type [Any]!: it leads to many downcastings.

Is there any better way to write this code? Swift provides tools like gerenic structs and classes or protocol conformance type arrays. Can't they help write more proficient code here?

Answer 1

The problem here isn't that your Swift looks like Objective-C. The problem is that, because you're developing a toy app, you've used a toy design, and now you don't like the toy design. Probably you followed a tutorial you found on the Internet (or even in Apple's docs), and most of those are for toy apps and toy designs. In those tutorials, the author hates adding classes, and ends up littering if statements all over the code. But fewer classes does not necessarily lead to better design.

Here's how I would design this app as if it were not a toy. Notice that in the design below, there are almost no if statements. In serious apps, I try to rely on message dispatch to choose the code path.

Objective-C

I know your question was about Swift, but I'll start with Objective-C to demonstrate that the problem is with the toy design, not the language choice.

First, let's create a real model layer. We have a master model, which is an array of details:

MasterModel.h

#import <Foundation/Foundation.h>

@interface MasterModel : NSObject

/** An array of `DetailModel`. */
@property (nonatomic, strong, readonly) NSArray *details;

@end

and we have a detail model:

DetailModel.h

#import <Foundation/Foundation.h>

@interface DetailModel : NSObject

@property (nonatomic, copy, readonly) NSString *title;
@property (nonatomic, copy, readonly) NSArray *options;
@property (nonatomic) NSUInteger selectedOptionIndex;

- (instancetype)initWithTitle:(NSString *)title options:(NSArray *)options;

@end

In a real app, you'd probably load the model from a database or a file. In this toy app, I'll hard-code it:

MasterModel.m

#import "MasterModel.h"
#import "DetailModel.h"

@implementation MasterModel

- (instancetype)init {
    if (self = [super init]) {
        _details = @[ [self newStringDetailModel], [self newIntDetailModel] ];
    }
    return self;
}

- (DetailModel *)newStringDetailModel {
    NSArray *options = @[ @"Yellow", @"Green", @"Blue", @"Red" ];
    return [[DetailModel alloc] initWithTitle:@"My String" options:options];
}

- (DetailModel *)newIntDetailModel {
    NSArray *options = @[ @8, @16, @32, @64 ];
    return [[DetailModel alloc] initWithTitle:@"My Int" options:options];
}

@end

DetailModel.m

#import "DetailModel.h"

@implementation DetailModel

- (instancetype)initWithTitle:(NSString *)title options:(NSArray *)options {
    if (self = [super init]) {
        _title = [title copy];
        _options = [options copy];
    }
    return self;
}

@end

Now we need a master view controller to display the master model:

MasterViewController.h

#import <UIKit/UIKit.h>

@class MasterModel;

@interface MasterViewController : UITableViewController

@property (nonatomic, strong, readonly) MasterModel *model;

@end

MasterViewController.m

#import "MasterViewController.h"
#import "MasterModel.h"
#import "MasterCell.h"
#import "DetailViewController.h"

@interface MasterViewController ()

@end

@implementation MasterViewController

- (void)awakeFromNib {
    [super awakeFromNib];

    _model = [[MasterModel alloc] init];
}

- (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView {
    return 1;
}

The master table view shows one row per detail model in the master model:

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section {
    return _model.details.count;
}

I use a custom cell class for each row. I just hand the cell its model and the cell takes care of the details:

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath {
    MasterCell *cell = [tableView dequeueReusableCellWithIdentifier:@"detail" forIndexPath:indexPath];
    cell.model = _model.details[indexPath.row];
    return cell;
}

In a real app, you might have many segues, so we dispatch to another method based on the segue identifier:

- (void)prepareForSegue:(UIStoryboardSegue *)segue sender:(id)sender {
    if ([segue.identifier isEqualToString:@"ShowDetail"]) {
        [self prepareForShowDetailSegue:segue sender:sender];
    }
}

Because each master cell knows which detail model it's displaying, we can just ask the cell for its model and hand that off to the detail view controller:

- (void)prepareForShowDetailSegue:(UIStoryboardSegue *)segue sender:(id)sender {
    DetailViewController *destination = segue.destinationViewController;
    MasterCell *cell = sender;
    destination.model = cell.model;
}

@end

MasterCell.h

#import <UIKit/UIKit.h>

@class DetailModel;

@interface MasterCell : UITableViewCell

@property (nonatomic, strong) DetailModel *model;

@end

MasterCell.m

#import "MasterCell.h"
#import "DetailModel.h"

@implementation MasterCell

Remember that cells can be reused. In this app, they won't be (because there are only going to be two instances of MasterCell and they fit on the screen together), but this is the general pattern I follow when I expect cell reuse:

- (void)setModel:(DetailModel *)model {
    [self disconnectFromModel];
    _model = model;
    [self connectToModel];
    [self update];
}

- (void)update {
    if (self.model != nil) {
        self.textLabel.text = self.model.title;
        self.detailTextLabel.text = [self.model.options[self.model.selectedOptionIndex] description];
    }
}

When the user goes into the detail view and changes the selected option, the master cell for that detail needs to be updated. We'll use key-value observing (KVO) to detect the change to the selection.

static char kSelectedOptionIndexContext;

- (void)disconnectFromModel {
    [self.model removeObserver:self forKeyPath:@"selectedOptionIndex" context:&kSelectedOptionIndexContext];
}

Note that if self.model was nil (as it will be the first time the cell is used), the above method has no effect, and if we were to set the model back to nil, the below method would have no effect.

- (void)connectToModel {
    [self.model addObserver:self forKeyPath:@"selectedOptionIndex" options:0 context:&kSelectedOptionIndexContext];
}

- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context {
    if (context == &kSelectedOptionIndexContext) {
        [self update];
    } else {
        [super observeValueForKeyPath:keyPath ofObject:object change:change context:context];
    }
}

Generally it's important to deregister observers when they are deallocated. Otherwise, you will crash if the observed property changes later.

- (void)dealloc {
    [self disconnectFromModel];
}

@end

DetailViewController.h

#import <UIKit/UIKit.h>

@class DetailModel;

@interface DetailViewController : UITableViewController

@property (nonatomic, strong) DetailModel *model;

@end

DetailViewController.m

#import "DetailViewController.h"
#import "DetailModel.h"

@interface DetailViewController ()

@end

@implementation DetailViewController

Since we don't reuse DetailViewController instances, and DetailViewController doesn't use KVO, setModel: is simpler:

- (void)setModel:(DetailModel *)model {
    _model = model;
    self.navigationItem.title = [@"Select " stringByAppendingString:_model.title];
}

- (NSInteger)numberOfSectionsInTableView:(UITableView *)tableView {
    return 1;
}

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section {
    return _model.options.count;
}

In this case, I don't need to store a model reference in the table view cell, and I can get away with just using a generic UITableViewCell:

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath {
    UITableViewCell *cell = [tableView dequeueReusableCellWithIdentifier:@"option" forIndexPath:indexPath];
    cell.textLabel.text = [_model.options[indexPath.row] description];
    if (_model.selectedOptionIndex == indexPath.row) {
        cell.accessoryType = UITableViewCellAccessoryCheckmark;
    } else {
        cell.accessoryType = UITableViewCellAccessoryNone;
    }
    return cell;
}

- (void)tableView:(UITableView *)tableView didSelectRowAtIndexPath:(NSIndexPath *)indexPath {
    UITableViewCell *priorSelectedCell = [tableView cellForRowAtIndexPath:[NSIndexPath indexPathForRow:_model.selectedOptionIndex inSection:0]];
    priorSelectedCell.accessoryType = UITableViewCellAccessoryNone;
    _model.selectedOptionIndex = indexPath.row;
    UITableViewCell *selectedCell = [tableView cellForRowAtIndexPath:indexPath];
    selectedCell.accessoryType = UITableViewCellAccessoryCheckmark;
}

@end

Swift

Now let's use Swift to implement this design. The model is pretty much the same:

MasterModel.swift

import UIKit

class MasterModel: NSObject {

    let details: Array<DetailModel>

    override init() {
        details = [ MasterModel.newStringDetailModel(), MasterModel.newIntDetailModel() ]
        super.init()
    }

    private class func newStringDetailModel() -> DetailModel {
        return DetailModel(title: "My String", options: [ "Yellow", "Green", "Blue", "Red" ])
    }

    private class func newIntDetailModel() -> DetailModel {
        let options: Array<Printable> = [ 8, 16, 32, 64 ]
        return DetailModel(title: "My Int", options:options)
    }

}

DetailModel.swift

import UIKit

class DetailModel: NSObject {

    let title: String
    let options: Array<Printable>
    dynamic var selectedOptionIndex: Int

    init(title: String, options: Array<Printable>) {
        self.title = title
        self.options = options
        self.selectedOptionIndex = 0
    }

}

I've actually cheated a bit by using Printable as the array element type (and by relying on NSObject as the array element type in the Objective-C version). In a real app, I'd probably create my own class hierarchy for the different option types, or at least add categories to NSString and NSNumber to format them appropriately instead of relying on the description provided by Printable/NSObject.

MasterViewController.swift

import UIKit

class MasterViewController: UITableViewController {

    let model = MasterModel()

    override func numberOfSectionsInTableView(tableView: UITableView) -> Int {
        return 1
    }

    override func tableView(tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return model.details.count
    }

    override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {
        let cell = tableView.dequeueReusableCellWithIdentifier("detail", forIndexPath: indexPath) as MasterCell
        cell.model = model.details[indexPath.row]
        return cell
    }

    override func prepareForSegue(segue: UIStoryboardSegue, sender: AnyObject?) {
        if (segue.identifier == "ShowDetail") {
            prepareForShowDetailSegue(segue, sender:sender)
        }
    }

    private func prepareForShowDetailSegue(segue: UIStoryboardSegue, sender: AnyObject?) {
        let destination = segue.destinationViewController as DetailViewController
        let cell = sender! as MasterCell
        destination.model = cell.model
    }

}

MasterCell.swift

import UIKit

class MasterCell: UITableViewCell {

    var model: DetailModel? {
        willSet {
            disconnectFromModel()
        }
        didSet {
            connectToModel()
            update()
        }
    }

What follows is actually the trickiest part in the Swift implementation, because of KVO. The context argument to KVO methods is a void * in Objective-C and an UnsafeMutablePointer<Void> in Swift. I found that it doesn't work unless I explicitly create an UnsafeMutablePointer instance, rather than relying on Swift's & operator, which converts to UnsafeMutablePointer in some circumstances.

Also, I would make this a class variable, but the compiler doesn't support those “yet”.

    // Swift doesn't support class variables yet.
    private lazy var selectedOptionIndexContext = UnsafeMutablePointer<Void>.alloc(1)

    private func disconnectFromModel() {
        model?.removeObserver(self, forKeyPath: "selectedOptionIndex", context: selectedOptionIndexContext)
    }

    private func connectToModel() {
        model?.addObserver(self, forKeyPath: "selectedOptionIndex", options: NSKeyValueObservingOptions(), context: selectedOptionIndexContext)
    }

    private class func toUnsafeMutablePointer(p: UnsafeMutablePointer<Void>) -> UnsafeMutablePointer<Void> {
        return p
    }

    override func observeValueForKeyPath(keyPath: String!, ofObject object: AnyObject!, change: [NSObject : AnyObject]!, context: UnsafeMutablePointer<Void>) {
        if (context == selectedOptionIndexContext) {
            update()
        } else {
            super.observeValueForKeyPath(keyPath, ofObject: object, change: change, context: context)
        }
    }

    private func update() {
        if (model != nil) {
            textLabel?.text = model?.title;
            detailTextLabel?.text = (model?.options[model!.selectedOptionIndex])?.description
        }
    }

Note that, to avoid a memory leak, we need to explicitly deallocated any KVO contexts we created.

    deinit {
        disconnectFromModel()
        selectedOptionIndexContext.dealloc(1)
    }

}

DetailViewController.swift

import UIKit

class DetailViewController: UITableViewController {

    var model: DetailModel? {
        didSet {
            navigationItem.title = "Select " + model!.title
        }
    }

    override func numberOfSectionsInTableView(tableView: UITableView) -> Int {
        return 1
    }

    override func tableView(tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return model!.options.count
    }

    override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {
        let cell = tableView.dequeueReusableCellWithIdentifier("option", forIndexPath: indexPath) as UITableViewCell
        cell.textLabel?.text = model!.options[indexPath.row].description
        if (model!.selectedOptionIndex == indexPath.row) {
            cell.accessoryType = .Checkmark
        } else {
            cell.accessoryType = .None
        }
        return cell
    }

    override func tableView(tableView: UITableView, didSelectRowAtIndexPath indexPath: NSIndexPath) {
        let priorSelectedCell = tableView.cellForRowAtIndexPath(NSIndexPath(forRow: model!.selectedOptionIndex, inSection: 0))
        priorSelectedCell?.accessoryType = .None
        model!.selectedOptionIndex = indexPath.row
        let selectedCell = tableView.cellForRowAtIndexPath(indexPath)
        selectedCell?.accessoryType = .Checkmark
    }

}

I've uploaded both versions to this github repository.