HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinorCanonical

Roman numeral converter in Ruby

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numeralromanconverterruby

Problem

For a Project Euler problem, I made a Ruby roman numeral converter:

def romanize(num)
    digits = {
        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"
    }
    digits.reduce("") do |acc, digit|
        key, numeral = digit
        occurances, num = num.divmod(key)
        acc + (numeral * occurances)
    end
end


This works great, but I'm not sure it's good Ruby. Specifically, I'm wondering if the body of the reduce block could be shortened, and if it could made more clear.

For example, I don't think its that obvious that num is being modified in the middle of the reduce block.

Also, I know I can monkey patch this into the Fixnum class, but that's not what I'm looking for.

Update #1

@Tokland's answer taught me some new things. His answer works well, and is pretty clever.

If I take just two of his concepts and apply them to mine, the digits.reduce("") ... block can be simplified to:

digits.map do |key, numeral|
    occurances, num = num.divmod(key)
    numeral * occurances
end.join


This needs one less line because it deconstructs the digit key/value in the block arguments. Also, it creates an array which is joined into a string at the end. For big numbers (around 100-trillion), this is much quicker.

The only problem I still see is that it's modifying the num variable, which is essentially global state to the block.

Solution

Some notes:

  • Use 2 spaces for indentation, not 4.



  • key, numeral = digit: You can unpack block arguments: do |acc, (key, numeral)|.



  • Concating strings is costly, better build an array and finally join it.



  • You are using inject, which is great because it'a functional abstraction, but you are modifying num on each iteration, so it ends up being a weird mixture of functional and imperative style. I know it's a bit cumbersome, but conceptually it's better to keep also num in the accumulator.



I'd write:

digits.reduce(:output => [], :num => num) do |state, (key, numeral)|
  ocurrences, remainder = state[:num].divmod(key)
  {:output => state[:output]  remainder} 
end[:output].join

Code Snippets

digits.reduce(:output => [], :num => num) do |state, (key, numeral)|
  ocurrences, remainder = state[:num].divmod(key)
  {:output => state[:output] << (numeral * ocurrences), :num => remainder} 
end[:output].join

Context

StackExchange Code Review Q#33170, answer score: 3

Revisions (0)

No revisions yet.