Is my code unelegant?

Discussion in 'Ruby' started by Mark Walker, Sep 5, 2010.

  1. Mark Walker

    Mark Walker Guest

    Mark Walker, Sep 5, 2010
    #1
    1. Advertising

  2. Mark Walker

    Mark Walker Guest

    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/.
    Mark Walker, Sep 5, 2010
    #2
    1. Advertising

  3. 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/.
    >
    Adam Prescott, Sep 6, 2010
    #3
  4. Mark Walker

    w_a_x_man Guest

    On Sep 5, 4:49 pm, 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.
    >
    > Attachments:http://www.ruby-forum.com/attachment/5007/romannumerals1.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 )
    w_a_x_man, Sep 6, 2010
    #4
  5. Mark Walker

    Ryan Davis Guest

    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.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).
    Ryan Davis, Sep 6, 2010
    #5
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Ron
    Replies:
    1
    Views:
    2,702
    Showjumper
    Jun 24, 2003
  2. Ian
    Replies:
    0
    Views:
    1,386
  3. Ben Miller [msft]

    Re: Code Behind vs. no code behind: error

    Ben Miller [msft], Jun 27, 2003, in forum: ASP .Net
    Replies:
    1
    Views:
    578
    Alphonse Giambrone
    Jun 28, 2003
  4. =?Utf-8?B?Q2FybG8gTWFyY2hlc29uaQ==?=

    Fire Code behind code AND Javascript code associated to a Button Click Event

    =?Utf-8?B?Q2FybG8gTWFyY2hlc29uaQ==?=, Feb 10, 2004, in forum: ASP .Net
    Replies:
    4
    Views:
    21,209
    =?Utf-8?B?Q2FybG8gTWFyY2hlc29uaQ==?=
    Feb 11, 2004
  5. keithb
    Replies:
    1
    Views:
    904
    Bruce Barker
    Mar 29, 2006
Loading...

Share This Page