New Rubyist needs some experienced comments

Ruby
1

I’m just starting to use Ruby, and need some direction. I’d appreciate
any and all suggestions on this (very basic) code. I understand the very
basic concepts and syntax of Ruby, but am having a hard time getting a
perspective on how to make it more elegant, more concise. Thanks
everyone!

#The beginnings of a basic craps game. No code for bets, odds, or
payouts yet.

Just using this as an entry into learning to code.

Please comment w/ better, more elegant ways to get things done! Thx.

class CrapsGame
def initialize
puts ‘A game of craps’.split.collect{|x| x.capitalize}.join(’ ')
@start=TRUE
end

def play
go=TRUE
while go
puts ‘Roll the dice? Y or N’
if gets.chomp.upcase == ‘Y’
if @start
shoot
else
shoot_again
end
else
puts ‘So long, sucker.’
go=FALSE
end
end
end

def shoot
die1=rand(6)+1
die2=rand(6)+1
@roll=die1+die2
puts 'You rolled a #{die1.to_s} and a #{die2.to_s} for a
#{@roll.to_s} ’
eval_dice
end

def shoot_again # is there a simple way to combine methods shoot and
shoot_again?
die1=rand(6)+1
die2=rand(6)+1
@roll=die1+die2
puts ‘You rolled a #{die1.to_s} and a #{die2.to_s} for a
#{@roll.to_s}’
eval_dice
end

def eval_dice
if @start #come-out roll
case @roll
when 7,11
game_won
when 2,3,12
game_lost
else
point_set
end
else #not come-out roll
case @roll
when 7
game_lost
when @point
game_won
end
end

end
def game_won
puts ‘You won!’
@start=TRUE
@point= nil
end
def game_lost
puts ‘You lost’
@start=TRUE
@point=nil
end

def point_set
@point = @roll
puts ‘The point is now set at: #{@point.to_s}’
@start=FALSE
end
end

current_game=CrapsGame.new
current_game.play

2

You want string interpolation on some of your puts (puts ‘You
rolled…’), but you’re using single quotes. Not gonna work. Change to
double quotes.

Also, drop the .to_s for each of those. It’s redundant.

I don’t see any difference between shoot and shoot again. I don’t know
what you’re hoping to gain there, but they do exactly the same thing.

I don’t generally use the constants for true and false. It’s not
actually wrong, it just looks weird.

I would use ‘rand(1…6)’ instead of “rand(6) + 1”. On top of that, I
would create a Die class, with a single method (roll) which does exactly
that. I would then create an instance of die and just call die.roll. If
you wanted to get really sneaky, you could modify the die to store and
return its value, create two instances, and then have die1 and die2,
call die1.roll then die2.roll, then use die1.value and/or die2.value
where appropriate. I would also consider creating a to_s method for die
to have it return the string representation of .value, to make it so you
could just stick @die1 or @die2 in the string interpolation section.

Instead of ‘while go’, I would consider just doing ‘while true’, and
where you set go = false, instead just have a break.

Your code doesn’t actually confirm that they entered 'N" to quit (I
would check for n to quit, and print ‘invalid input’ for anything
else.). I would probably also use match instead of ==.

The code to set @point to nil after game lost or game won is probably
irrelevant. You can’t get to a point in the code where it would still
have a value from a previous run (testing would confirm this, but I’m
pretty solidly confident).

Don’t know if any of that is actually helpful, but that’s what I’d call
out on a first reading/CR.

3

PS: Here’s my modified version of your code, with the changes I
suggested.

#The beginnings of a basic craps game. No code for bets, odds, or
payouts yet.

Just using this as an entry into learning to code.

Please comment w/ better, more elegant ways to get things done! Thx.

class Die
attr_accessor :value
def roll
@value = rand(1…6)
end

def to_s
@value.to_s
end
end

class CrapsGame
def initialize
puts ‘A game of craps’.split.collect{|x| x.capitalize}.join(’ ')
@start = true
@die1 = Die.new
@die2 = Die.new
end

def play
while true
puts ‘Roll the dice? Y or N’
if gets.chomp.upcase == ‘Y’
shoot
else
puts ‘So long, sucker.’
break
end
end
end

def shoot
@die1.roll
@die2.roll
@roll = @die1.value + @die2.value
puts “You rolled a #{@die1} and a #{@die2} for a #{@roll}”
eval_dice
end

def eval_dice
if @start #come-out roll
case @roll
when 7,11
game_won
when 2,3,12
game_lost
else
point_set
end
else #not come-out roll
case @roll
when 7
game_lost
when @point
game_won
end
end
end

def game_won
puts ‘You won!’
@start = true
end

def game_lost
puts ‘You lost’
@start = true
end

def point_set
@point = @roll
puts “The point is now set at: #{@point}”
@start = false
end
end

current_game = CrapsGame.new
current_game.play

4

I think the advice by James is good, you can split your program up into
objects with responsibilities. One of the best books about Object
Oriented design in my opinion is “Practical Object Oriented Design”, by
Sandi Metz. It helped me a lot in thinking about how to structure
programs in Ruby.

The other thing is, I try to avoid case statements in ruby, this is a
design decision, but also they are just not as easy to understand as a
hash, or some other solution (like a Hash).

Also, naming variables is really important, especially in dynamic
languages like Ruby, so with that in mind, I would refactor:

module Craps

runs the game

class Croupier
def self.play
self.round
end

def round
  die = Craps::Die.new
  puts "#{die}"
  puts die.result(die.score)
end

end

note the die knows what the result is and whether it has won, lost,

class Die
POINTS = {
2 => :lost,
3 => :lost,
7 => :won,
11 => :won,
12 => :list
}

def self.roll_dice
  rand(1..6)
end

attr_reader :first_dice, :second_dice

def initialize
  @first_dice, @second_dice = Die.roll_dice, Die.roll_dice
end

def score
  self.first_dice + self.second_dice
end

def result(score)
  POINTS[score] || :point_set
end

def to_s
  "You rolled #{self.first_dice} and #{self.second_dice}"
end

end
end

Craps::Croupier.new.round

–

You will notice that I cheated a bit because the Croupier knows
something about what to do when a point is set, but because I don’t know
how to play craps, I didn’t include that. But anyway, you get the idea,
I hope.

5

Thanks James. All your points made sense.
One thing I’m still a little fuzzy about: calls to to_s. When does to_s
get called? When the interpreter sees the interpolation? When an
instance of a class gets created?
On this topic, here’s some code that’s bugging me. If you run it, you’ll
see that when the b1 instance of class Box gets created, it seems to
call the to_s method, because the variable fill is set to ‘(spaces)’.
What gives?

Box drawing class.

class Box

Initialize to given size, and filled with spaces.

def initialize(w,h)
@wid = w
@hgt = h
@fill = ’ ’
end

Change the fill.

def fill(f)
@fill = f
return self
end

Generate (print) the box

def gen
line(’+’, @wid - 2, ‘-’)
(@hgt - 2).times { line(’|’, @wid - 2, @fill) }
line(’+’, @wid - 2, ‘-’)
end

For printing

def to_s
fill = @fill
if fill == ’ ’
fill = ‘(spaces)’
end
return "Box " + @wid.to_s + “x” + @hgt.to_s + ", filled: " + fill
end

private

Print one line of the box.

def line(ends, count, fill)
print ends;
count.times { print fill }
print ends, “\n”;
end
end

Create some boxes.

b1 = Box.new(10, 4)
b2 = Box.new(5,12).fill(’$’)
b3 = Box.new(3,3).fill(’@’)

print "b1 = ", b1, "\nb2 = ", b2, "\nb3 = ", b3, “\n\n”

6

I’m not sure what you’re trying to do.

But.

When you call print, or puts, or p, Ruby knows that it needs a string
there. So, it will attempt to call to_s, if it exists. Object has a
default to_s, so most things have to_s, but in many cases it’s useless.

When you call “print <all that stuff”, Ruby “knows” it needs a string,
and calls to_s then.

This line:
return "Box " + @wid.to_s + “x” + @hgt.to_s + ", filled: " + fill
be better as:
“Box #{@wid}x#{@hgt}, filled: #{fill}”

The to_s functions will get called automatically. You can exclude the
return statement (it’s more idiomatic to leave it out, but some folks
find it bothersome)

If you want to see that your fill is correct, you can puts the result of
each gen:

puts b1.gen
puts b2.gen
puts b3.gen

Or you could change your to_s method to either be or call gen, and then
any time you attempt to puts/print/p b1, you’ll get the box output.

One other note. Some objects have a to_s, and some have a to_str, and
some have both. There’s others like to_i/to_int. The “shorter” versions
will be called implicitly (when needed, without your extra effort), the
“longer” versions are generally only used explicitly.

7

Hi Lazlo,

yes, that’s right, logically it’s the same. Because ruby’s dynamic you
find this kind of logic a lot: Can I call the method I want to on this
object? Is this object null? etc.

The logical “or” operator ‘||’ is often used to ensure that an object
exists and if it doesn’t create a default object.

It’s slightly related to what James wrote above. If objects know how to
print themselves, by having a public to_s method, you get to use all the
methods (like puts), which expect objects to have such a method.
Sounds kind of a bit stupid, but it’s a very powerful technique.

8

Thanks Joe. This will definitely help me get used to the more idiomatic
way of thinking through my code.

So, just to confirm:

def result(score)
POINTS[score] || :point_set
end

… is this the same thing as:

def result(score)
if POINTS[score]
return POINTS[score]
else
return :point_set
end
end

Joe Gain wrote in post #1183267:

I think the advice by James is good, you can split your program up into
objects with responsibilities. One of the best books about Object
Oriented design in my opinion is “Practical Object Oriented Design”, by
Sandi Metz. It helped me a lot in thinking about how to structure
programs in Ruby.

The other thing is, I try to avoid case statements in ruby, this is a
design decision, but also they are just not as easy to understand as a
hash, or some other solution (like a Hash).

Also, naming variables is really important, especially in dynamic
languages like Ruby, so with that in mind, I would refactor:

module Craps

runs the game

class Croupier
def self.play
self.round
end

def round
  die = Craps::Die.new
  puts "#{die}"
  puts die.result(die.score)
end

end

note the die knows what the result is and whether it has won, lost,

class Die
POINTS = {
2 => :lost,
3 => :lost,
7 => :won,
11 => :won,
12 => :list
}

def self.roll_dice
  rand(1..6)
end

attr_reader :first_dice, :second_dice

def initialize
  @first_dice, @second_dice = Die.roll_dice, Die.roll_dice
end

def score
  self.first_dice + self.second_dice
end

def result(score)
  POINTS[score] || :point_set
end

def to_s
  "You rolled #{self.first_dice} and #{self.second_dice}"
end

end
end

Craps::Croupier.new.round

–

You will notice that I cheated a bit because the Croupier knows
something about what to do when a point is set, but because I don’t know
how to play craps, I didn’t include that. But anyway, you get the idea,
I hope.

9

Hi:

Excellent advise by you guys. I would use this class:

class Dice

:attr_ascessor value

@die1
@die2
@value

def roll
@die1 = rand(1…6)
@die2 = rand(1…6)
@value = @die1 + @die2
end

def won?() [2,3,7].contains(@value) end

def lost? [7,11].contains(@value) end

#returns matching number if 0 if not matching dice.
def matching?
@die1 == @die2 ? die1 : 0
end

def field_won?() [2,3,4,8,9,10,11,12].contains(@value) end

def pass?() not [7,11].contains(@value)

end

Then you can write:

if @dice.matching? = 3 #both dice = 3
…
end

if dice.won? etc.

Your code will come together better this way. Really, the logic for
“won” and “lost” belongs elsewhere. That’s because there aren’t
consistent numbers for winning and losing. For example, the number 7 is
a loser normally, but on the come line its a winner. That belongs in
the game object portion.

10

Thanks Joe.
Slowly but surely figuring out those idioms. I’m also reading Sandy
Metz’ book,
which is helping me get a handle on some fundamental OOP concepts. I
still find myself falling
back on clunky code.

I have 4 problems (at least those that I’m aware of!)
1- Class variables. I can’t find a way to maintain a record of the state
of the ‘marker’ to determine whether the point has been established
(and the value of that point) without using class variables.
2- PitBoss. I feel like this is the key. I’m creating an instance of
PitBoss with every roll. If I instantiate
him one time, will that solve my problem? Can’t wrap my mind around
that.
3- Memory. Does the Ruby interpreter collect garbage automatically.
Every time I go through that loop in
Craps::Croupier.play, aren’t I using up more of the heap?
4- Please feel free to blast away at my evaluate_roll method. I’m sure
there’s a way to make it better.

Here’s the code:

module Craps

runs the game

class Croupier

def self.play
  while true
    puts
    puts 'Roll the dice? Y or N'
    ans=gets.chomp.upcase
    if ans== 'Y'
      pit_boss=Craps::PitBoss.new
      pit_boss.say_something
      Craps::Croupier.new.round(pit_boss)
    elsif ans == 'N'
      puts 'OK, bye!'
      break
    else
      puts 'Invalid input.'
    end
  end
end

def round (pit_boss)
  die = Craps::Die.new
  puts "#{die} = #{die.score}" #shows each die and dice total
  pit_boss.evaluate_roll(die.score)
end

end

class PitBoss #evaluates each roll

@@marker = "OFF"  #hate to use a class var here, but can't figure

out how to otherwise keep track of the marker
@@point_value = nil

POINTS = {
    2 => 2,
    3 => 3,
    4 => 4,
    5 => 5,
    6 => 6,
    8 => 8,
    9 => 9,
    10 => 10,
    11 => 11,
    12 => 12
}

def self.marker_state  #reader- don't really need this at this phase
  @@marker
end

def self.marker_state=(val) #writer- don't really need this at this

phase
@@marker=val
end
def say_something
puts “Hi from the Pit Boss”
end
def evaluate_roll(score)

  if @@marker == "ON"  #if the point is already established by 'come

out roll’

    if score == @@point_value
      puts "You hit the point!"
      @@marker = "OFF"
      @@point_value = 0
      :won
    elsif score == 7
      puts "You 7'd out!"
      @@marker = "OFF"
      @@point_value = 0
      :lost
    else
      POINTS[score]
    end

  else # if this is the 'come out roll and @@marker is "OFF"'

    if [2,3,12].include?(score)
      puts "Craps!"
      :lost
    elsif [7,11].include?(score)
      puts "Shooter wins! "
      :won
    else
      @@marker="ON"
      @@point_value= score
      puts "The point is now set to #{@@point_value}"
    end
  end
end

end

class Die

#The die only passes the score back to round, which then passes it to
the PitBoss class
#to evaluate the roll.

def self.roll_dice
  rand(1..6)
end

attr_reader :first_dice, :second_dice

def initialize
  @first_dice, @second_dice = Die.roll_dice, Die.roll_dice
end

def score
  self.first_dice + self.second_dice
end

def to_s
  "You rolled #{self.first_dice} and #{self.second_dice}"
end

end

end

Craps::Croupier.play

Joe Gain wrote in post #1183309:

Hi Lazlo,

yes, that’s right, logically it’s the same. Because ruby’s dynamic you
find this kind of logic a lot: Can I call the method I want to on this
object? Is this object null? etc.

The logical “or” operator ‘||’ is often used to ensure that an object
exists and if it doesn’t create a default object.

It’s slightly related to what James wrote above. If objects know how to
print themselves, by having a public to_s method, you get to use all the
methods (like puts), which expect objects to have such a method.
Sounds kind of a bit stupid, but it’s a very powerful technique.

11

OK, so scratch questions 1 and 2.
I just moved the instantiation of PitBoss out of the loop, and now the
pitboss instance is only called once. And all class vars just got
changed to instance ones. Works fine!
Still would like to know about ruby garbage collection, as well as how
to make that evaluation method prettier!