10
\$\begingroup\$

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[0].to_i]} #{NUMBER_SCALE[1]}"
        two_digit_number = n[1]+n[2]
        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[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
\$\endgroup\$
  • 3
    \$\begingroup\$ 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 \$\endgroup\$ – RubberDuck Aug 7 '14 at 20:05
  • \$\begingroup\$ @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. \$\endgroup\$ – archie Aug 7 '14 at 21:09
11
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.