From: "David A. Black" <dblack@...>
Date: 2009-10-21T06:15:46+09:00
Subject: Re: Please Review My First Ruby Program

Hi Simeon --

On Wed, 21 Oct 2009, Simeon Willbanks wrote:

> I've written a simple program to simulate a Soccer game.  Its the first
> full program I've written in Ruby, so please forgive any obvious
> mistakes.  :)  In advance, thanks for your comments.  It requires Ruby
> 1.9.  Please run the unit tests to see the program in action.

Your code looks very nice and overall very idiomatic. I've got a few
things to suggest. They're not necessarily things you urgently need to
do, but I think they'll interest you as things to think about for this
or future programs.

> ### SoccerGame.rb
> require "SoccerTeam"
> require "SoccerStats"
>
> class SoccerGame
>
>  attr_accessor :title
>
>  attr_reader :home, :away,
>    :yellow_cards, :red_cards,
>    :clock, :score, :goals,
>    :first_half, :second_half
>
>  def initialize(home, away)
>    @home = SoccerTeam.new(home['club'],home['players'])
>    @away = SoccerTeam.new(away['club'],away['players'])
>    @yellow_cards = {:home => [], :away => []}
>    @red_cards = {:home => [], :away => []}
>    @score = {:home => 0, :away => 0}
>    @goals = {:home => {}, :away => {}}

If you change that to:

   @goals = { :home => Hash.new(0), :away => Hash.new(0) }

then later, you can do:

   def goal(team, player)
     @score[team] += 1
     @goals[team][player] += 1
   end

without having to test for the presence of the player key.

This method:

>  def yellow_card(team, player)
>    # Second yellow card so, show a red
>    if @yellow_cards[team].index(player)
>      @yellow_cards[team].push player
>      red_card(team, player)
>    else
>      @yellow_cards[team].push player
>    end
>  end

could be:

   def yellow_card(team, player)
     if @yellow_cards[team].include?(player)
       red_card(team, player)
     end
     @yellow_cards[team].push player
   end

which represents the logic a little more cleanly, I think.

This one:

>  def red_card(team, player)
>    @red_cards[team].push player
>    if team == :home
>      # Send player to the showers
>      @home.showers[player] = @home.players[player]
>      @home.players.delete(player)
>    else
>      @away.showers[player] = @away.players[player]
>      @away.players.delete(player)
>    end
>  end

could conceivably be this:

   def red_card(team, player)
     @red_cards[team].push player
     send(team).send_to_showers(player)
   end

However, you have to write a send_to_showers method in the SoccerTeam
class:

   def send_to_showers(player)
     showers[player] = players.delete(player)
   end

That's actually good anyway, because it puts the business logic of the
team in the Team class. (Note that Hash#delete returns the deleted
value, so you can assign it directly to the other hash in one
operation.)

As for the "send(team)" bit: what I'm doing is taking advantage of the
fact that you have "home" and "away" reader attributes, and your team
variable is (we hope) either :home or :away. So I'm sending that
symbol to myself... and that will return either my home team or my
away team. That way, you don't have to inline essentially the same
code twice.

The other thing I played around with was creating a SoccerPlayer
class, and adding SoccerPlayer objects to the players hash in the team
object. Ultimately I think that's a good idea, so that players can be
traded, queried for career stats outside of one particular team, etc.

Finally, I'd recommend splitting out the constants, so that you have:

module Soccer
   class Team

and so on. It's a little more idiomatic, and makes the namespacing
clear.


David

-- 
The          Ruby training with D. Black, G. Brown, J.McAnally
Compleat     Jan 22-23, 2010, Tampa, FL
Rubyist      http://www.thecompleatrubyist.com

David A. Black/Ruby Power and Light, LLC (http://www.rubypal.com)