It certainly seems like you're working too hard here. Some long methods, a lot of to_i and to_s (and back to_i) and so on. A line like this:
ret_val << a.to_i.to_s[start..3*(i+1)+(rem-1)].to_i.to_s
is, well, horrifying. Sorry.
Quick review
As ckuhn203 pointed out in the comments, this might be worth monkey patching into Integer. But if you choose not to, instantiating an object for it is a bit much. The entire thing is state-less, so it could just as well be a class method or module function.
You make heavy use of the ret_val name. Besides being unnecessarily shortened, it's also completely opaque. Ok, it's the return value, but what is it? It's sort of like naming a method method - it doesn't really explain much.
You make a lot of use of unless some_string.empty?. A simpler solution would be to (conditionally) push strings to an array, and then call join at the end. No need for "manual" string concatenation.
And, as mentioned, there's a lot of number-to-string-to-number stuff going, a lot of slicing and dicing, and tricky loops with complex ranges like start..3*(i+1)+(rem-1). As a rule of thumb, you very, very rarely need to do index arithmetic in Ruby, so if you find yourself doing that, take a step back and reconsider.
In terms of readability, I pretty much just gave up, to be honest. I mean, I get the gist of what your code is doing in one place or another, but I just don't want to read through it in detail and make heads and tails of all the slicing and looping.
It doesn't have to be this way, though. While it's tempting to approach this as slicing up a string, what we're dealing with is just a number, so a little arithmetic works too; you basically want to work with the remainders of dividing by 1000, 100, and 10.
Alternative approach
Your friends for this task are Numeric#divmod and multiple assignments (aka array destructuring). For instance, to split the input number into "thousands-chunks":
chunks = []
while number > 0
number, chunk = number.divmod(1000)
chunks << chunk
end
or, if you want to be a little obtuse about it:
chunks = []
number, chunks[chunks.count] = number.divmod(1000) while number > 0
That basically replaces your chunks_of_thousand method. Note that the chunks array is "backwards", e.g. if number = 1_123_456_089 you'll get:
[89, 456, 123, 1]
Now that we've got chunks to work on, we can again use divmod to our advantage. However, it'd be nice to reorganize those lookup tables a little. I'd do something like this, combining the first 20 numbers into one hash (or just an array)
ZERO_TO_TWENTY = {
0 => nil,
1 => "one",
# ... snip ...
19 => "nineteen"
}.freeze
The TENS can remain as-is for now (although it's recommended to call freeze on objects that are declared as constants), and the NUMBER_SCALE we'll change later.
Now, to convert each 0-999 chunk to English, we can do this (it's not super pretty though - I've included some further alternatives later on):
strings = chunks.map do |number|
string = []
# get the hundreds
hundreds, remainder = number.divmod(100)
string << "#{ZERO_TO_TWENTY[hundreds]} hundred" if hundreds > 0
# get the tens and units (with a special case for numbers below 20)
tens, units = remainder < 20 ? [nil, remainder] : remainder.divmod(10)
string << [ TENS[tens], ZERO_TO_TWENTY[units] ].compact.join("-")
string.join(" ")
end
Yes, I have hard-coded the string "hundred" in there, which is less than ideal. I wouldn't take much to move it to a constant of course. However, there's no sense in sticking it in the NUMBER_SCALE hash like your current code does, but where it doesn't belong.
Now, to match those strings to their "scale", we have Array#zip. Note, however, that for this to work, we have to redefine your NUMBER_SCALE a bit:
NUMBER_SCALE = [
nil,
"thousand",
"million",
"billion",
"trillion"
].freeze
After that, we can do this bit of chaining magic:
strings.zip(NUMBER_SCALE).reverse.flatten.compact.join(" ")
which'll give us the result. E.g.:
in_english(1_123_456_089)
#=> "one billion one hundred twenty-three million four hundred fifty-six thousand eighty-nine"
All together now
All we need is the input checks from your code and we've got:
# constants declared as above
def in_english(number)
number = number.to_i
raise RangeError if number < 0 or number > 999_999_999_999
return "zero" if number.zero?
chunks = []
number, chunks[chunks.count] = number.divmod(1000) while number > 0
strings = chunks.map do |number|
string = []
hundreds, remainder = number.divmod(100)
string << "#{ZERO_TO_TWENTY[hundreds]} hundred" if hundreds > 0
tens, units = remainder < 20 ? [nil, remainder] : remainder.divmod(10)
string << [ TENS[tens], ZERO_TO_TWENTY[units] ].compact.join("-")
string.join(" ")
end
strings.zip(NUMBER_SCALE).reverse.flatten.compact.join(" ")
end
It's not brilliant, but I'd say it's at least better. If nothing else, there's just a lot less code there.
You'll note I've just made a single method here, not a class. You can package it however you want of course (e.g., as ckuhn203 suggested, as a monkey patch on integer) - but I'll leave that as an exercise to the reader. If you make a class, it'd of course be beneficial to break the various parts of the logic into separate (private) methods.
Also of note is that I'm raising a RangeError, as that's the most appropriate error to use when a numeric value is, well, out of range. It would be very nice to append a custom error message to that error while you're at it, to let the user know what the range actually is.
And I'm using zero? instead of == 0, as that's more idiomatic.
I'm a little on the fence about the number.to_i line. I think I'd prefer to assume that a number has been passed in, and if not then it's the user's problem (since the code will just raise a NoMethodError at some point). Garbage In, Garbage Out, basically. I've left it in to match your current code, but I would probably leave it out otherwise.
You can try it here if you'd like.
Further alternatives
Here's a different approach, using a recursive lambda
def in_english(number)
number = number.to_i
raise RangeError if number < 0 or number > 999_999_999_999
return "zero" if number.zero?
convert = -> (number) do
case
when number.zero?
nil
when number < 20
ZERO_TO_TWENTY[number]
when number < 100
tens, units = number.divmod(10)
[ TENS[tens], convert[units] ].compact.join("-")
when number < 1000
hundreds, number = number.divmod(100)
[ "#{ZERO_TO_TWENTY[hundreds]} hundred", convert[number] ].compact.join(" ")
else
chunks = []
number, chunks[chunks.count] = number.divmod(1000) while number > 0
chunks.map(&convert).zip(NUMBER_SCALE).reverse.flatten.compact.join(" ")
end
end
convert[number]
end
I like this a little better myself. The lambda could of course be a (private) method instead, which would make sense in a class. The only thing that's keeping this from simply being a single, recursive method is the "zero" vs nil return values.
You could handle that with just an extra method argument, however. It might in fact be useful to the user - not just as a hack to make the code work. It'd let someone pass in, say, "none" instead of zero which could be nice is some situations.
Doing so, we get a single, recursive method:
def in_english(number, zero_string = "zero")
number = number.to_i
raise RangeError if number < 0 or number > 999_999_999_999
case
when number.zero?
zero_string
when number < 20
ZERO_TO_TWENTY[number]
when number < 100
tens, units = number.divmod(10)
[ TENS[tens], in_english(units, nil) ].compact.join("-")
when number < 1000
hundreds, number = number.divmod(100)
[ "#{ZERO_TO_TWENTY[hundreds]} hundred", in_english(number, nil) ].compact.join(" ")
else
chunks = []
number, chunks[chunks.count] = number.divmod(1000) while number > 0
chunks.map { |n| in_english(n, nil) }.zip(NUMBER_SCALE).reverse.flatten.compact.join(" ")
end
end
It's still quite long for a Ruby method, but I like it all the same.
