patternrubyMinorCanonical
Roman numeral converter in Ruby
Viewed 0 times
numeralromanconverterruby
Problem
For a Project Euler problem, I made a Ruby roman numeral converter:
This works great, but I'm not sure it's good Ruby. Specifically, I'm wondering if the body of the
For example, I don't think its that obvious that
Also, I know I can monkey patch this into the
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
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
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
endThis 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.joinThis 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:
I'd write:
- 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 modifyingnumon 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 alsonumin 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].joinCode 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].joinContext
StackExchange Code Review Q#33170, answer score: 3
Revisions (0)
No revisions yet.