Is my code unelegant?

[Mark Walker]

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

 

[Mark Walker]

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.

 

[Adam Prescott]

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.

 

[w_a_x_man]

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

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 )

 

[Ryan Davis]

Is the attached code too messy to be considered "good"?
=20
I can't figure out if I created too many variables.
=20
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 =3D input/1000
remainder =3D input%1000
=20
number_of_d =3D remainder/500
remainder_2 =3D remainder%500
=20
number_of_c =3D remainder_2/100
remainder_3 =3D remainder_2%100
=20
number_of_l =3D remainder_3/50
remainder_4 =3D remainder_3%50
=20
number_of_x =3D remainder_4/10
remainder_5 =3D remainder_4%10
=20
number_of_v =3D remainder_5/5
last_remainder =3D remainder_5%5
=20
number_of_i =3D last_remainder
=20
return_string =3D ('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
=20
# fold all the unnecessary remainder vars back into input, remove last = var
def roman_numerals1 input
number_of_m =3D input/1000
input =3D input%1000
=20
number_of_d =3D input/500
input =3D input%500
=20
number_of_c =3D input/100
input =3D input%100
=20
number_of_l =3D input/50
input =3D input%50
=20
number_of_x =3D input/10
input =3D input%10
=20
number_of_v =3D input/5
input =3D input%5
=20
number_of_i =3D input
=20
('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
=20
# remove unnecessary variable prefixes
def roman_numerals2 input
m =3D input/1000
input =3D input%1000
=20
d =3D input/500
input =3D input%500
=20
c =3D input/100
input =3D input%100
=20
l =3D input/50
input =3D input%50
=20
x =3D input/10
input =3D input%10
=20
v =3D input/5
input =3D input%5
=20
i =3D input
=20
('M' * m) + ('D' * d) + ('C' * c) + ('L' * l) + ('X' * x) + ('V' * = v) + ('I' * i)
end
=20
# remove unnecessary variable prefixes
def roman_numerals3 input
m =3D input/1000
input =3D input%1000
=20
d =3D input/500
input =3D input%500
=20
c =3D input/100
input =3D input%100
=20
l =3D input/50
input =3D input%50
=20
x =3D input/10
input =3D input%10
=20
v =3D input/5
input =3D input%5
=20
i =3D input
=20
('M' * m) + ('D' * d) + ('C' * c) + ('L' * l) + ('X' * x) + ('V' * = v) + ('I' * i)
end
=20
# switch to operator equals format
def roman_numerals4 input
m =3D input / 1000
input %=3D 1000
=20
d =3D input / 500
input %=3D 500
=20
c =3D input / 100
input %=3D 100
=20
l =3D input / 50
input %=3D 50
=20
x =3D input / 10
input %=3D 10
=20
v =3D input / 5
input %=3D 5
=20
i =3D input
=20
('M' * m) + ('D' * d) + ('C' * c) + ('L' * l) + ('X' * x) + ('V' * = v) + ('I' * i)
end
=20
# rename input and clean up formatting to be more readable
def roman_numerals5 n
m =3D n / 1000; n %=3D 1000
d =3D n / 500; n %=3D 500
c =3D n / 100; n %=3D 100
l =3D n / 50; n %=3D 50
x =3D n / 10; n %=3D 10
v =3D n / 5; n %=3D 5
i =3D n
=20
('M' * m) + ('D' * d) + ('C' * c) + ('L' * l) +
('X' * x) + ('V' * v) + ('I' * i)
end
=20
# avoid string +
def roman_numerals6 n
m =3D n / 1000; n %=3D 1000
d =3D n / 500; n %=3D 500
c =3D n / 100; n %=3D 100
l =3D n / 50; n %=3D 50
x =3D n / 10; n %=3D 10
v =3D n / 5; n %=3D 5
i =3D n
=20
[('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 =3D [1000, 500, 100, 50, 10, 5, 1]
ROMAN_ROM =3D %W(M D C L X V I)
=20
def roman1 n
units =3D []
ROMAN_DEC.inject(n) { |num, unit| units << num / unit; num % unit }
units.zip(ROMAN_ROM).map { |num, unit| unit * num }.join
end
=20
def roman2 n
units =3D []
ROMAN_DEC.map { |unit| x, n =3D 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 =3D ROMAN_DEC.zip(ROMAN_ROM).sort.reverse
=20
def roman3 n
units =3D []
ROMAN.map { |dec,rom| x, n =3D n.divmod dec; rom * x }.join
end

Next, benchmarking so you realize the impact of your code:

require 'benchmark'
=20
max =3D (ARGV.shift || 1_000_000).to_i
=20
puts "# of iterations =3D #{max}"
Benchmark::bm(20) do |x|
x.report("null_time") do
for i in 0..max do
# do nothing
end
end
=20
x.report("roman_numerals") do
for i in 0..max do
roman_numerals 2002
end
end
=20
x.report("roman_numerals1") do
for i in 0..max do
roman_numerals1 2002
end
end
=20
x.report("roman_numerals2") do
for i in 0..max do
roman_numerals2 2002
end
end
=20
x.report("roman_numerals3") do
for i in 0..max do
roman_numerals3 2002
end
end
=20
x.report("roman_numerals4") do
for i in 0..max do
roman_numerals4 2002
end
end
=20
x.report("roman_numerals5") do
for i in 0..max do
roman_numerals5 2002
end
end
=20
x.report("roman_numerals6") do
for i in 0..max do
roman_numerals6 2002
end
end
=20
x.report("roman1") do
for i in 0..max do
roman1 2002
end
end
=20
x.report("roman2") do
for i in 0..max do
roman2 2002
end
end
=20
x.report("roman3") do
for i in 0..max do
roman3 2002
end
end
end
outputs:

# of iterations =3D 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).