Refactor sort process into a service

[elrayle]

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.)

[elrayle]

Multiple fixes to yarddoc @return statements. Oops ;)

[carolyncole]

@elrayle This is not a complete review. I'm suggesting some rejiggering of the code locations which I believe would make it easier to read and review. @randalldfloyd what are your thoughts?

That said it may be easier for you to make additional refactorings a separate step (or not at all).

[carolyncole]

I wonder if this would be easier to read if the array was first mapped to a deep sort element and then sorted. The setup comparison method is a little magic which makes it a smidge hard to read. It may also be more efficient since you are constructing the elements only once instead of at each comparison.

   sortable_elements = @the_array.map {|item|  DeepSortElement.new(item[sort_key)}
  @sorted_array = soratble_elements.sort do |a_element, b_element|
      if a_element.integer? && b_element.integer?
          numeric_comparator(a_element, b_element)
     elsif a_element.single? && b_element.single?
          single_value_comparator(a_element, b_element)
     else 
          multiple_value_comparator(a_element, b_element)
     end
  end

Or what could be even better is if the DeepSortElement knew how to compare itself with another element and this would read

   sortable_elements = @the_array.map {|item|  DeepSortElement.new(item[sort_key)}
  @sorted_array = soratble_elements.sort do |a_element, b_element|
       a_element.compare(b_element)
  end

You could then put a bunch of this code into the DeepSortElement class.

[elrayle]

I like that idea. I'll look at making that adjustment.

[carolyncole]

If the array was mapped to DeepSortElements you could take advantage of sorted_literals in that class here?

[randalldfloyd]

@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]

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

[elrayle]

@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]

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]

Done and ready for review.

[carolyncole]

@elrayle Just a couple simplifications that could be made. I like this version!

[carolyncole]

Given the literals are sorted by language you could just test language == preferred_language since the first item will either be in the prefered language or it is not in the list at all.

[elrayle]

Done

[carolyncole]

Since the literal list is sorted by language you should be able to compare language(0) == language(size-1) to see if they are all the same

[elrayle]

Done

[carolyncole]

this can be simplified into filtered_literals_by_language.fetch(filter_language,[]) see https://ruby-doc.org/core-2.5.1/Hash.html#method-i-fetch

[elrayle]

Done

[carolyncole]

This can also be simplified by fetch filter = bins.fetch(lang, [])

[elrayle]

Done

[randalldfloyd]

This version is a lot easier to understand on a first read. Given the substantial tests showing it in action, any developer should be able to come into this cold and be able to debug/refactor/enhance without too much trouble if they take the time to digest it.

[elrayle]

@Cam156 Requested changes are complete.