Request for code review of beginning project

Discussion in 'Ruby' started by Ben Schaffhausen, Sep 21, 2006.

  1. Hello,

    I have some experience in programming ruby as well as java, (and
    certainly enjoy ruby more) but I don't feel I am that great(eg I can
    build tree houses, but I'm no contract carpenter). Part of the solution
    is of course simply programming more- to this end I started working on a
    Mars game that has mechanics based on "The Settlers" series of RTS
    games.

    To start I am making a few classes to model the buildings and delivery
    or materials to and from other buildings. Currently the program is ~250
    lines long and seems to work within my intentions thus far.

    Still I can't help but feel that the code is messy or that I'm doing
    things the hard way/non-ruby-way. So before I became to entrenched in
    pre-existing code, I thought I would ask if someone would be kind enough
    to look through my code and provide suggestions (Although I'm a little
    concerned that this list may not be the appropriate place to ask, if so
    I apologize in advance).

    So if you feel like helping a amateur programmer out, or just interested
    in the project in general, here's where you can find it:

    Code available on Google's Project page
    http://code.google.com/p/rubymars/
    or more directly:
    http://rubymars.googlecode.com/svn/trunk/model.rb

    Thank you,
    Ben Schaffhausen
    bhs128 (at) gmail (dot) com

    --
    Posted via http://www.ruby-forum.com/.
     
    Ben Schaffhausen, Sep 21, 2006
    #1
    1. Advertising

  2. On Sep 21, 2006, at 2:55 AM, Ben Schaffhausen wrote:

    > Hello,


    Hello and welcome to Ruby.

    > To start I am making a few classes to model the buildings and delivery
    > or materials to and from other buildings. Currently the program is
    > ~250
    > lines long and seems to work within my intentions thus far.


    "seems to work" is a good sign that you might enjoy learning a little
    about unit testing. You could build tests to to ensure that it is
    indeed working as you expect and give you peace of mind as the code
    grows.

    > Still I can't help but feel that the code is messy or that I'm doing
    > things the hard way/non-ruby-way.


    I really didn't see any big red flags in the code. Mostly you are
    assigning to or making minor adjustments to variables. There's not a
    lot for us to improve on for something like that.

    Some general tips:

    * Drop the parens around if( ..) lines. Ruby doesn't need them.
    * Try to use indices as little as possible. They are generally un-
    Rubyish. Here's an example targeting your queue:

    >> # with indices...

    ?> @q = [ ["Beads", "Denver", "Oklahoma City"],
    ?> ["Keyboards", "New Haven", "Chicago"] ]
    => [["Beads", "Denver", "Oklahoma City"], ["Keyboards", "New Haven",
    "Chicago"]]
    >> # show what is going to where

    ?> @q.each { |item| puts "#{item[0]} to #{item[2]}" }
    Beads to Oklahoma City
    Keyboards to Chicago
    => [["Beads", "Denver", "Oklahoma City"], ["Keyboards", "New Haven",
    "Chicago"]]
    >> # without indices...

    ?> Shippment = Struct.new:)product, :eek:rigin, :destination)
    => Shippment
    >> @q = [ Shippment.new("Beads", "Denver", "Oklahoma City"),

    ?> Shippment.new("Keyboards", "New Haven", "Chicago") ]
    => [#<struct Shippment product="Beads", origin="Denver",
    destination="Oklahoma City">, #<struct Shippment product="Keyboards",
    origin="New Haven", destination="Chicago">]
    >> # show what is going to where

    ?> @q.each { |ship| puts "#{ship.product} to #{ship.destination}" }
    Beads to Oklahoma City
    Keyboards to Chicago
    => [#<struct Shippment product="Beads", origin="Denver",
    destination="Oklahoma City">, #<struct Shippment product="Keyboards",
    origin="New Haven", destination="Chicago">]

    Hope that helps.

    James Edward Gray II
     
    James Edward Gray II, Sep 21, 2006
    #2
    1. Advertising

  3. Thanks for taking the time to look through it- I appreciate the advice.

    I'll have to review how to write test cases again and put some in place.

    I intend on integrating this file/set of classes in with a Ruby-GNOME2 /
    OpenGL interface but am kind of aprehensive about where to instatiate my
    classes and still maintain decent seperation of code.

    Obviously there is going to be a lot of interaction as GTK takes in the
    user input, and through gtk/opengl commands handles the output. My
    current idea is to have all of the building(and perhaps courier) objects
    have a draw() function where it will draw it's current state. So somehow
    both my draw loop and event handlers (each in different classes already)
    both need access to all of my City classes (and eventually a Player
    class of some sort to hold multiple cities)

    I'm already in a similar situation in my current test opengl program (a
    cube that I can rotate and zoom (ArcBall rotations/Quarternions) and I'm
    already using global variables for the rot_x, rot_y and zoom (so that
    both the event handlers and the opengl draw function have access). I'll
    post that code tonight.

    Anyways, I'm kinda thinking out loud at this point, any comments are
    always appreciated.

    Thanks again,
    Ben Schaffhausen

    James Gray wrote:
    > On Sep 21, 2006, at 2:55 AM, Ben Schaffhausen wrote:
    >
    >> Hello,

    >
    > Hello and welcome to Ruby.
    >
    >> To start I am making a few classes to model the buildings and delivery
    >> or materials to and from other buildings. Currently the program is
    >> ~250
    >> lines long and seems to work within my intentions thus far.

    >
    > "seems to work" is a good sign that you might enjoy learning a little
    > about unit testing. You could build tests to to ensure that it is
    > indeed working as you expect and give you peace of mind as the code
    > grows.
    >
    >> Still I can't help but feel that the code is messy or that I'm doing
    >> things the hard way/non-ruby-way.

    >
    > I really didn't see any big red flags in the code. Mostly you are
    > assigning to or making minor adjustments to variables. There's not a
    > lot for us to improve on for something like that.
    >
    > Some general tips:
    >
    > * Drop the parens around if( ..) lines. Ruby doesn't need them.
    > * Try to use indices as little as possible. They are generally un-
    > Rubyish. Here's an example targeting your queue:
    >
    > >> # with indices...

    > ?> @q = [ ["Beads", "Denver", "Oklahoma City"],
    > ?> ["Keyboards", "New Haven", "Chicago"] ]
    > => [["Beads", "Denver", "Oklahoma City"], ["Keyboards", "New Haven",
    > "Chicago"]]
    > >> # show what is going to where

    > ?> @q.each { |item| puts "#{item[0]} to #{item[2]}" }
    > Beads to Oklahoma City
    > Keyboards to Chicago
    > => [["Beads", "Denver", "Oklahoma City"], ["Keyboards", "New Haven",
    > "Chicago"]]
    > >> # without indices...

    > ?> Shippment = Struct.new:)product, :eek:rigin, :destination)
    > => Shippment
    > >> @q = [ Shippment.new("Beads", "Denver", "Oklahoma City"),

    > ?> Shippment.new("Keyboards", "New Haven", "Chicago") ]
    > => [#<struct Shippment product="Beads", origin="Denver",
    > destination="Oklahoma City">, #<struct Shippment product="Keyboards",
    > origin="New Haven", destination="Chicago">]
    > >> # show what is going to where

    > ?> @q.each { |ship| puts "#{ship.product} to #{ship.destination}" }
    > Beads to Oklahoma City
    > Keyboards to Chicago
    > => [#<struct Shippment product="Beads", origin="Denver",
    > destination="Oklahoma City">, #<struct Shippment product="Keyboards",
    > origin="New Haven", destination="Chicago">]
    >
    > Hope that helps.
    >
    > James Edward Gray II



    --
    Posted via http://www.ruby-forum.com/.
     
    Ben Schaffhausen, Sep 21, 2006
    #3
  4. On Sep 21, 2006, at 12:55am, Ben Schaffhausen wrote:

    > Still I can't help but feel that the code is messy or that I'm
    > doing things the hard way/non-ruby-way.


    The code looks great. If every "beginning" Ruby programmer wrote
    that way... well, I'm not sure what would happen. ;)

    As someone else pointed out, you can lose many of those parentheses,
    especially the clauses after "if".

    I tend to drop "return" for short methods that don't have much
    structural complexity -- in other words, if the last statement of the
    flow of execution in the method is "return foo", you can replace that
    with simply "foo". You get more of a functional language feel,
    rather than procedural.

    I'd add an #== method to Coord so you can easily compare two of them:

    def ==(other)
    @x == other.x && @y == other.y && @z == other.z
    end

    Consider making a Request class to encapsulate the (product, origin,
    dest) tuple. This would make your queue-handling a little more
    obvious (instead of q[0], etc.).

    I'd investigate using #select and #inject instead of code like this:

    count = 0
    @couriers.each { |c| count += 1 if c.available? }

    In fact, it might be better for methods like #available_couriers to
    return an array (using #select), instead of a count. Then you can
    just say "available_couriers.count" to get the count. However, it
    might be inefficient, depending on how often this is done and how
    large the lists are.

    If you define a #log method that did the formatting (or even delegate
    that to Log4r), you could shorten your various "puts" statements.

    Instead "while(true)", you can just use "loop begin".

    Well, I hope all that helps a bit.

    --John
     
    John Labovitz, Sep 21, 2006
    #4
  5. On Sep 21, 2006, at 3:28 PM, John Labovitz wrote:

    > Instead "while(true)", you can just use "loop begin".


    I think you meant "loop do:"

    loop do
    # ...
    end

    James Edward Gray II
     
    James Edward Gray II, Sep 21, 2006
    #5
  6. > I think you meant "loop do:"

    Yes, thanks. (For some reason, I always type it wrong when I'm
    actually coding, too.)

    --John
     
    John Labovitz, Sep 21, 2006
    #6
    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. Code Review Request

    , Jun 30, 2005, in forum: Java
    Replies:
    2
    Views:
    521
    Andrew Thompson
    Jun 30, 2005
  2. P Kenter

    Request for code review

    P Kenter, May 28, 2004, in forum: C++
    Replies:
    4
    Views:
    362
    P Kenter
    Jun 2, 2004
  3. www
    Replies:
    51
    Views:
    1,487
  4. Andrea Francia
    Replies:
    2
    Views:
    252
    Steven D'Aprano
    Jan 6, 2009
  5. Jesse B.
    Replies:
    9
    Views:
    236
    Jesse B.
    Mar 27, 2010
Loading...

Share This Page