HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Single Player Repeatable BlackJack Game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
repeatableplayergamesingleblackjack

Problem

You can play this game as much as you want and after every game, you win/loss is recorded. Javascript has -0 and +0 so I guess that's why the counter acts weird at the beginning. Took me 4 months to do this (from never having heard of objects to this : feels good). I can't load images here at codereview so the link to the site is hosted here.



`'use strict'
// dom elements and event handlers
var deal = document.getElementById('dealBtn')
var hit = document.getElementById('hitBtn')
var stand = document.getElementById('standBtn')
var reset = document.getElementById('resetBtn')
deal.addEventListener('click', playGame)
hit.addEventListener('click', goToHitMethod)
stand.addEventListener('click', userStands)
reset.addEventListener('click', resetGame)
var playerSum = document.getElementById('playersum')
var dealerSum = document.getElementById('dealersum')
var writeResult = document.getElementById('resultbox')
var winsCounter = document.getElementById('winscounter')
var playerCards = document.getElementById('playercards')
var dealerCards = document.getElementById('dealercards')
var noOfCardsPlayer = 0,
noOfCardsDealer = 0,
noOfWins = 0

// Defining properties and methods for every single card object created by fillPlayingCards function
function CardObject(cardNum, cardSuit) {
this.cardNum = cardNum
this.cardSuit = cardSuit
}
CardObject.prototype.getCardValue = function() {
if (this.cardNum === 'jack' || this.cardNum === 'queen' || this.cardNum === 'king') {
return 10
} else if (this.cardNum === 'ace') {
return 11
} else {
return this.cardNum
}
}

// Deck object constructer with its properties and methods
function DeckObject() {
this.iniDeck = []
this.displayCards = function(cards) { // displaying corresponding card images on DOM
// var fragment = document.createDocumentFragment()
for (var i = 0; i 21, sum is decreased by 10
aces += 1
sum = sum + cards[i].getCardValue()
} else {
sum = sum + cards[i].get

Solution

Good job on writing a full game! But I do have a review:

Semi-colons

JavaScript lines should end with a semi-colon. Your code works, yes, but that's because the interpreter figures out where semicolons should go, and inserts them itself. I don't like relying on that. It can lead to some weird bugs.

To me, it's a bit like lazy texting, like "r u there yet". Yes it gets the point across but it's not English. If you're writing code, take it seriously and be formal.

Also, you current code can't necessarily be minified. Naïve minification will just remove whitespace and linebreaks, so without semicolons your code just becomes one long line of run-on nonsense. Good minifiers don't suffer from this, though.

(Edit: Before the flamewars start, yes, this is my opinion as much as anything. In the comments OP mentions the "standardjs" style guide, which says "No semicolons". I just plain disagree. But I encourage anyone to read standardjs's references and reasons as well, and form their own opinion. Meanwhile, I'll still use semicolons, and I'll keep encouraging others to do the same.)

Naming

Giving things an *Object suffix is redundant - and sort of incorrect. It's redundant because, well, object-oriented programming uses objects. What's interesting is what those object represent. So your DeckObject should just be called Deck. It doesn't represent a "deck object", it represents a deck.

The suffix is also sort of incorrect because DeckObject and CardObject are constructors: They instantiate objects rather than being objects themselves1. It'd make more sense to say deckObject = new Deck();.

Also, PlayingDeck shouldn't be capitalized, since it's not a constructor. It's just a variable, and so should be playingDeck.

Prototyping

Your DeckObject constructor essentially creates a blank object, and then adds all sorts of methods to it with this.sumCards = ... etc.. There's no real reason for this, as none of the functions use closures to simulate private variables or anything like that.

Your CardObject constructor does better: It adds its methods as prototype methods. DeckObject should do the same with its methods.

The difference is very subtle in practice (and in your particular code it doesn't actually make any difference what approach you take). But think of the prototype as the template for instances: Methods on the prototype automagically exist on every instance based on that prototype.

Conversely, methods added in constructor are only added to the instance being constructed. In this case, since it's done in the constructor, all instances will - in your case - still end up having the same methods, and they'll behave the same, but each instance will have its own copy of the methods - they're not part of the template.

Mixing responsibilities

Why is the DeckObject in charge of figuring out image URLs? The images are for the cards, so it'd make more sense for the cards to have that responsibility. The images belong to the cards; not the deck.

Similarly, why isn't the deck responsible for shuffling itself? Calling someDeck.shuffle() would be more natural.

Also, for some reason the DeckObject methods like sumCards take an array of cards. Why? If it's a deck, doesn't it have its own cards? It seems you're not using the deck to model a collection of cards, but as a namespace for methods instead. Which sort of defeats the point. You get things like:

dealerSum.value = mainDealer.sumCards(mainDealer.iniDeck)


when it'd make more sense to write:

dealerSum.value = mainDealer.sum();


(Incidentally, why is it called main dealer? Is there a non-main dealer in a game of Blackjack? Again, the naming is a little odd.)

You also have functions that do too much compared to their names. For instance, checkIfBust does check if the player busted, but it also tallies the score and updates the UI. Not what it says on the tin. Such mixing of concerns also makes it more difficult to change things later. E.g. if you just want to check if a hand is bust, you can't do that without either triggering a UI update, repeating code to check a hand's score, or refactoring checkIfBust to not do so many different things.

I'd separate the game logic from the UI concerns. Create something that that can just play Blackjack in the console, and then worry about UI and presentation. Build a support structure around your core Blackjack "engine" to display the game state.

Domain modelling

What "objects" does a Blackjack game consist of? Deck, dealer, player, and hands. The latter isn't modelled in your code? Instead you model your players as decks, which is a little... off.

True, both are collections of cards, but a hand has a score/sum, a deck does not; a hand can receive new cards, a deck cannot; cards can be drawn from a deck, but not from a hand; decks can be shuffled, hands cannot. And so on. So the two models are very different.

I'd do something like this (this is just a sketch/skeleton

Code Snippets

dealerSum.value = mainDealer.sumCards(mainDealer.iniDeck)
dealerSum.value = mainDealer.sum();
function Card(suit, value) {
  this.suit = suit;
  this.value = value;
  this.imageUrl = ...;
}

function Deck() {
  this.cards = ...; // create a deck of Card objects
  this.shuffle();
}

Deck.prototype = {
  // shuffles the deck
  shuffle: function () { ... },

  // pops a card of the stack
  draw: function () { ... }
};

function Hand() {
  this.cards = [];
}

Hand.prototype = {
  // adds a card to the hand, e.g. hand.addCard(deck.draw())
  addCard: function (card) { ... },

  // checks if bust
  isBust: function () { ... },

  // checks for blackjack
  isBlackjack: function () { ... },

  // sums card values
  score: function () { ... }
};

Context

StackExchange Code Review Q#153251, answer score: 4

Revisions (0)

No revisions yet.