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  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  0
          ret_val  0
          ret_val  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[0].to_i] if n.length == 2 && n[1] == "0"
    return TEENS[n.to_i] if n.length == 2 && n[0] == "1"
    return "#{TENS[n[0].to_i]}-#{UNITS[n[1].to_i]}" if n.length == 2
    raise "Number length is greater then 2"
  end
end

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

StackExchange Code Review Q#59436, answer score: 11