iOS delegation design pattern

Question

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 BookModelObserver: class {
 func evt_bookModel(bookModel: BookModel, didUpdateCurrentBook currentBook: Book)
}

The Model will update itself, and emit an event back up to the Presenter:

class BookModel {
 private(set) var currentBook: Book!
 weak var observer: BookModelObserver?
 func updateCurrentBook(book: Book) {
 currentBook = book
 observer?.evt_bookModel(self, didUpdateCurrentBook: currentBook
 }
}

The Presenter will implement the BookModelObserver protocol, and handle that emitted event. In some cases, it has to update the View again, and thus will call a function on the BookView.

class BookPresenter: BookModelObserver, BookViewObserver {
 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!)
 }
 }
}

Finally the View will update itself, bringing everything full circle:

class BookView: UIView, UITableViewDataSource, UITableViewDelegate {
 func updateData(book: Book, atIndex index: Int) {
 books[index] = book
 tableView.reloadRowsAtIndexPaths([NSIndexPath(forRow: index, inSection: 0)], withRowAnimation: UITableViewRowAnimation.None)
 }
}

Answer 1

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.