From: Ryan Davis <ryand-ruby@...>
Date: 2010-09-06T10:17:49+09:00
Subject: Re: Is my code unelegant?


On Sep 5, 2010, at 14:49 , Mark Walker 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.

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%1000
> 
>   number_of_d = remainder/500
>   remainder_2 = remainder%500
> 
>   number_of_c = remainder_2/100
>   remainder_3 = remainder_2%100
> 
>   number_of_l = remainder_3/50
>   remainder_4 = remainder_3%50
> 
>   number_of_x = remainder_4/10
>   remainder_5 = remainder_4%10
> 
>   number_of_v = remainder_5/5
>   last_remainder = remainder_5%5
> 
>   number_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%1000
> 
>   number_of_d = input/500
>   input = input%500
> 
>   number_of_c = input/100
>   input = input%100
> 
>   number_of_l = input/50
>   input = input%50
> 
>   number_of_x = input/10
>   input = input%10
> 
>   number_of_v = input/5
>   input = input%5
> 
>   number_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%1000
> 
>   d = input/500
>   input = input%500
> 
>   c = input/100
>   input = input%100
> 
>   l = input/50
>   input = input%50
> 
>   x = input/10
>   input = input%10
> 
>   v = input/5
>   input = input%5
> 
>   i = 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%1000
> 
>   d = input/500
>   input = input%500
> 
>   c = input/100
>   input = input%100
> 
>   l = input/50
>   input = input%50
> 
>   x = input/10
>   input = input%10
> 
>   v = input/5
>   input = input%5
> 
>   i = 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 %= 1000
> 
>   d  = input / 500
>   input %= 500
> 
>   c  = input / 100
>   input %= 100
> 
>   l  = input / 50
>   input %= 50
> 
>   x  = input / 10
>   input %= 10
> 
>   v  = input / 5
>   input %= 5
> 
>   i = 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
> end
> 
> def 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
>   end
> 
>   x.report("roman_numerals") do
>     for i in 0..max do
>       roman_numerals 2002
>     end
>   end
> 
>   x.report("roman_numerals1") do
>     for i in 0..max do
>       roman_numerals1 2002
>     end
>   end
> 
>   x.report("roman_numerals2") do
>     for i in 0..max do
>       roman_numerals2 2002
>     end
>   end
> 
>   x.report("roman_numerals3") do
>     for i in 0..max do
>       roman_numerals3 2002
>     end
>   end
> 
>   x.report("roman_numerals4") do
>     for i in 0..max do
>       roman_numerals4 2002
>     end
>   end
> 
>   x.report("roman_numerals5") do
>     for i in 0..max do
>       roman_numerals5 2002
>     end
>   end
> 
>   x.report("roman_numerals6") do
>     for i in 0..max do
>       roman_numerals6 2002
>     end
>   end
> 
>   x.report("roman1") do
>     for i in 0..max do
>       roman1 2002
>     end
>   end
> 
>   x.report("roman2") do
>     for i in 0..max do
>       roman2 2002
>     end
>   end
> 
>   x.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).