Is the attached code too messy to be considered "good"?
I can't figure out if I created too many variables.
I'm just now learning Ruby, and I just can't figure out if that's bad
code.
Attachments:
http://www.ruby-forum.com/attachment/5007/romannumerals1.rb
--
Posted via http://www.ruby-forum.com/.
I should add that the point of this program is to have a user enter a
number between 1 and 3000 and it will convert it to Old Roman Numerals.
--
Posted via http://www.ruby-forum.com/.
def old_roman n
[1000,500,100,50,10,5,1].
map{|d| quot, n = n.divmod(d); quot }.
zip( %w(M D C L X V I) ).
map{|n,c| c * n}.join
end
puts "Enter a number and I'll convert " +
"it to the older Roman Numerals!"
puts old_roman( gets.to_i )
On Sep 5, 4:49 pm, Mark Walker <mark.walker...@live.com> wrote:
Is the attached code too messy to be considered "good"?
I can't figure out if I created too many variables.
I'm just now learning Ruby, and I just can't figure out if that's bad
code.Attachments:http://www.ruby-forum.com/attachment/5007/romannumerals1.rb
--
Posted viahttp://www.ruby-forum.com/.
Is the attached code too messy to be considered "good"?
I can't figure out if I created too many variables.
I'm just now learning Ruby, and I just can't figure out if that's bad
code.
This is the type of feedback I'd give my students or coworkers:
# original code
def roman_numerals input
number_of_m = input/1000
remainder = input%1000number_of_d = remainder/500
remainder_2 = remainder%500number_of_c = remainder_2/100
remainder_3 = remainder_2%100number_of_l = remainder_3/50
remainder_4 = remainder_3%50number_of_x = remainder_4/10
remainder_5 = remainder_4%10number_of_v = remainder_5/5
last_remainder = remainder_5%5number_of_i = last_remainder
return_string = ('M' * number_of_m) + ('D' * number_of_d) + ('C' * number_of_c) + ('L' * number_of_l) + ('X' * number_of_x) + ('V' * number_of_v) + ('I' * number_of_i)
end# fold all the unnecessary remainder vars back into input, remove last var
def roman_numerals1 input
number_of_m = input/1000
input = input%1000number_of_d = input/500
input = input%500number_of_c = input/100
input = input%100number_of_l = input/50
input = input%50number_of_x = input/10
input = input%10number_of_v = input/5
input = input%5number_of_i = input
('M' * number_of_m) + ('D' * number_of_d) + ('C' * number_of_c) + ('L' * number_of_l) + ('X' * number_of_x) + ('V' * number_of_v) + ('I' * number_of_i)
end# remove unnecessary variable prefixes
def roman_numerals2 input
m = input/1000
input = input%1000d = input/500
input = input%500c = input/100
input = input%100l = input/50
input = input%50x = input/10
input = input%10v = input/5
input = input%5i = input
('M' * m) + ('D' * d) + ('C' * c) + ('L' * l) + ('X' * x) + ('V' * v) + ('I' * i)
end# remove unnecessary variable prefixes
def roman_numerals3 input
m = input/1000
input = input%1000d = input/500
input = input%500c = input/100
input = input%100l = input/50
input = input%50x = input/10
input = input%10v = input/5
input = input%5i = input
('M' * m) + ('D' * d) + ('C' * c) + ('L' * l) + ('X' * x) + ('V' * v) + ('I' * i)
end# switch to operator equals format
def roman_numerals4 input
m = input / 1000
input %= 1000d = input / 500
input %= 500c = input / 100
input %= 100l = input / 50
input %= 50x = input / 10
input %= 10v = input / 5
input %= 5i = input
('M' * m) + ('D' * d) + ('C' * c) + ('L' * l) + ('X' * x) + ('V' * v) + ('I' * i)
end# rename input and clean up formatting to be more readable
def roman_numerals5 n
m = n / 1000; n %= 1000
d = n / 500; n %= 500
c = n / 100; n %= 100
l = n / 50; n %= 50
x = n / 10; n %= 10
v = n / 5; n %= 5
i = n('M' * m) + ('D' * d) + ('C' * c) + ('L' * l) +
('X' * x) + ('V' * v) + ('I' * i)
end# avoid string +
def roman_numerals6 n
m = n / 1000; n %= 1000
d = n / 500; n %= 500
c = n / 100; n %= 100
l = n / 50; n %= 50
x = n / 10; n %= 10
v = n / 5; n %= 5
i = n[('M' * m), ('D' * d), ('C' * c), ('L' * l),
('X' * x), ('V' * v), ('I' * i)].join
end
Here are a few iterations of what I wrote. Since the process of old roman numerals is reductive, it is essentially an inject:
ROMAN_DEC = [1000, 500, 100, 50, 10, 5, 1]
ROMAN_ROM = %W(M D C L X V I)def roman1 n
units =
ROMAN_DEC.inject(n) { |num, unit| units << num / unit; num % unit }
units.zip(ROMAN_ROM).map { |num, unit| unit * num }.join
enddef roman2 n
units =
ROMAN_DEC.map { |unit| x, n = n.divmod unit; x }.zip(ROMAN_ROM).map { |num, unit| unit * num }.join
end
Eventually I realized that inject wasn't necessary at all and the whole thing could be cleaned up to:
ROMAN = ROMAN_DEC.zip(ROMAN_ROM).sort.reverse
def roman3 n
units =
ROMAN.map { |dec,rom| x, n = n.divmod dec; rom * x }.join
end
Next, benchmarking so you realize the impact of your code:
require 'benchmark'
max = (ARGV.shift || 1_000_000).to_i
puts "# of iterations = #{max}"
Benchmark::bm(20) do |x|
x.report("null_time") do
for i in 0..max do
# do nothing
end
endx.report("roman_numerals") do
for i in 0..max do
roman_numerals 2002
end
endx.report("roman_numerals1") do
for i in 0..max do
roman_numerals1 2002
end
endx.report("roman_numerals2") do
for i in 0..max do
roman_numerals2 2002
end
endx.report("roman_numerals3") do
for i in 0..max do
roman_numerals3 2002
end
endx.report("roman_numerals4") do
for i in 0..max do
roman_numerals4 2002
end
endx.report("roman_numerals5") do
for i in 0..max do
roman_numerals5 2002
end
endx.report("roman_numerals6") do
for i in 0..max do
roman_numerals6 2002
end
endx.report("roman1") do
for i in 0..max do
roman1 2002
end
endx.report("roman2") do
for i in 0..max do
roman2 2002
end
endx.report("roman3") do
for i in 0..max do
roman3 2002
end
end
end
outputs:
# of iterations = 100000
user system total real
null_time 0.010000 0.000000 0.010000 ( 0.010981)
roman_numerals 0.840000 0.000000 0.840000 ( 0.843659)
roman_numerals1 0.840000 0.000000 0.840000 ( 0.839783)
roman_numerals2 0.840000 0.000000 0.840000 ( 0.842840)
roman_numerals3 0.850000 0.000000 0.850000 ( 0.843407)
roman_numerals4 0.830000 0.000000 0.830000 ( 0.837354)
roman_numerals5 0.840000 0.000000 0.840000 ( 0.839698)
roman1 1.820000 0.000000 1.820000 ( 1.819902)
roman2 1.530000 0.000000 1.530000 ( 1.530743)
roman3 1.110000 0.000000 1.110000 ( 1.108522)
So, all your times except the last are basically the same and the only thing I did was massage it to be cleaner. On the last iteration, I did perform a change that actually saves you a fair amount of time and memory by avoiding String#+ in favor of Array#join. String concatination would probably fare just as well, but wouldn't be as readable imo.
My times are larger because I'm doing more looping. Your code is essentially an unwound loop and I wanted to see how it looked wound back up. It doesn't fare as well until I hit my last iteration where it finally becomes a fair balance between readable and performant. I would probably go with my last version over your last version just for readability sake (it isn't like this could will be performed billions of times a day).
On Sep 5, 2010, at 14:49 , Mark Walker wrote:
One immediate modification would be to replace the final two lines of
your roman_numerals() method with:
('M' * number_of_m) + ('D' * number_of_d) + ('C' * number_of_c) + ('L'
* number_of_l) + ('X' * number_of_x) + ('V' * number_of_v) + ('I' *
number_of_i)
since the method will return that without the need for an explicit
return statement.
You'll notice that there is no puts in this, either. Your method
should return a string. Creating the roman numeral version of the
given number is a separate concern to actually printing that number.
You're better off then using puts roman_numerals(argument).
Just looking at it, I see nothing which restricts the input to between
1 and 3000.
On Sun, Sep 5, 2010 at 10:52 PM, Mark Walker <mark.walker360@live.com> wrote:
I should add that the point of this program is to have a user enter a
number between 1 and 3000 and it will convert it to Old Roman Numerals.
--
Posted via http://www.ruby-forum.com/\.