From: John Labovitz <johnl@...>
Date: 2006-09-22T05:28:59+09:00
Subject: Re: Request for code review of beginning project

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