«

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...annumerals1.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/.

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 <> 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/.
>

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...annumerals1.rb
>
> --
> Posted viahttp://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, 2010, at 14:49 , Mark Walker wrote:

> 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.83969
> 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).