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\$ – Nic Hartley Jul 2 '15 at 0:39

protected by Jamal Mar 3 at 2:26

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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