Refactor sort process into a service

samvera / questioning_authority

Question your authorities

Other

54 stars 30 forks source link

Refactor sort process into a service #189

Closed elrayle closed 5 years ago

elrayle commented 5 years ago

The sort process is complex. The tests for deep_sort_service lays out the various potential sorting scenarios. Precedence on sort...

numeric sort, if both are integers
simple alpha sort of one term, if both are single value
same language list comparison, if all terms in a list are the same language or no language
preferred language comparison only, if both lists have the at least one term in the preferred language
calculate the comparator value for every language individually and sum to get a relative comparison of two multi-language lists (Best I could come up with. I am open to suggestions. In most cases, if not all, one of the earlier comparators will be selected. This is the junk drawer of multi-lingual sorting.)

randalldfloyd commented 5 years ago

@elrayle @cam156 Just throwing in an initial response from a first pass...

Any custom sort is going to be complex so there's no way around that, but I was able to follow the logic and syntax for the most part. The way it is used and supposed to work is still eluding me a bit so I'm going to read the tests to see it in action and maybe that will help.

I was also trying to think if any of the heavy lifting or setup could be simplified by using the Comparable mixin. Not sure it would make it better or worse or just not applicable:

https://docs.ruby-lang.org/en/2.5.0/Comparable.html. https://medium.com/@farsi_mehdi/the-comparable-mixin-8a3096fa1a0a https://rubyplus.com/articles/2791-Ruby-Comparable-Basics

I don't have experience with it in Ruby but I've seen it in Java. I think the gist of it is that you get to supply behavior for comparison operators that objects are going to get for free anyway (or something like that) which could potentially simplify a layer of comparison setup.

elrayle commented 5 years ago

@cam156 @randalldfloyd Reworked DeepSortService to make better use of ruby <=> and comparators patterns.

elrayle commented 5 years ago

@randalldfloyd I took a look at the links you provided. Thanks for adding those.

RE: Comparable mixin - The doc states "Comparable uses <=> to implement the conventional comparison operators (<, <=, ==, >=, and >) and the method between?." I don't think we need these extra methods so I didn't add the mixin.

elrayle commented 5 years ago

I think I'm going to make one more change to this. It bothers me that DeepSortElement is a class that anyone can use since it is specific to the DeepSortService. I'm going to make it a private class using the approach described here... https://blog.arkency.com/2016/02/private-classes-in-ruby/

elrayle commented 5 years ago

Done and ready for review.

elrayle commented 5 years ago

@cam156 Requested changes are complete.