Skip to content

Conversation

@Janglinator
Copy link
Contributor

@Janglinator Janglinator commented Oct 29, 2020

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!

Copy link

@SgtJorny SgtJorny left a 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.

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

@Janglinator Janglinator Nov 2, 2020

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@Janglinator Janglinator Nov 2, 2020

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.

Nathan Jangula added 2 commits November 2, 2020 16:51
…wasn't taking effect because selectionColor was being determined in the intializer."

This reverts commit f83cd21.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants