# Spell a given number in English

I wrote a program that will take a number from 0 to 999,999,999,999 and spell out that number in English.

How could I improve the readability of my code?

class Say
UNITS ={
1=> "one",
2=> "two",
3=> "three",
4=> "four",
5=> "five",
6=> "six",
7=> "seven",
8=> "eight",
9=> "nine"
}
TEENS ={
11=> "eleven",
12=> "twelve",
13=> "thirteen",
14=> "fourteen",
15=> "fifteen",
16=> "sixteen",
17=> "seventeen",
18=> "eighteen",
19=> "nineteen"
}
TENS ={
1=> "ten",
2=> "twenty",
3=> "thirty",
4=> "forty",
5=> "fifty",
6=> "sixty",
7=> "seventy",
8=> "eighty",
9=> "ninety"
}
NUMBER_SCALE = {
1=>"hundred",
2=>"thousand",
3=>"million",
4=>"billion",
5=>"trillion"
}

def initialize(number)
@number = number.to_s
end

def in_english
raise ArgumentError if @number.to_i < 0 or @number.to_i > 999_999_999_999
return "zero" if @number == "0"
ret_val =""
chunks = chunks_of_thousand(@number)
chunks.each.with_index do |n,i|
if n.length > 2
ret_val << " " unless ret_val.empty?
ret_val << "#{UNITS[n.to_i]} #{NUMBER_SCALE}"
two_digit_number = n+n
if two_digit_number.to_i > 0
ret_val << " " unless ret_val.empty?
ret_val << "#{less_than_100(two_digit_number)}"
end
else
if n.to_i > 0
ret_val << " " unless ret_val.empty?
ret_val << less_than_100(n)
end
end
ret_val << " #{NUMBER_SCALE[chunks.length-i]}" if chunks.length-i > 1 && n.to_i > 0
end
#system say #{ret_val}
ret_val
end

private
def chunks_of_thousand(a)
ret_val =[]
n = a.to_i.to_s.length
num_chunks= n/3
rem = n % 3
if rem > 0 then
ret_val << a.to_i.to_s.split("")[0..rem-1].join().to_i.to_s
start = rem
else
start = 0
end
0.upto(num_chunks-1) do |i|
ret_val << a.to_i.to_s[start..3*(i+1)+(rem-1)].to_i.to_s
start= 3*(i+1)+rem
end
ret_val
end

def less_than_100(n)
return UNITS[n.to_i] if n.length == 1
return TENS[n.to_i] if n.length == 2 && n == "0"
return TEENS[n.to_i] if n.length == 2 && n == "1"
return "#{TENS[n.to_i]}-#{UNITS[n.to_i]}" if n.length == 2
raise "Number length is greater then 2"
end
end

• Some might disagree with me, but I think this would be a good candidate for extending ("monkey patching") the Integer class. i.e. 5.to_english => five – RubberDuck Aug 7 '14 at 20:05
• @ckuhn203 yes I have done that before and it is a good approach. I was most concerned with readability of the code that breaks number into groups/chunks. – aarti Aug 7 '14 at 21:09

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.