8
\$\begingroup\$

I'm writing a program to convert integers to Roman numerals (naively -- it doesn't know how to do the subtraction trick yet). What I have is functional, but it is not "Good Ruby".

VALUES = [
  ["M", 1000], 
  ["D", 500], 
  ["C", 100], 
  ["L", 50], 
  ["X", 10], 
  ["V", 5], 
  ["I", 1], 
]

def romanize n
  roman = ""

  VALUES.each do |pair|
    letter = pair[0]
    value = pair[1]
    roman += letter*(n / value)
    n = n % value
  end
  return roman
end

Perhaps a hash makes more sense than the array of arrays, but the way I update n, order matters. Passing in pair to the block is dumb, but passing letter, value didn't work in the way I expected.

\$\endgroup\$
  • 2
    \$\begingroup\$ With ruby 1.9 hashes are also ordered. \$\endgroup\$ – knut Jan 18 '12 at 21:22
  • 1
    \$\begingroup\$ No kidding! That's good to know. \$\endgroup\$ – tom Jan 20 '12 at 11:45
  • \$\begingroup\$ I'm unsure why 'passing letter, value didn't work in the way [you] expected'; it worked well for me in Ruby 1.8.7p352 and 1.9.2p290. \$\endgroup\$ – MarkDBlackwell Jan 22 '12 at 2:00
  • 3
    \$\begingroup\$ "With ruby 1.9 hashes are also ordered". Hashes are not ordered, they are remembered in their insertion order. It's a subtle difference. \$\endgroup\$ – the Tin Man Jan 27 '12 at 22:17
12
\$\begingroup\$

I don't know what you mean about 'good ruby'. I show you a object-oriented version. Instead of

romanize(4)

you can call

4.roman

My solution is similar to your solution. It is Hash-oriented, so you need ruby 1.9.

I also tried to use a array in array solution with ROMAN_NUMBERS.each do |value, letter| without any problem (I'm not sure if it is only ruby 1.9).

class Fixnum
  ROMAN_NUMBERS = {
    1000 => "M",  
     900 => "CM",  
     500 => "D",  
     400 => "CD",
     100 => "C",  
      90 => "XC",  
      50 => "L",  
      40 => "XL",  
      10 => "X",  
        9 => "IX",  
        5 => "V",  
        4 => "IV",  
        1 => "I",  
  }

  def roman
    n = self
    roman = ""
    ROMAN_NUMBERS.each do |value, letter|
      roman << letter*(n / value)
      n = n % value
    end
    return roman
  end
end

With ruby 1.8 you may add use ROMAN_NUMBERS.sort.each.

As an alternative, you can use a recursive solution:

class Fixnum
  ROMAN_NUMBERS = {
    1000 => "M",  
     900 => "CM",  
     500 => "D",  
     400 => "CD",
     100 => "C",  
      90 => "XC",  
      50 => "L",  
      40 => "XL",  
      10 => "X",  
        9 => "IX",  
        5 => "V",  
        4 => "IV",  
        1 => "I",  
        0 => "",  
  }
  def roman
    return '' if self == 0
    ROMAN_NUMBERS.each do |value, letter|
      return ( letter * (self / value)) << (self % value).roman if value <= self
    end
    return (self % value).roman
  end
end

I don't recommend the recursive solution. Each number must start again with M.


A little testcode, to check the results:

require 'test/unit'

class NumberTest < Test::Unit::TestCase
  def test_0; assert_equal('',0.roman); end
  def test_1; assert_equal('I',1.roman); end
  def test_2; assert_equal('II',2.roman); end
  def test_3; assert_equal('III',3.roman); end
  def test_4; assert_equal('IV',4.roman); end
  def test_5; assert_equal('V',5.roman); end
  def test_6; assert_equal('VI',6.roman); end
  def test_7; assert_equal('VII',7.roman); end
  def test_8; assert_equal('VIII',8.roman); end
  def test_9; assert_equal('IX',9.roman); end
  def test_10; assert_equal('X',10.roman); end
  def test_11; assert_equal('XI',11.roman); end
  def test_12; assert_equal('XII',12.roman); end
  def test_13; assert_equal('XIII',13.roman); end
  def test_14; assert_equal('XIV',14.roman); end
  def test_15; assert_equal('XV',15.roman); end
  def test_16; assert_equal('XVI',16.roman); end
  def test_20; assert_equal('XX',20.roman); end
  def test_30; assert_equal('XXX',30.roman); end
  def test_40; assert_equal('XL',40.roman); end
  def test_50; assert_equal('L',50.roman); end
  def test_60; assert_equal('LX',60.roman); end
  def test_70; assert_equal('LXX',70.roman); end
  def test_80; assert_equal('LXXX',80.roman); end
  def test_90; assert_equal('XC',90.roman); end
  def test_99; assert_equal('XCIX',99.roman); end
  def test_100; assert_equal('C',100.roman); end
  def test_200; assert_equal('CC',200.roman); end
  def test_300; assert_equal('CCC',300.roman); end
  def test_400; assert_equal('CD',400.roman); end
  def test_500; assert_equal('D',500.roman); end
  def test_600; assert_equal('DC',600.roman); end
  def test_900; assert_equal('CM',900.roman); end
  def test_1000; assert_equal('M',1000.roman); end
  def test_2000; assert_equal('MM',2000.roman); end
  def test_2003; assert_equal('MMIII',2003.roman); end
end
\$\endgroup\$
3
\$\begingroup\$

Regardless of whether your code works completely, converted to the Ruby way it perhaps would be:

letters = %w[ M     D    C    L   X   V  I ]
values  =   [ 1000, 500, 100, 50, 10, 5, 1 ]
LETTERS = letters.zip values

def romanize number
  n=number
  c=0 # Avoid reallocating count.
  LETTERS.map{|l,v| c, n = n.divmod v; l*c}.join ''
end
\$\endgroup\$
  • 1
    \$\begingroup\$ It's wrong Try this: p romanize(4); Result will be: "IIII" \$\endgroup\$ – A H K Jan 16 '15 at 8:58
  • \$\begingroup\$ @AHK it's not so wrong, in the sophisticated and venerable sense of Why Do Some Clocks Use Roman Numeral IIII? and the Alternative forms section of Wikipedia's article on Roman_numerals. \$\endgroup\$ – MarkDBlackwell Jun 18 '15 at 12:40
  • \$\begingroup\$ Oh! i didn't know that @Mark \$\endgroup\$ – A H K Jun 19 '15 at 13:48
1
\$\begingroup\$

MarkDBlackwell's use of divmod and map works with the hash containing the 4s & 9s from knut's answer.

Franken-solution cobbled together from their answers below:

ROMAN_NUMS = {
  "M" => 1000,
  "CM" => 900, "D" => 500, "CD" => 400, "C" => 100,
  "XC" => 90,  "L" => 50,  "XL" => 40,  "X" => 10,
  "IX" => 9,   "V" => 5,   "IV" => 4,   "I" => 1
}

def romanize(num)
  ROMAN_NUMS.map do |ltr, val| 
    amt, num = num.divmod(val)
    ltr * amt
  end.join
end

Also works as a version closer to knut's original answer.

class Fixnum

  ROMAN_NUMS = {
    "M" => 1000,
    "CM" => 900, "D" => 500, "CD" => 400, "C" => 100,
    "XC" => 90,  "L" => 50,  "XL" => 40,  "X" => 10,
    "IX" => 9,   "V" => 5,   "IV" => 4,   "I" => 1
  }

  def roman
    num = self
    ROMAN_NUMS.map do |ltr, val| 
      amt, num = num.divmod(val)
      ltr * amt
    end.join
  end

end
\$\endgroup\$
  • 2
    \$\begingroup\$ When you're posting an answer on Code Review, please make sure it's a code review, not other code that does the same thing, or a code dump of the "better" code. Instead, please include a list of the changes you made alongside the modified code. \$\endgroup\$ – Fund Monica's Lawsuit Jul 2 '15 at 0:39

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