Deal action with a pack of cards

Question

I have a pack of cards and I am doing a 'deal' action. I am dealing n number of cards, for now to one player.

How can I DRY up the while and unless loops to have fewer lines?

def deal(number_of_players, number_of_cards_each)

 # 1..number_of_players

  players_cards_array=[]

  (1..number_of_cards_each).each do |a_card|

    added_card=false
    while added_card==false
      new_card = choose_card()
      unless @currently_dealt_cards.include?(new_card)
        players_cards_array << new_card
        added_card= true
        @currently_dealt_cards << new_card
      end 
    end 
  end 

  return players_cards_array

end

Answer 1

I agree with @mnhg: if you had the collection of cards beforehand, things would simplify dramatically:

players_cards = all_cards.sample(nplayers * ncards_each).each_slice(ncards_each).to_a

Answer 2

If I get your code right you are creating random cards while dealing. I would move this at the beginning to make your dealing cleaner. I'm no ruby guy, so I have to offer some pseudo code:

// List/Array of 52/32... Cards (just a trivial loop over the ranks and suits)
all_cards=generateDeck() 
// use what ever ruby offers, i.e. array_shuffle in php
shuffled_cards=shuffle(all_cards)
card_player[0]=subArray(all_cards,0,numberOfCardsPerPlayer)
..

Of course you can use some fancy stuff here to deal card in the 3-2-4-2 order for skat. You could use some array_pop/shift to get the first/last card of you shuffled deck and assign it to a player.

Answer 3

While I agree with the existing answers (you should refactor to simply shuffle and return the first n cards), you can clean up your existing code dramatically by getting rid of all the temporary variables. The result is 5 slim lines of Ruby:

def deal(number_of_players, number_of_cards_each)
  (1..number_of_cards_each).map do
    begin
      card = choose_card()
    end while @current_dealt_card.include? card
    @currently_dealt_cards << card
    card
  end
end

First thing, note the use of map to do away with the players_card_array stuff. Any time you see this very typical pattern....

def get_items
  # make an empty array
  items_to_return = []

  # map some existing array to a set of new values
  some_other_items.each do |item|
    items.to_return << modify_item(item)
  end

  # return new values
  return items_to_return
end

... you should be using map, and letting the result "fall off" the end of the method and be implicitly returned:

def get_items
  some_other_items.map |item|
    modoify_item(item)
  end
end

Secondly, instead of initializing a watch variable (added_card = false) and looping until it's set to true by some condition inside the loop, make the condition a condition of the loop:

added_card=false
while added_card==false
  new_card = choose_card()
  unless @currently_dealt_cards.include?(new_card)
    players_cards_array << new_card
    added_card= true
    @currently_dealt_cards << new_card
  end 
end 

becomes Ruby's equivalent of a do/while loop:

begin
  card = choose_card()
end while @currently_dealt_cards.include? card
@currently_dealt_cards << new_card

You could further DRY up the above, by having @currently_dealt_cards be a Set, and then use:

begin
  card = choose_card()
end until @currently_dealt_cards.add? card

Answer 4

I would model the domain with objects. A good set of classes would be Deck, Player, and Card. This will hide away implementation details and keep things easy to read. It also helps with naming, e.g. a 'hand' attribute is much easier to grok than 'players_cards_array'.

Then your code might look something like this:

players.each do |player|
  player.hand = @deck.draw(5)
end

Simple, eh? Of course, the details are in the classes. The classes will start very simple but the nice thing is when you need to add more logic you'll have a place for it, and this will help prevent your application from becoming a BBOC (big ball of code). Sample supporting classes might look something like this:

class Player
  attr_accessor :hand
end

class Deck
  attr_reader :cards

  def initialize
    # Create your 52 Cards
    @cards.shuffle!
  end

  def draw(n)
    @cards.take(n)
  end
end