3
\$\begingroup\$

I built a simple Blackjack game in Ruby on Rails, and am looking for feedback, because I believe I may have overcomplicated things. This was a homework assignment, where I was told to make 1 be an Ace card.

$number_array = []

def pull_push_random()
    number = Random.rand(1..10)
    $number_array.push(number)
    $array_sum = $number_array.inject(0){|sum,number_array| sum + number_array }
end

pull_push_random()
pull_push_random()
puts "This is an array #{$number_array}"

puts "Would you like to draw another card (random number)? y/n"
answer = gets.chomp
if answer == 'y'
    pull_push_random()
    puts "This is the array: #{$number_array}"
    if $array_sum < 16
        pull_push_random()
        puts "This is the array #{$number_array}"
    elsif $array_sum > 21
        puts "Done"
    else
        puts "Invalid option"
    end
elsif answer == 'n'
    puts "Done"
else
    puts "You did not enter a valid option."
end
\$\endgroup\$
  • 1
    \$\begingroup\$ Is this really Ruby on Rails, or just Ruby? You can't do puts and gets in Rails. \$\endgroup\$ – 200_success Sep 18 '17 at 22:11
1
\$\begingroup\$

You didn't implement the game of Blackjack

number = Random.rand(1..10)

This does not bear a strong resemblance to drawing from a shuffled deck of playing cards. A value of 10 should occur four times more often than 9, even if Aces are only valued as a 1. I understand the desire to simplify, but this goes a bit far.

To model a dealer with a large (or "infinite") number of decks shuffled into the shoe, draw with replacement by using Random.rand(1..13) and then map all values above 9 to 10, to account for J Q K.

To model a single shuffled deck, or a small number of them, that we draw from without replacement, allocate an array of size at least 52, and use Fisher-Yates-Knuth.

aces are valued as either 1 or 11 according to the player's choice.

This rule is pretty central to the decision of whether to hit or pass. You really can't use simple + (addition) when summarizing a player's hand. Minimally you need to model the number of Ace cards he holds and (separately) the sum of non-Ace card values, to properly determine if he is "bust" yet. Also, please don't make $array_sum global. And a better name for pull_push_random() would be deal().

There are several other house rules, such as betting options, that would add more complexity if implemented. I agree with your design decision to avoid such complexities. One can still capture the essence of Blackjack in a fairly short program, if a handful of important rules are implemented.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Since this was a homework assignment (Sorry I did not note that) the professor requested that 1 be used as an ace. How do I avoid globals? \$\endgroup\$ – tda Sep 18 '17 at 21:45
  • \$\begingroup\$ Globals like $number_array (a vague identifier name, cards would be better) and $array_sum lead to coupling, they lead to hard-to-predict interactions between components. Better to define narrow interfaces which are more easily understood. In this case, define class Player which is responsible for maintaining a player's hand. For player p1 you want to twice call p1.deal(), then ask whether p1.is_bust() or p1.sum() < 16. Notice also that you can bury implementation details, like whether Aces are always one or not, in different classes, and the top-level app won't see details. \$\endgroup\$ – J_H Sep 18 '17 at 22:01
1
\$\begingroup\$

Naming conventions: Please use names in the domain of the program. In this case, blackjack-related words. Names like push_pull_random are confusing because they don't add any readability to the program. At first glance, I'd have no idea this was a blackjack game.

Object Orientation: With an assignment like a BlackJack game, I would hope the class would have already taught about objects. I recommend modeling the game with objects - Card and Deck at the minimum, I would say.

With a Card object, you'd want to be able to create one with a number (1..13) and a suit (hearts, spades, diamonds, and clubs). Also, you'd probably want to output its value (e.g. face cards are 10). And maybe have a nice print representation like K♣.

Here's a sample implementation:

class Card
  attr_accessor :num, :suit

  def initialize(num, suit)
    raise "Invalid card" unless (1..13).include? num
    @num = num
    raise "Invalid suit" unless ["spades", "hearts", "diamonds", "clubs"].include? suit
    @suit = suit
  end

  def value
    @num > 10 ? 10 : @num
  end

  def to_s
    ["","A",2,3,4,5,6,7,8,9,10,"J","Q","K"][num].to_s + 
    {spades: "♠", hearts: "♥", diamonds: "♦", clubs: "♣"}[suit.to_sym]
  end
end

Next, a Deck. This would need to be initialized with all the combinations of nums and suits. You'd need to be able to draw cards, and it should know which ones are left in the deck.

class Deck
  attr_accessor :cards, :cards_played

  def initialize
    @cards = (1..13).to_a.product(["spades", "hearts", "diamonds", "clubs"]).collect{|n,s| Card.new(n,s)}
    @cards_played = []
  end

  def draw(n=1)
    draw = @cards.sample(n).each do |card|
      @cards_played.push @cards.delete(card)
    end
  end

  def cards_left
    @cards - @cards_played
  end
end

(I got a little fancy with the initializer, using product; this could have been done with nested loops as well)

Now you should be able to interact with a player. This is not complete, but it shows how you can string everything together, and how by modeling your domain with objects, it stays very readable and easy to follow.

deck = Deck.new
hand = deck.draw(2)
loop do
  puts "Your cards are: "
  puts hand
  value = hand.map(&:value).sum
  puts "Your hand's value is #{value}"
  if value > 21
    puts "Bust!"
    break
  end
  puts "Hit (H) or Stay (S)?"
  action = gets.chomp.downcase
  if action == "s"
    break
  elsif action == "h"
    hand += deck.draw
  end
end

Sample run:

Your cards are:
4♦
K♣
Your hand's value is 14
Hit (H) or Stay (S)?
h
Your cards are:
4♦
K♣
5♦
Your hand's value is 19
Hit (H) or Stay (S)?
s
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

Naming of Variables and Methods

I agree with what my learned friends have said above regarding naming convention. IMO your naming is not intuitive.

Object Orientation

And re: object orientation: I believe there is no hard and fast rule about needing to develop a Card class and a Deck class - seeing this is a straightforward homework assignment - keep it as simple as possible.

Code must be more readable

Code must be somewhat easy to read and understand. Consider the readability of the code below, compared to your code - I'm sure you'd be able to understand it even though it is written in an entirely different language (c# - sorry about that i'm not near my ruby machine) - the point i'm making is that your code should be easily readable - like you're reading a story book or something - hopefully this will give you some ideas:

public class BlackJackGame
{
        private List<int> hand;

        public BlackJackGame()
        {                
            this.hand = DealHand();

            PrintHand();

            while (YouWannaDrawAgain())
            {
                DrawCard();
            }
        }
}

Simply your code by writing many small methods

If you need to refactor or extract an object out of your existing code, it will be a lot easier if you have many small methods, rather than a large wall of code. It makes a big difference:

Consider the rest of the code below:

public class BlackJackGame
{
    private List<int> hand;

    public BlackJackGame()
    {
        this.hand = DealHand();

        PrintHand();

        while (YouWannaDrawAgain())
        {
            DrawCard();
        }
    }

    private List<int> DealHand()
    {
        return new List<int>() { RandomCard(), RandomCard() };
    }

    private int RandomCard()
    {
        Random randomGenerator = new Random();
        int card = randomGenerator.Next(1, 11);   // card: >= 1 and < 11
        return card;
    }

    private void PrintHand()
    {
        Console.WriteLine("\nTotal: {0}. This is your current hand: {1}", HandScore(), string.Join(",", hand));
    }

    private int HandScore()
    {
        return hand.Sum(x => x);
    }

    private bool YouWannaDrawAgain()
    {
        Console.WriteLine("\nWould you like to draw another card (random number)? y/n");
        char answer = Console.ReadKey().KeyChar;

        if (answer.Equals('y'))
        {
            return true;
        }
        else if (answer.Equals('n'))
        {
            return false;
        }
        else
        {
            Console.WriteLine("\nInvalid input.");
            return false;
        }
    }

    private void DrawCard()
    {
        hand.Add(RandomCard());
        PrintHand();

        if (HandScore() < 16)
        {
            DrawCard();
        }
        else if (HandScore() > 21)
        {
            Bust();
        }
    }

    private static void Bust()
    {
        Console.WriteLine("\nBust!");
        Console.WriteLine("\nThanks for playing. Press any key to exit.");
        Console.ReadLine();

        throw new ArgumentOutOfRangeException("\nBust");
    }
}

And here's how you call the code:

internal class Program
{
    private static void Main(string[] args)
    {
        try
        {
            BlackJackGame bj = new BlackJackGame();
        }
        catch (ArgumentOutOfRangeException ex)
        {
            // game over!
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @MarkThomas - I figured that the concepts i outlined are much more important than the language (also from a pragmatic viewpoint i was not on my ubuntu machine). i hope it's of benefit to everyone nonetheless. \$\endgroup\$ – BKSpurgeon Sep 21 '17 at 11:41
  • 1
    \$\begingroup\$ Yeah, no problem. I agree that there is no hard and fast rule about creating objects, but it's easy in Ruby -- and I'd say that the benefits of many small methods would similarly apply to many small objects (each with a single responsibility, of course). \$\endgroup\$ – Mark Thomas Sep 21 '17 at 11:47
  • \$\begingroup\$ @MarkThomas yes absolutely. \$\endgroup\$ – BKSpurgeon Sep 21 '17 at 11:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.