iOS delegation design pattern

I am working on a project that has a Model-View-Presenter structure and use the delegation design pattern that has the following structure/implementation. I would love feedback.

The ViewController will set up the MVP triad:

class BookViewController: UIViewController {

    private var bookModel: BookModel!
    private var bookView: BookView!
    private var bookPresenter: bookPresenter!

    override func loadView() {
        super.loadView()

        bookModel = BookModel()
        bookView = BookView()
        bookPresenter = BookPresenter(model: bookModel, view: bookView)

        view.addSubview(bookView)
    }
}

The Presenter will set itself up as the observer to both the View and Model:

class BookPresenter: BookModelObserver, BookViewObserver {

    let bookModel: BookModel
    var bookView: BookView

    init(model: BookModel, view: BookView) {
        self.bookModel = bookModel
        self.bookView = bookView        

        self.bookModel.observer = self
        self.bookView.observer = self

        let book = self.model.currentBook
        self.bookView.setData(book)
    }
}

The BookView will have a protocol defined:

protocol BookViewObserver: class {

    func evt_bookView(bookView: BookView, didSelectItem: Book)
}

A user action on the View layer will emit an event to the Presenter:

class BookView: UIView, UITableViewDataSource, UITableViewDelegate {

    weak var observer: BookViewObserver?

    func tableView(tableView: UITableView, didSelectRowAtIndexPath indexPath: NSIndexPath) {
        observer?.evt_bookView(self, didSelectItem: books[indexPath.row])
    }
}

The Presenter will implement the BookViewObserver protocol, and handle that emitted event:

class BookPresenter: BookModelObserver, BookViewObserver {

    func evt_bookView(bookView: BookView, didSelectItem book: Book) {
        model.updateCurrentBook(book)
    }
}

The BookModel will have a protocol defined:

```
protocol BookModelObse

func evt_bookModel(bookModel: BookModel, didUpdateCurrentBook book: Book) {
    var index: Int?
    for (key, value) in DataStore.defaultInstance.books.enumerate() {
        if value.id == book.id {
            index = key
            break
        }
    }

    if let _ = index {
        bookView.updateBook(book, atIndex: index!)
    }
}

I see a few problems in this block of code alone.

Ultimately, this could be written far more simply as:

func evt_bookModel(bookModel: BookModel, didUpdateCurrentBook book: Book) {
    for (key, value) in DataStore.defaultInstance.books.enumerate() where value.id == book.id {
        bookView.updateBook(book, atIndex: index)
        break
    }
}

This eliminates a variable, and better yet, eliminates a force unwrap (that isn't even necessary if you just used the if let correctly).

class BookModel {

    private(set) var currentBook: Book!
    weak var observer: BookModelObserver?

    func updateCurrentBook(book: Book) {
        currentBook = book
        observer?.evt_bookModel(self, didUpdateCurrentBook: currentBook)
    }
}

We've got more unnecessary optional problems in this class. There's not a particularly good reason for currentBook to be an implicitly unwrapped optional in this case. It can cause nothing but problems.

There's currently nothing else to this class, so it's unclear why we need to keep a reference to the book at all, but our updateCurrentBook can be changed slightly to do the same thing, not need any unwrapping code, and not need the implicitly unwrapped optional (which again is only going to lend itself to a future Stack Overflow "fatal error found nil while unwrapping optional" question).

class BookModel {
    private(set) var currentBook: Book?
    weak var observer: BookModelObserver?

    func updateCurrentBook(book: Book) {
        currentBook = book
        observer?.evt_bookModel(self, didUpdateCurrentBook: book)
    }
}

Of course, this might be even better if we just used the didSet property observer method on the property itself:

class BookModel {
    private(set) var currentBook: Book? {
        didSet {
            if let newBook = currentBook {
                observer?.evt_bookModel(self, didUpdateCurrentBook: newBook)
            }
        }
    }

    weak var observer: BookModelObserver?
}

But now it seems weird that currentBook could be set to nil and the property observers not notified of that. So here's what makes most sense to me:

class BookModel {
    var currentBook: Book {
        // a willSet probably also makes sense
        didSet {
            observer?.evt_bookModel(self, didUpdateCurrentBook: newBook)
        }
    }

    weak var observer: BookModelObserver?

    init(book: Book, observer: BookModelObserver? = nil) {
        self.book = book
        self.observer = observer
    }
}

In this case, the book property always has a good value.

Or, alternatively, we make it fully optional. It doesn't matter if it's public or private.

Here's the biggest problem with your original implementation:

let fooBookModel = BookModel()
let fooBook: Book = fooBookModel.currentBook // crash, book is nil, it was never set

This is perhaps not how the class is intended to be used, but nothing at all about your code prevents this usage.

My sample prevents the empty initializer and forces this:

let barBook = Book()
let fooBookModel = BookModel(barBook)
let fooBook = fooBookModel.currentBook

Here, the currentBook property can never be nil. And the property observer code is handled in a far more Swift-standard way versus the sort of Swift-weird setter approach in the original implementation.

StackExchange Code Review Q#122678, answer score: 2