12
\$\begingroup\$

I have created a JavaScript based game, which is based on the card game War. I think I am reusing a lot of my code, but I am not entirely sure what I can really improve. I have a lot of functions, which could probably be improved.

Please take a look and give me a review of it. I think I nailed it, but let me know what you think. Also, it is supposed to run on a Node.js server, so basically everything will be authorized through the server, which will check everything through before giving the user input (which prevents faking their total money, their bettings, etc.).

Note: Some variables may not be used in the code. That's on purpose (most of them are used) because it is for the future of the project.

Also on JSFiddle - To view the actual page Go here!

//Each player's deck
var dealerDeck = [];
var playerDeck = [];

//52 cards = 26 cards left
var dealerAmountLeft = 26;
var playerAmountLeft = 26;

//The amount of money and the bet amount
var playerTotal = 40;
var betAmount = 0;

//Each round, dealerCard and playerCard are set to a random card from their deck
//warDealerCards/warPlayercards is a string, which contains 1, 2, then 3 cards seperated by a +
var dealerCard, playerCard, warDealerCards, warPlayerCards;

//Checks how many cards has been giving when a war has been started
var warCards = 0;

//Whether a war has been started or not
var warStarted = false;

//Keeps track of how many times each player has won in a started war
var dealerWarsWon = 0;
var playerWarsWon = 0;
var warInterval;

//Sets a language
var language = "en-GB";

//Card class
function Card(value, name, suit) {
    this.value = value;
    this.name = name;
    this.suit = suit;
}

//Deck class
function Deck() {
    switch (language) {
        case "da-DK":
            this.suits = ['Hjerter', 'Ruder', 'Spar', 'Klør'];
            this.names = ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', 'knægt', 'dronning', 'konge'];
            break;
        case "en-US":
            this.suits = ['Hearts', 'Diamonds', 'Spades', 'Clubs'];
            this.names = ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', 'Jack', 'Queen', 'King'];
            break;
        case "en-GB":
            this.suits = ['Hearts', 'Diamonds', 'Spades', 'Clubs'];
            this.names = ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', 'Jack', 'Queen', 'King'];
            break;
    }
    var cards = [];

    for (var s = 0; s < this.suits.length; s++) {
        for (var n = 0; n < this.names.length; n++) {
            cards.push(new Card(n + 1, this.names[n], this.suits[s]));
        }
    }
    return cards;
}

//Created a deck
var myDeck = new Deck();

function shuffle(o) {
    for (var j, x, i = o.length; i; j = parseInt(Math.random() * i), x = o[--i], o[i] = o[j], o[j] = x);
    return o;
};

//Shuffles the deck
myDeck = shuffle(myDeck);

//Gives cards to each player (dealer and player)
function giveCards(o) {
    var count = o.length / 2;
    for (i = 0; i < count; i++) {
        dealerDeck.push(o.shift(0));
        playerDeck.push(o.shift(0));
    }
    dealerAmountLeft = 26;
    playerAmountLeft = 26;
}

giveCards(myDeck);

//Created a DealerCard class
function DealerCard() {}

//Overrides toString() on the dealer card class, which means "new DealerCard().toString()" will return the suit and name
DealerCard.prototype.toString = function() {
    return dealerCard[0].suit + " " + dealerCard[0].name;
}

//Created a PlayerCard class
function PlayerCard() {}

//Overrides toString() on the player card class, which means "new PlayerCard().toString()" will return the suit and name
PlayerCard.prototype.toString = function() {
    return playerCard[0].suit + " " + playerCard[0].name;
}

//Call this = Give each player a random card from their deck
function selectCards() {
    dealerCard = dealerDeck.splice(Math.floor(Math.random() * dealerDeck.length), 1);
    playerCard = playerDeck.splice(Math.floor(Math.random() * playerDeck.length), 1);

    //Ace is the highest/best card. King is second highest card. King's value == 13, therefore set Ace's value to 14 (this is hacky)
    if (dealerCard[0].value == 1) {
        dealerCard[0].value = 14;
    }
    if (playerCard[0].value == 1) {
        playerCard[0].value = 14;
    }
}

//Sets a war
function setWar() {
    warStarted = true;
    $("#dealer").text("");
    $("#player").text("");
    startWar();
    warInterval = setInterval(startWar, 1000);
}

//Starts a war
function startWar() {
    //Only run this while the war is still going
    if (warCards < 3) {
        selectCards();
        if (warCards == 0) {
            warDealerCards = new DealerCard().toString();
            warPlayerCards = new PlayerCard().toString();
            $("#dealer").append(warDealerCards);
            $("#player").append(warPlayerCards);
        } else {
            warDealerCards += " + " + new DealerCard().toString();
            warPlayerCards += " + " + new PlayerCard().toString();
            $("#dealer").text(warDealerCards);
            $("#player").text(warPlayerCards);
        }
        checkWinner();
        warCards++;
        dealerAmountLeft--;
        playerAmountLeft--;
        console.log("War cards: " + warCards);
    } else if (warCards == 3) {
        warWinner();
        warCards = 0;
        warStarted = false;
        $("#bet").show();
    }
}

//Does stuff like creating history, when the winner has been found
function warWinner() {
    if (playerWarsWon >= 2 && warCards == 3) {
        console.log("PLAYER WON!!!");
        createHistory("#12345", "you", warDealerCards, warPlayerCards);
        $("#winner").text("Player won!").addClass("winnerFound");
        setPlayerTotal(betAmount, false);
    } else if (dealerWarsWon >= 2 && warCards == 3) {
        setPlayerTotal(betAmount, true);
        createHistory("#12345", "dealer", warDealerCards, warPlayerCards);
        $("#winner").text("Dealer won!").addClass("winnerFound");
        console.log("DEALER WON!!!");
    }
    clearInterval(warInterval);
    warStated = false;
}

//Checks who wins each card draw. Pretty self-explanatory
function checkWinner() {
    if (warStarted) {
        if (dealerCard[0].value < playerCard[0].value) {
            $("#winner").text("Player wins!");
            playerWarsWon++;
        } else if (dealerCard[0].value > playerCard[0].value) {
            $("#winner").text("Dealer wins!");
            dealerWarsWon++;
        } else if (dealerCard[0].value == playerCard[0].value) {
            $("#winner").text("TIME TO WAR");
            $("#warButtons").show();
            $("#bet").hide();
            $("#deal").hide();
        }
    } else {
        warDealerCards = new DealerCard().toString();
        warPlayerCards = new PlayerCard().toString();
        if (dealerCard[0].value < playerCard[0].value) {
            $("#winner").text("Player wins!");
            createHistory("#12345", "you", warDealerCards, warPlayerCards);
            setPlayerTotal(betAmount, false);
        } else if (dealerCard[0].value > playerCard[0].value) {
            $("#winner").text("Dealer wins!");
            createHistory("#12345", "dealer", warDealerCards, warPlayerCards);
            setPlayerTotal(betAmount, true);
        } else if (dealerCard[0].value == playerCard[0].value) {
            $("#winner").text("TIME TO WAR");
            $("#warButtons").show();
            $("#bet").hide();
            $("#deal").hide();
        }
    }
}

//When the deal button has been pressed, start dealing the cards
$("#deal").on("click", function() {
    console.log("War cards: " + warCards);
    console.log("Amount of player cards: " + playerAmountLeft);
    if (betAmount > playerTotal) {
        alert("BET AMOUNT TOO HIGH");
        playerTotal(0);
        updateBetAmount();
    } else if (playerTotal > 0) {
        selectCards();
        dealerAmountLeft--;
        playerAmountLeft--;

        $("#dealer").text(new DealerCard().toString());
        $("#player").text(new PlayerCard().toString());

        checkWinner();

        if (playerAmountLeft == 0) {
            console.log("GAME DONE! STARTING NEW");
            myDeck = new Deck();
            myDeck = shuffle(myDeck);
            giveCards(myDeck);
        }
    } else {
        alert("OUT OF MONEY");
    }
});

//Sets the bet amount when one of the amounts has been clicked
$(".amount").on("click", function() {
    setBetAmount($(this).attr("data-amount"), false);
    $(".amount").removeClass("selected");
    $(this).addClass("selected");
});

//Resets the bet amount
$("#reset").on("click", function() {
    setBetAmount(betAmount, true);
});

//Checks which button has been pressed (forfeit, continue or double)
//Used to set the bet amount
$("#warButtons button").on("click", function() {
    if ($(this).attr("data-id") == "forfeit") {
        setPlayerTotal(betAmount * 0.9, true);
    } else if ($(this).attr("data-id") == "continue") {
        setWar();
    } else if ($(this).attr("data-id") == "double") {
        setBetAmount(betAmount, false);
        setWar();
    }
    $("#deal").show();
    $("#warButtons").hide();
});

//Function to update the bet amount
function updateBetAmount() {
    $("#betAmount").text("Bet Amount: " + betAmount);
}

//Function to update the player total
function updatePlayerTotal() {
    $("#totalAmount").text("Total: " + playerTotal);
}

//Function to set the bet amount. Takes 2 arguments. n is the amount and neg determines if it should deduct or add the amount
function setBetAmount(n, neg) {
    if (neg) {
        betAmount += -Math.abs(parseFloat(n))
    } else {
        betAmount += parseFloat(n);
    }
    if (betAmount >= playerTotal) {
        betAmount = playerTotal;
    }
    updateBetAmount();
}

//Sets the player total after winning or losing 
function setPlayerTotal(n, neg) {
    if (neg) {
        playerTotal += -Math.abs(parseFloat(n));
    } else {
        playerTotal += parseFloat(n);
    }
    updatePlayerTotal();
}

//Adds an "element" to the history on the left side of the screen
function createHistory(id, winner, dealer, player) {
    $("#historyHeader").after('<div class="historyGame">ID: <span class="historyId"></span>' + id + ' - Winner: <span class="historyWinner">' + winner + '</span> - Bet amount: <span class="historyAmount">' + betAmount + '</span><br /><span class="historyDealer">Dealer: <span class="historyDealerInner">' + dealer + '</span></span><br /><span class="historyPlayer">You: <span class="historyPlayerInner">' + player + '</span></span></div>');
    if (winner == "you") {
        $(".historyGame:first .historyPlayerInner").addClass("historyWinner");
    } else {
        $(".historyGame:first .historyDealerInner").addClass("historyWinner");
    }
    if ($(".historyGame").length > 1) {
        $(".historyGame:first").append("<hr>");
    }
}

updateBetAmount();
updatePlayerTotal();
#game {
    width: 800px;
    height: 100%;
    margin: 0 auto;
    text-align: center;
}

#deal {
    width: 150px;
    height: 70px;
    font-size: 30px;
}

#dealer,
#player {
    height: 300px;
    font-size: 30px;
    font-weight: bold;
}

#winner {
    height: 100px;
    font-size: 40px;
}

#warButtons {
    display: none;
    -webkit-user-select: none;
    -moz-user-select: none;
    -khtml-user-select: none;
    -ms-user-select: none;
}

#warButtons button {
    width: 100px;
    height: 40px;
    font-size: 16px;
}

.amount {
    width: 50px;
    height: 50px;
    border-radius: 25px;
    line-height: 50px;
    background-color: #CCC;
    display: inline-block;
    cursor: pointer;
    -webkit-user-select: none;
    -moz-user-select: none;
    -khtml-user-select: none;
    -ms-user-select: none;
}

#betAmount {
    -webkit-user-select: none;
    -moz-user-select: none;
    -khtml-user-select: none;
    -ms-user-select: none;
}

.amount:hover {
    background-color: #CCA;
}

.selected {
    background-color: #CCA;
}

.winnerFound {
    font-size: 80px !important;
}

#history {
    width: 400px;
    height: 200px;
    overflow-y: scroll;
    overflow-x: hide;
    background-color: #CCC;
    float: left;
    margin: 0;
    padding: 6px;
}

#history hr {
    border: 1px solid black;
    margin: 20px 0px 20px 0px;
}

#historyHeader {
    height: 30px;
    width: 400px;
    background-color: darkgrey;
    padding: 0;
    margin: 0;
    text-align: center;
    font-size: 28px;
    line-height: 30px;
}

.historyGame:nth-child(2) {
    margin-top: 10px;
}

.historyWinner {
    color: lime !important;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div id="history">
    <div id="historyHeader">History</div>
</div>
<div id="game">
    <div id="betAmount"></div>
    <div id="totalAmount"></div>
    <div id="dealer"></div>
    <div id="winner"></div>
    <div id="player"></div>
    <div id="bet">
        <button id="reset">Reset</button>
        <div class="amount" data-amount="1.00">1</div>
        <div class="amount" data-amount="2.00">2</div>
        <div class="amount" data-amount="2.50">2.5</div>
        <div class="amount" data-amount="5.00">5</div>
    </div>
    <div id="warButtons">
        <button data-id="forfeit" title="Forfeit the game and get 90% of your bet back">Forfeit (0.9x)</button>
        <button data-id="continue" title="Continue the game with original bet">Continue (1x)</button>
        <button data-id="double" title="Continue the game, but double the bet">Double (2x)</button>
    </div>
    <button id="deal">Deal</button>
</div>

\$\endgroup\$
  • 3
    \$\begingroup\$ Nice question! Are you aware that SE has a built in fiddler? It'd be pretty cool to embed the game right here. \$\endgroup\$ – RubberDuck Dec 20 '15 at 18:31
  • 1
    \$\begingroup\$ @RubberDuck Thanks! I knew that, but then I would have to adapt the page size. I haven't given design much thought (which you can probably tell LOL). It's literally just a bunch of boxes. \$\endgroup\$ – MortenMoulder Dec 20 '15 at 18:32
  • 1
    \$\begingroup\$ That's cool. I'm just a fan of playing games when I should be reviewing code. =;)- \$\endgroup\$ – RubberDuck Dec 20 '15 at 18:33
  • \$\begingroup\$ @RubberDuck You can do that through the JSFiddle I provided ;) \$\endgroup\$ – MortenMoulder Dec 20 '15 at 18:36
10
\$\begingroup\$

Pretty nice - kudos.

If I were to point out some things - which of course I am, this being a review - it'd be your prototype/class structure.

You have a Card constructor, which is cool. But you also have PlayerCard and DealerCard constructors/prototypes. But they're problematic:

  • They don't actually derive from Card, though that'd make sense.
  • They repeat each other quite a lot.
  • They're really just functions in disguise.

If you call new PlayerCard() you just get an empty object, really. It's not really a "card", just a container for a toString method, which reaches into a global array to find something to return.

So, in essence, you can just as well call PlayerCard.prototype.toString() directly, when you want a card.

It'd make a lot more sense to only have the Card class, since a card is the same regardless of who's holding it. So I'd do:

function Card(value, name, suit) {
    this.value = value;
    this.name = name;
    this.suit = suit;
}

Card.prototype = {
  toString: function () {
    return this.suit + " " + this.name;
  }
};

Now, to keep track of multiple cards - i.e. the deck - I'd expand the Deck constructor a little. Namely, I'd add shuffle as prototype method, so you can call deck.shuffle(), and I'd add a deal() method too, which returns and removes a card from the deck. Or you can have deal pick a card at random to remove and return, negating the need for a separate shuffle call. (In fact, you're already doing something like that in selectCards even though you're also shuffling the deck.)

With such changes, you'll model the domain a lot more closely. And skip a bunch of code, too.

Incidentally, the "ace is 1 but is really 14"-hack has a fairly simple solution: Change your names array (typically called "ranks" when talking about playing cards) to the order you're actually using:

['2', '3', '4', '5', '6', '7', '8', '9', '10', 'Jack', 'Queen', 'King', 'Ace']

Eller, på dansk:

['2', '3', '4', '5', '6', '7', '8', '9', '10', 'knægt', 'dame', 'konge', 'es']

Now an ace will automatically have the highest value, because that's the order you've defined.

On another note, don't do things like this:

dealerAmountLeft = 26;
//...
dealerAmountLeft--;

You have arrays which know their own length; ask them how many cards are left. Besides, even if you had to manually keep track of it (which you don't), there's no reason to keep track of two separate values, since - in this variant of the game - both players will always have the same number of cards.

Oh, and "amount" is bad name for "number of cards" - especially when you're also dealing with bets and "amount" is typically used to describe money.

In terms of rules, war is usually played without bets, and the winner of each round just "wins" the cards on the table. Hence the reason to put down extra cards in case of a war: More to lose/win. But your implementation of 3 extra cards all being used to determine the outcome is a little strange.

Especially as you're not checking if all those cards happen to be equal too. Haven't checked, but I think everything just grinds to a halt if a war ends in a stalemate. Edit: No, it doesn't; it's just war-time, all the time. But it does grind to a halt if there aren't enough cards to wage war. It tries to draw 3, but runs out.

The more common way to implement things (doubly so when cards are not also treated as currency), is simply to keep drawing cards as long as there's a stalemate.

\$\endgroup\$
  • \$\begingroup\$ Whoa thanks a lot! What I was really interested in reading, was expanding to using prototypes. Prototypes are new to me, which is why I haven't thought of using them for this. Expanding to that as well as adding the Ace to the last would make it pretty good. I only added Ace first, simply because it looks better. Do you think there is a possible way to make it more "centralized"? We have a player. We have a dealer. Everything they do is basically the same, however, I keep repeating myself. What do you think? Thanks for the review! \$\endgroup\$ – MortenMoulder Dec 20 '15 at 21:00
  • 1
    \$\begingroup\$ @Snorlax Indeed the player and dealer are pretty much the same. But they're still different in that one requires user interaction, and the other is automatic; one has a limited amount of money, and the other can (presumably) match any bet. But what they have in common are their hands. If you assume that a "hand" is always 1 or more cards, you can abstract that concept into a "class". For both sides, you draw a card from the deck, and add it to the hand. Then you can compare the two hands on a higher level, as they encapsulate what's on the table. That might be a place to start. \$\endgroup\$ – Flambino Dec 20 '15 at 21:11
  • \$\begingroup\$ Alright that sounds good! What do you think about the approach of starting the war game? Doing the interval, checking the winner, etc. \$\endgroup\$ – MortenMoulder Dec 20 '15 at 21:21
  • 1
    \$\begingroup\$ @Snorlax Well, I can tell you that there's a bug :) If there aren't enough cards to wage war, you just get an error. Hence why I think it might be nicer to implement this as a "draw one card at a time as long as the players are tied". It's simple and lends itself to streamlined code. \$\endgroup\$ – Flambino Dec 20 '15 at 21:34
  • 1
    \$\begingroup\$ @Snorlax Sure, it's ok. There are many way to tackle it, but this one works. I'm a little unsure about the interval. That's really a UX concern: It's more dramatic. But the code can figure out the winner immediately. So tying the code to the timer is a little iffy (not bad, just... iffy). I'd perhaps rather just figure out the winner, and then handle UI/UX stuff separately. But that's a bigger rewrite. \$\endgroup\$ – Flambino Dec 20 '15 at 21:42
5
\$\begingroup\$

I'm gonna start with some comments you knew about, referenced by the comments:

[On the subject of playing the game through the StackSnippet]

I knew that, but then I would have to adapt the page size. I haven't given design much thought (which you can probably tell. It's literally just a bunch of boxes.

You're really right. If someone sent me the fullscreen fiddle link, I'd ask what I'm playing.

Not to mention, it doesn't even fit (for me) on the fiddle link entirely, I have to scroll down.

Instead of using fixed width, use percentages or media queries.

On the subject of your actual design, and not the size of the page:

You could redo it a little better, for example, with a cards table placed in the background, and you deal the cards onto it. Or something similar.


Code structure:

Instead of having loose functions and globals, consider using the prototype chain.

This lets you package all the variables and functions inside a parent object so they can be called without having to pass parameters, etc.


Bug:

You have a bug in Line 170:

warStated = false;

That should be warStarted


jQuery:

Every function you use can be easily replaced with a vanilla equivalent.

The performance benefits of using jQuery are far outweighed by the strain on resources caused by loading it.


Language:

You offer different languages, but support no way to change them:

A button in the header would be excellent.


\$\endgroup\$
  • \$\begingroup\$ I appreciate your comment. The actual code is what I wanted someone to look at. As I said in my post, many things are left to be finished later on. The page styling is simply temporary, which I should have stated. If it works, it works. What do you mean about loose functions and globals? Can you give me an example of how I could change some of my code into the prototype chain perhaps? I did not notice that bug, thanks for that. I know jQuery is simply easier, which is why I went with it. I will never go vanilla when jQuery can do the same thing. It's a game. One static page. \$\endgroup\$ – MortenMoulder Dec 20 '15 at 20:41

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.