Skip to content
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

Dan criteria instances #282

Merged
merged 16 commits into from
Apr 18, 2016
Merged

Dan criteria instances #282

merged 16 commits into from
Apr 18, 2016

Conversation

dankohn
Copy link
Contributor

@dankohn dankohn commented Apr 12, 2016

@david-a-wheeler Please review this work to convert Criteria to a real class with an instance for each criterion. The key insight was from http://stackoverflow.com/a/36559091/1935918 which is that the class can and should instantiate if it is ever empty, such as on first use and when using reload! from rails console.

Regarding accessors, please note that you are currently testing for 3 accessors that are not present in criteria.yml. I dynamically generate the set of unique accessors and then manually add in those 3, plus :name.

Also note that I added memoization for most class methods via .each, which calls .all, which is memoized. I separately added memoization for .active and each use of .find_by_name.

Comments welcome as always. Otherwise, please mark that you approve and I'll squash merge.

@david-a-wheeler
Copy link
Collaborator

(Repeat of private email):
I wonder – would it be better to split this into 2 classes, “Criterion” to represent a single one, and “Criteria” to represent the collection of them all?

The “Criteria” class could be the place for queries like the badge level, as well as ways to get particular sets of Criteria (e.g., the “active” / non-future criteria).

@dankohn
Copy link
Contributor Author

dankohn commented Apr 13, 2016

It seems like the class/instance method distinction perfectly matches the difference between Criteria and each criterion. Criteria.active correctly captures the appropriate subset, and memoizes the result for fast re-access. I'm not seeing the benefit of a separate Criterion class at this time (though we could always add it later if the need comes up).

@dankohn
Copy link
Contributor Author

dankohn commented Apr 18, 2016

@david-a-wheeler Could I please get the OK to merge?

@david-a-wheeler
Copy link
Collaborator

Ok to merge!

@dankohn dankohn merged commit 5fae482 into master Apr 18, 2016
@dankohn dankohn deleted the dan-criteria-instances branch April 18, 2016 08:18
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.

None yet

3 participants