5
\$\begingroup\$

This is a very long solution I've been working on to a Number to Words problem. I've identified that there's a lot of repeating logic (e.g. 'if writing > 0', 'writing = integer /',).

  • How would you refactor this?
  • When refactoring is there a process you use?
  • For readability sake would it be better to break this method into various pieces? Why?

def in_words (integer)
  ones_array = ['zero','one','two','three','four','five','six','seven','eight','nine']
  teens_array = ['blank','eleven','twelve','thirteen','fourteen','fifteen','sixteen','seventeen','eighteen','nineteen']
  tens_array = ['blank','ten','twenty','thirty','fourty','fifty','sixty','seventy','eighty','ninety']

  result = ""

  #FOR MILLIONS
  writing = integer / 1_000_000
  left_over = integer % 1_000_000

  if writing > 0
    result << in_words(writing) << "million "
    integer = left_over
  end

  #FOR THOUSANDS
  writing = integer / 1000
  left_over = integer % 1000

  if writing > 0
    result << in_words(writing) << "thousand "
    integer = left_over
  end

  #FOR HUNDREDS 
  writing = integer / 100
  left_over = integer % 100

  if writing > 0
    result << in_words(writing) << "hundred "
    integer = left_over
  end

  #FOR TENS!!!
  writing = integer / 10
  left_over = integer % 10

  if writing > 0 
    if writing == 1 && left_over > 0
      result << teens_array[left_over]
      left_over = 0
    else
      result << tens_array[writing]
    end
  end

  writing = left_over

  #FOR ONES!!!!
  if writing > 0
    result << ones_array[writing]
  end

  result

end


p in_words(4) # => "four"
p in_words(15) # => "fifteen"
p in_words(92) # => "ninety two" or "ninety-two"
p in_words(101) # => "one hundred one" 
p in_words(9915) # => "nine thousand nine hundred fifteen"
p in_words(1456789) # => 
    #"one million four hundred fifty six thousand seven hundred eighty nine"
\$\endgroup\$

migrated from stackoverflow.com Feb 3 '14 at 20:11

This question came from our site for professional and enthusiast programmers.

2
\$\begingroup\$

I would refactor by using a loop, which has the advantage of letting you easily add new words (i.e. billions). Here is a quick version I wrote up to demonstrate using a loop to reduce the repetition. Just did some basic testing and I'm sure it has some issues, but hopefully it sets you on the right path in your refactoring.

Unfortunately due to the lack of regularity in english the resulting loop is not all that readable :) Its possible that pulling the 'tens', 'teens', etc logic into methods might make this more readable, but ultimately it will still have special cases.

def in_words(integer)
  result = ""
  [[1_000_000, 'million'], [1000, 'thousand'], [100, 'hundred'], [10, ''], [1, '']].each do |n, s|
    next if integer < n
    writing = integer / n
    integer = integer % n

    case n
    when 10
      if writing == 1 && integer != 0
        result << ['eleven','twelve','thirteen','fourteen','fifteen','sixteen','seventeen','eighteen','nineteen'][integer - 1]
        break
      else
        result << ['ten','twenty','thirty','fourty','fifty','sixty','seventy','eighty','ninety'][writing - 1]
      end
    when 1
      result << ['zero','one','two','three','four','five','six','seven','eight','nine'][writing]
    else          
      result << "#{in_words(writing)} #{s}" if writing > 0
    end
  end

  result
end
\$\endgroup\$
3
\$\begingroup\$

You should use Ruby's Numeric#divmod function:

writing, integer = integer.divmod(1_000_000)

Your function parameter claims to accept an integer, so you should also handle negative inputs. (It shouldn't be hard to prepend "negative " and flip the sign.)

\$\endgroup\$
3
\$\begingroup\$

I would take a different approach. First I would convert the number to words using an idealized numbering system, with none of the complications of the tens or teens. By way of example, 413,230 would be 4 ones hundred 1 tens 3 ones thousand 2 ones hundred 3 tens. Here 1 tens 3 ones would be 13. (I have shown the results of this first step in the examples below.) Then I would use the hash GRAMMAR to correct the grammar. This results in there being relatively little code, but a fair amount of data.

Note that the order of the elements in the hash GRAMMAR is important. (Insertion order of hash elements is maintained in Ruby 1.9+. This is the first time I have found that change useful, but useful it is.) The i tens substrings must be replaced first, then the ten i ones substrings (the numbers 11-19) and lastly the i ones substrings. Please let me know if what I have is not quite correct.

GRAMMAR={
  '1 tens'     => 'ten'      , '2 tens'     => 'twenty'  , '3 tens'     => 'thirty'  ,
  '4 tens'     => 'fourty'   , '5 tens'     => 'fifty'   , '6 tens'     => 'sixty'   ,
  '7 tens'     => 'seventy'  , '8 tens'     => 'eighty'  , '9 tens'     => 'ninety'  ,
  'ten 1 ones' => 'eleven'   , 'ten 2 ones' => 'twelve'  , 'ten 3 ones' => 'thirteen',
  'ten 4 ones' => 'fourteen' , 'ten 5 ones' => 'fifteen' , 'ten 6 ones' => 'sixteen' ,
  'ten 7 ones' => 'seventeen', 'ten 8 ones' => 'eighteen', 'ten 9 ones' => 'nineteen',
  '1 ones'     => 'one'      , '2 ones'     => 'two'     , '3 ones'     => 'three'   ,
  '4 ones'     => 'four'     , '5 ones'     => 'five'    , '6 ones'     => 'six'     ,
  '7 ones'     => 'seven'    , '8 ones'     => 'eight'   , '9 ones'     => 'nine'    ,
  }

UNITS=['ones million','ones hundred','tens','ones thousand','ones hundred','tens','ones']

def in_words(n)
  raise ArgumentError, "n = #{n} is not a Fixnum" unless n.is_a? Fixnum
  raise ArgumentError, "n = #{n} not >= 0 and < 10,000,000" if n < 0 || n >= 10_000_000
  return "zero" if n.zero?
  y = 1_000_000
  str = UNITS.each_with_object('') do |t,str|
    x, n = n.divmod(y)
    str << " #{x} #{t}" if x > 0
    y /= 10
  end.lstrip!
  GRAMMAR.each { |k,v| str.gsub!(k,v) }
  str
end

in_words(3_464_284)
# => three million four hundred sixty four thousand two hundred eighty four
# str before LABELS.each ...
#   => 3 ones million 4 ones hundred 6 tens 4 ones thousand 2 ones hundred 8 tens 4 ones

in_words(3_413_230)
# => three million four hundred thirteen thousand two hundred thirty
# str before LABELS.each...
#   => 3 ones million 4 ones hundred 1 tens 3 ones thousand 2 ones hundred 3 tens

in_words(9_000_064)
# => nine million sixty four
# str before LABELS.each ...
#   => # 9 ones million 6 tens 4 ones

in_words(818_000)   
# => eight hundred eighteen thousand
# str before LABELS.each ...
#   => 8 ones hundred 1 tens 8 ones thousand
\$\endgroup\$

Your Answer

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