Request for code review of beginning project

Ruby
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
Google Code Archive - Long-term storage for Google Code Project Hosting.
or more directly:
http://rubymars.googlecode.com/svn/trunk/model.rb

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

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, :origin, :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 G. II

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 G. 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, :origin, :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 G. II

4

On Sep 21, 2006, at 3:28 PM, John L. wrote:

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

I think you meant “loop do:”

loop do

…

end

James Edward G. II

5

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. :wink:

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

6

I think you meant “loop do:”

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

–John