-
Notifications
You must be signed in to change notification settings - Fork 1
Multi coupler factory #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
… per model index.
…aking effect because selectionColor was being determined in the intializer.
SgtJorny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good to me.
CellDataCoupler/Classes/Base.swift
Outdated
| public func getItem(for index: Int) -> BaseCellCoupler? { | ||
| if let cache = cache, let cachedItem = cache[index] { | ||
| return cachedItem | ||
| internal func getItem(for index: Int, forceCache: Bool) -> BaseCellCoupler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should return an optional. If an item doesn't exist for the index, return nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But cellForRowAtIndexPath is non-optional. We have to provide a cell. Here's what I was doing before:
guard let coupler = sections[indexPath.section].factory.getItem(for: indexPath.row) else {
let errorCell = UITableViewCell()
errorCell.textLabel?.text = "CellDataCoupler Error - No Coupler Provided!"
return errorCell
}
I'm honestly not sure why I decided to do that...
I was allowing the coupler fetch to return a nil coupler, which doesn't make sense, since I was displaying an ugly and invalid cell if the user did give me a null coupler. I instead refactored this to require the coupler fetch to have a non-nil coupler:
public typealias CouplerFetch = ((Int) -> BaseCellCoupler)
Sweet, no more fake cells, right?
However, with this new arrayed coupler fetch:
public typealias MultiCouplerFetch = ((Int) -> [BaseCellCoupler])
There's still an opportunity for the user to not give me enough couplers in their array, so it's still something I had to deal with. Instead of punishing the developer by slapping in an ugly cell into their view (which could get released into prod!) I instead print a descriptive message into the console, and give the tableview an "empty" cell with zero height. This makes it essentially invisible.
I also moved this empty cell state into the CouplerFactory, since its this classes' fault I have to deal with this issue anyways, and shouldn't bleed into other classes that don't allow nil coupler/cells.
I'm not really sure of a cleaner way to do this. The problem is tableviews need to know how many cells I plan to give it. I can't just let the user give me however many couplers they feel like in the MultiCouplerFetch (see snippet above), because I can't call that coupler fetch on ALL indexes, that defeats the purpose of the CouplerFactory (which, btw, is to only create couplers when we need them, which can be super expensive for HUGE data sets). I thought of doing another "fetch" to just get the number of couplers that the user plans to return for each index, but now we're starting to look like UITableViewDataSource again, having different numbers based on indexes.
So I just decided the user will need to tell me a fixed number of how many couplers they plan to give me for each iteration, and yell at them if their array doesn't match that number they promised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did that because it is the job of that function to know what it should default to. Delegating that assumption to the getItem function seems like bad practice because it assumes that anything else that will ever call it wants the same defaulting.
Also - I feel like it compromises the "honesty" of getItem. Imagine asking for the index of an int array and, when out of bounds, it returns you -1. Well, now you have to ask yourself if that is real value or not. An optional would have been more true to the data of said array.
If you don't want the ugly cell, that is fine. We can keep the cell you created, but I think the defaulting should occur wherever getItem is called because you give the callers more control over the behavior.
TLDR - I'm fine with defaulting to your new cell, I just think the defaulting should be downstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point.
…wasn't taking effect because selectionColor was being determined in the intializer." This reverts commit f83cd21.
Levi ran into an issue in a project where he wanted to create 2 couplers for each model object. Because of the enormous Realm lookup for this model list, we had to use the CouplerFactory. However, the coupler factory only supports creating one coupler per model object index. This PR fixes that.
For now, in said project, we just "spoofed" the index by taking models.count * 2, but we'd like to avoid that!