deck.rb: Code Review

[scott-steadman]

Suggestions in comments...

[scott-steadman]

Change fresh_deck from class variable to method. Reason: I prefer methods to constants or class variables because it allows you stub that method during testing. Also, this will reduce your startup time because the code won't be invoked until fresh_deck is called.

def fresh_deck
  deck = []

  ['A','2','3','4','5','6','7','8','9','10','J','Q','K'].each do |rank|
    ['Clubs', 'Diamonds', 'Hearts', 'Spades'].each do |suit|
      deck << Card.new(rank, suit)
    end
  end

  return deck
end

[scott-steadman]

Another solution here would be to add ranks and suits class methods to Card. Then you could do something like this...

class Card
...
  def self.ranks
    ['A','2','3','4','5','6','7','8','9','10','J','Q','K']
  end

  def self.suits
    ['Clubs', 'Diamonds', 'Hearts', 'Spades']
  end
...
end // class Card

def Deck
...
private
  def fresh_deck
    deck = []
    Card.ranks.each do |rank|
      Card.suits.each do |suit|
        deck << Card.new(rank, suit)
      end
    end
    return deck
  end

end // class Deck

[scott-steadman]

Make the cards attribute read-only. Reason: cards seems to be a internal variable that you won't want someone else to change accidentally.

Example Code:

class Deck
  attr_reader :cards

  def initialize
    @cards = fresh_deck
  end

end // class Deck

Alternatively, you could just memoize the cards method:

class Deck

  # no initializer needed

  def cards
    @cards ||= fresh_deck
  end

end // class Deck

[scott-steadman]

Add a shuffle! method Reason: I think this will simplify your deal logic. You'll just pop a card off the deck.

def shuffle!
   cards.shuffle!
end

def deal_one_card
  cards.pop || raise IndexError 'Out of cards'
end

Note: cards is an array of cards, so you're calling the shuffle! method on that array.

[joncodo]

Thats a great idea. Ill do that.

[joncodo]

Expect these changes to be done on friday night. Ill be coding then if you would like to join me

[scott-steadman]

Rename show to to_s Reason: to_s is the idiomatic way to render a class as a string

Example code:

def to_s
  cards.to_s
end