dry-rb / dry-inflector

Inflector for Ruby
https://dry-rb.org/gems/dry-inflector
MIT License
95 stars 15 forks source link

Ensure #ordinalize to work with all the numbers from 0 to 100 #14

Closed jodosha closed 6 years ago

jodosha commented 6 years ago

@sondnm reported in chat:

inflector = Dry::Inflector.new
inflector.ordinalize(17) # nil
inflector.ordinalize(24) # nil
inflector.ordinalize(25) # nil
inflector.ordinalize(34) # nil

This PR fixes the problem.

abinoam commented 6 years ago

By the way @jodosha, the current Rails implementation has a clearer semantic.

We put 4 to 16 as exceptions. When the only real exceptions are 11 to 13. 4 to 10 and 14 to 16 falls into the "not ending with 1, 2 or 3" category, the else branch.

Well, I don't know If I'm making myself clear. What I mean is... Looking at the ActiveSupport code is clearer to me that all numbers ending in 1, 2, or 3 has a special end aside from 11, 12 and 13.

We could take this opportunity to improve this. What do you think?

    # Returns the suffix that should be added to a number to denote the position
    # in an ordered sequence such as 1st, 2nd, 3rd, 4th.
    #
    #   ordinal(1)     # => "st"
    #   ordinal(2)     # => "nd"
    #   ordinal(1002)  # => "nd"
    #   ordinal(1003)  # => "rd"
    #   ordinal(-11)   # => "th"
    #   ordinal(-1021) # => "st"
    def ordinal(number)
      abs_number = number.to_i.abs

      if (11..13).include?(abs_number % 100)
        "th"
      else
        case abs_number % 10
        when 1; "st"
        when 2; "nd"
        when 3; "rd"
          else    "th"
        end
      end
    end

    # Turns a number into an ordinal string used to denote the position in an
    # ordered sequence such as 1st, 2nd, 3rd, 4th.
    #
    #   ordinalize(1)     # => "1st"
    #   ordinalize(2)     # => "2nd"
    #   ordinalize(1002)  # => "1002nd"
    #   ordinalize(1003)  # => "1003rd"
    #   ordinalize(-11)   # => "-11th"
    #   ordinalize(-1021) # => "-1021st"
    def ordinalize(number)
      "#{number}#{ordinal(number)}"
    end
abinoam commented 6 years ago

And...

I would not indent the else beyond the whens. Ugly!

The foolowing is prettier.

        case abs_number % 10
        when 1; "st"
        when 2; "nd"
        when 3; "rd"
        else    "th"
        end
Ptico commented 6 years ago

@abinoam Looks like (4..16) was introduced by me, i dunno why 🤦‍♂️

jodosha commented 6 years ago

@Ptico @abinoam Thanks for the really good feedback. I wasn't sure about the ORDINALIZE_TH values, but with 11..13 the tests are passing. Please feel free to cleanup, according your experience. 👍


Regarding the Set vs Hash it's a perf trick: Set wraps a Hash to guarantee unique results. So we can avoid the wrapping at all and use Hash#key?, which is a bit faster than Set#include?. See https://gist.github.com/jodosha/743d5c8672ec8d90358f7cbc50f39806

abinoam commented 6 years ago

which is a bit faster than Set#include?

👍 So let's stay with Hash

I'll take the opportunity to do the 11..13 trick.

jodosha commented 6 years ago

Thank you all. Great team work. High 5! ✋

abinoam commented 6 years ago

High 5 ✋