# Request for code review of beginning project

**URL:** <https://www.ruby-forum.com/t/request-for-code-review-of-beginning-project/69479>\
**Category:** Ruby\
**Created:** [September 21, 2006, 9:55am UTC](https://www.ruby-forum.com/t/request-for-code-review-of-beginning-project/69479 "2006-09-21T09:55:40Z")\
**Posts on this page:** 6\
**Page:** 1

<div class="post-metadata">

**Author:** ![bensinc](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/b/8e7dd6/32.png) [@bensinc](https://www.ruby-forum.com/u/bensinc)\
**Post date:** [September 21, 2006, 9:55am UTC](https://www.ruby-forum.com/t/request-for-code-review-of-beginning-project/69479/1 "2006-09-21T09:55:40Z")

</div>

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.](http://code.google.com/p/rubymars/)  
or more directly:  
[http://rubymars.googlecode.com/svn/trunk/model.rb](http://rubymars.googlecode.com/svn/trunk/model.rb)

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

---

<div class="post-metadata">

**Author:** ![bensinc](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/b/8e7dd6/32.png) [@bensinc](https://www.ruby-forum.com/u/bensinc)\
**Post date:** [September 21, 2006, 4:08pm UTC](https://www.ruby-forum.com/t/request-for-code-review-of-beginning-project/69479/2 "2006-09-21T16:08:01Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![bensinc](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/b/8e7dd6/32.png) [@bensinc](https://www.ruby-forum.com/u/bensinc)\
**Post date:** [September 21, 2006, 8:17pm UTC](https://www.ruby-forum.com/t/request-for-code-review-of-beginning-project/69479/3 "2006-09-21T20:17:14Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![bensinc](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/b/8e7dd6/32.png) [@bensinc](https://www.ruby-forum.com/u/bensinc)\
**Post date:** [September 21, 2006, 11:33pm UTC](https://www.ruby-forum.com/t/request-for-code-review-of-beginning-project/69479/4 "2006-09-21T23:33:30Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![bensinc](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/b/8e7dd6/32.png) [@bensinc](https://www.ruby-forum.com/u/bensinc)\
**Post date:** [September 21, 2006, 10:29pm UTC](https://www.ruby-forum.com/t/request-for-code-review-of-beginning-project/69479/5 "2006-09-21T22:29:21Z")

</div>

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

---

<div class="post-metadata">

**Author:** ![bensinc](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/b/8e7dd6/32.png) [@bensinc](https://www.ruby-forum.com/u/bensinc)\
**Post date:** [October 18, 2006, 10:43am UTC](https://www.ruby-forum.com/t/request-for-code-review-of-beginning-project/69479/6 "2006-10-18T10:43:23Z")

</div>

> I think you meant “loop do:”

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

–John