3
\$\begingroup\$

I'm working on a game of War for a potential employer and I was hoping someone could review my code and provide feedback on my code.

HTML/CSS

<!doctype html>
<html lang="en">
    <head>
        <title></title>
        <style>
            .card{
                position: relative;
                float: left;
                width: 350px;
                height: 500px;
                text-align: center;
            }

            .wonDeck{
                position: absolute;
                bottom: 0;
                width: 75px;
                height: 100px;
                text-align: center;
            }

            .card{
                margin: 0 5%;
            }

            .card:first-of-type{
                margin-left: 0;
            }

            .card:last-of-type{
                margin-right: 0;
            }

            .card .text{
                position: absolute;
                margin: 0 0 0 -25%;
                left: 35%;
                height: 30%;
                width: 50%;
                font-size: 26px;
                color: rgb(150, 150, 150);
            }

            .wonDeck .text{
                position: absolute;
                margin: 0 0 0 -25%;
                left: 50%;
                height: 30%;
                width: 50%;
                font-size: 16px;
                color: rgb(150, 150, 150);
            }

            .warDeck .text{
                position: absolute;
                margin: 0 0 0 -25%;
                left: 50%;
                height: 30%;
                width: 50%;
                font-size: 16px;
                color: rgb(150, 150, 150);
            }

            .cardHolder{
                position: absolute;
                top: 0;
                left: 0;
                width: 75%;
                height: 350px;
                font-size: 26px;
                color: rgb(0, 0, 0);
                border: 1px dashed black;
                background-color: rgba(0, 0, 0, 0.5);
            }

            .wonCardsHolder, .warCardsHolder{
                position: absolute;
                top: 0;
                left: 0;
                width: 100%;
                height: 100%;
                font-size: 16px;
                color: rgb(0, 0, 0);
                border: 1px dashed black;
                background-color: rgba(0, 0, 0, 0.5);
            }


            .warDeck{
                position: relative;
                float: right;
                width: 75px;
                height: 100px;
            }

            #reshuffle{
                display: none;
            }
        </style>
    </head>
    <body>
        <div id="player1CurrentCard" class="card">
            <div class="warDeck">
                <div class="warCardsHolder"></div>
                <div class="text">
                    <p>War Deck</p>
                </div>
            </div>
            <div class="cardHolder"></div>

            <div class="text">
                <p>Player 1 Card</p>
            </div>
            <div id="player1WonDeck" class="wonDeck">
                <div class="wonCardsHolder"></div>
                <div class="text">
                    <p>Won Deck</p>
                </div>
            </div>
        </div>

        <div id="player2CurrentCard" class="card">
            <div class="cardHolder"></div>
            <div class="warDeck">
                <div class="warCardsHolder"></div>
                <div class="text">
                    <p>War Deck</p>
                </div>
            </div>
            <div class="text">
                <p>Player 2 Card</p>
            </div>
            <div id="player2WonDeck" class="wonDeck">
                <div class="wonCardsHolder"></div>
                <div class="text">
                    <p>Won Deck</p>
                </div>
            </div>
        </div>
        <button id="play">Play</button>
        <button id="reshuffle">Reshuffle</button>
    </body>
    <script src="war.js"></script>
</html>

JS

var play = document.getElementById("play");
var reshuffle = document.getElementById("reshuffle");
var cardHolder = document.getElementsByClassName("cardHolder");
var wonCardsHolder = document.getElementsByClassName("wonCardsHolder");
var warCardsHolder = document.getElementsByClassName("warCardsHolder");

play.onclick = function(){
    PlayGame(Player1, Player2);
}
Player = function(name, currentDeck, wonDeck){
    this.name = name;
    this.currentDeck = currentDeck;
    this.wonDeck = wonDeck;
}
Player.prototype.GetCurrentCard = function(){
    this.currentCard = this.currentDeck.shift();
}
Player.prototype.isReshuffling = false;
Player.prototype.isAtWar = false;
Deck = {
    //suits: ["Clubs", "Diamonds", "Hearts", "Spades"],
    suits: ["Spades", "Clubs"],
    cards: [14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2],
    deck: [],
    shuffledDeck: [],
    BuildDeck: function(){
        for(var suit = 0; suit < this.suits.length; suit++){
            for(var card = 0; card < this.cards.length; card++){
                this.deck.push([this.cards[card], this.suits[suit]]);
            }
        }
    },
    ShuffleDeck: function(){
        while(this.deck.length){
            var index = Math.floor(Math.random() * this.deck.length); 
            this.shuffledDeck.push(this.deck.splice(index, 1)[0]);    
        }
    },
    DistributeCards: function(player1Deck, player2Deck){
        for(var i = 0; i < this.shuffledDeck.length / 2; i++){
            player1Deck.push(this.shuffledDeck[i]);
            player2Deck.push(this.shuffledDeck[this.shuffledDeck.length - i - 1]);
        }
    },
    ReshuffleDeck: function(player){
        while(player.wonDeck.length){
            var index = Math.floor(Math.random() * player.wonDeck.length);
            player.currentDeck.push(player.wonDeck.splice(index, 1)[0]);
        }
        player.wonDeck = [];
    }
}
PlayGame = function(player1, player2){
    if(player1.currentDeck.length == 0){
        player1.isReshuffling = true;
    }
    if(player2.currentDeck.length == 0){
        player2.isReshuffling = true;
    }
    GoToWar = function(){
        console.log("War");
        var player1WarDeck = [];
        var player2WarDeck = [];
        player1WarDeck.push(player1.currentCard);
        player1.GetCurrentCard();
        player1WarDeck.push(player1.currentCard);
        player1.GetCurrentCard();
        player1WarDeck.push(player1.currentCard);

        player2WarDeck.push(player2.currentCard);
        player2.GetCurrentCard();
        player2WarDeck.push(player2.currentCard);
        player2.GetCurrentCard();
        player2WarDeck.push(player2.currentCard);

        if(player1WarDeck[player1WarDeck.length - 1][0] == player2WarDeck[player2WarDeck.length - 1][0]){
            console.log("Tie");
            player1.GetCurrentCard();
            player1WarDeck.push(player1.currentCard);
            player1.GetCurrentCard();
            player1WarDeck.push(player1.currentCard);

            player2.GetCurrentCard();
            player2WarDeck.push(player2.currentCard);
            player2.GetCurrentCard();
            player2WarDeck.push(player2.currentCard);
        }
        if(player1WarDeck[player1WarDeck.length - 1][0] > player2WarDeck[player2WarDeck.length - 1][0]){
            player1.wonDeck = player1.wonDeck.concat(player1WarDeck, player2WarDeck);
            console.log("Player 1 wins");
        }
        else{
            player2.wonDeck = player2.wonDeck.concat(player1WarDeck, player2WarDeck);
            console.log("Player 2 wins");
        }
        console.log(player1WarDeck, player2WarDeck)
        warCardsHolder[0].textContent = player1WarDeck[player1WarDeck.length - 1][0] +" of " +player1WarDeck[player1WarDeck.length - 1][1];
        warCardsHolder[1].textContent = player2WarDeck[player2WarDeck.length - 1][0] +" of " +player2WarDeck[player2WarDeck.length - 1][1];
        cardHolder[0].textContent = player1WarDeck[0][0] +" of " +player1WarDeck[0][1];
        cardHolder[1].textContent = player2WarDeck[0][0] +" of " +player2WarDeck[0][1];
    }
    if(!player1.isReshuffling && !player2.isReshuffling){
        player1.GetCurrentCard();
        player2.GetCurrentCard();
        if(player1.currentCard[0] == player2.currentCard[0]){
            player1.isAtWar = true;
            player2.isAtWar = true;
        }
        else if(player1.currentCard[0] > player2.currentCard[0]){
            player1.wonDeck.push(player1.currentCard);
            player1.wonDeck.push(player2.currentCard);
            player1.isAtWar = false;
        }
        else{
            player2.wonDeck.push(player2.currentCard);
            player2.wonDeck.push(player1.currentCard);
            player2.isAtWar = false;
        }
        if(player1.isAtWar && player2.isAtWar){
            GoToWar();
        }
        else{
            cardHolder[0].textContent = player1.currentCard[0] +" of " +player1.currentCard[1];
            cardHolder[1].textContent = player2.currentCard[0] +" of " +player2.currentCard[1];
            warCardsHolder[0].textContent = "";
            warCardsHolder[1].textContent = "";
        }
        wonCardsHolder[0].textContent = player1.wonDeck.length;
        wonCardsHolder[1].textContent = player2.wonDeck.length;
    }
    if(player1.isReshuffling){
        ReshuffleDeck(player1);
        player1.isReshuffling = false;
    }
    if(player2.isReshuffling){
        ReshuffleDeck(player2);
        player2.isReshuffling = false;
    }
    if(player1.currentDeck.length == 26){
        GameOver(player1);
    }
    if(player2.currentDeck.length == 26){
        GameOver(player2);
    }
}
ReshuffleDeck = function(player){
    Deck.ReshuffleDeck(player)
}
GameOver = function(player){
    console.log(player.name +" wins!");
}
Player1 = new Player("Player 1", [], []);
Player2 = new Player("Player 2", [], []);
Deck.BuildDeck();
Deck.ShuffleDeck();
Deck.DistributeCards(Player1.currentDeck, Player2.currentDeck);

Fiddle

Pen

Updated Question with implemented feedback

Final Submission

Improve this question
\$\endgroup\$
0
4
\$\begingroup\$

Thoughts on HTML/CSS:

  • Put CSS into it's own file.
  • Consolidate CSS declarations - For example - don't have multiple definitions for .card, remove common attributes for .* .text into .text an only have your .* .text CSS reflect incremental differences, same with .cardHolder and similar classes, etc.
  • Lose one level of indentation on HTML. Commonly, you will not see indentation maintained for content between <html> and </html>, so as to eliminate extra indent for every line in between.

Now on to javascript, where most of my feedback relates:

Don't cram all your game logic into one PlayGame() function. You have way too much going in this this function - game setup, cards being dealt for individual turns, card deck being shuffled, win/loss resolution, etc.

Get your javascript out of global scope - for example into IIFE triggered in page ready event.

Why do you need two shuffling functions? You should consider having a shuffle function that operates against whatever deck/hand is passed to it.

Consider moving your javascript classes/functions into separate files than the javascript that actually performs logic on the page to bind game play to DOM elements.

Why do you only have two suits of cards? Why numbered 2-14? As opposed to common J,Q,K,Ace nomenclatures? This looks to me like someone who does not have forethought to move between numerical values, that may be needed for calculation/comparison to values that are meaningful to the user.

I find it hard to read to have this code:

play.onclick = function(){
    PlayGame(Player1, Player2);
}

before the code where the player variables are defined in scope. Also, Player1 and Player2 should probably have lower case p at begining which is standard javascript variable syntax.

    player1WarDeck.push(player1.currentCard);
    player1.GetCurrentCard();
    player1WarDeck.push(player1.currentCard);
    player1.GetCurrentCard();
    player1WarDeck.push(player1.currentCard);

This sort of code is overly repetitive. Consider a method that can draw X number of cards at a time. Your Deck class should probably contain all the methods related to dealing cards.

Wouldn't this look a lot nicer?

player1WarDeck.concat( player1.deck.deal(3) );

I am a little confused by where you attach some of your behaviors from an object-oriented design standpoint. Why would a player have currentCard or isResfhuffing properties? I would think those would be properties the deck held by the player.

Why are suits and values properties of a Deck, as opposed to properties of cards in a deck?

Why just have an array represent the user's different decks as opposed to Deck objects (which could then be used to shuffle the smaller decks)?

I know this might sound like a nitpick, but as a hiring manager, I want to see someone who understands that when you try to model objects in code along real-world lines, you tend to get the best results.

Deck.BuildDeck();
Deck.ShuffleDeck();
Deck.DistributeCards(Player1.currentDeck, Player2.currentDeck);

Why do you need three different method calls to start this game? The caller should be able to just do something like:

Deck.startGame(player1, player2);

With all the intermediate methods calls being abstracted away inside the Deck class. By the way, all your method names should start with lowercase letter.

ReshuffleDeck = function(player){
    Deck.ReshuffleDeck(player)
}
GameOver = function(player){
    console.log(player.name +" wins!");
}

Why are these globally scoped functions? When should you ever need to call these from global scope vs, having these methods calls be made from within the classes that control game action.

You are using inconsistent syntax to declare your classes:

Player = function(name, currentDeck, wonDeck){

vs.

Deck = {

Not sure you really need your isReshuffling and isAtWar properties. These seem like contrived properties on the players that were added because you were struggling with flow control within your overly complex PlayGame function.

I would like to see you adopt the use of strict comparisons (===, !==) as default coding behavior. Especially in loosely type languages like javascript, you can often introduce fragility to your code by making loose comparisons like this that can trigger on unexpected truthy/falsey values. There are cases where loose comparisons make sense, but they should be the exception rather than the norm.

Improve this answer
\$\endgroup\$
0
2
\$\begingroup\$

It seems to be functional, but very limited moving forward. For example, I believe your cards should be objects (especially since this seems to be an object-oriented challenge) so that you can determine face value, suit, card text, maybe an image or class associated with that card, etc.

For example:

function Card (options) {
  this.suit = options.suit;
  this.faceValue = options.faceValue;
  this.cardText = (function () {
    switch(this.faceValue) {
      case 14: { return "Ace"; }
      case 13: { return "King"; }
      case 12: { return "Queen"; }
      case 11: { return "Jack";}
      default: { return String(this.faceValue); }
    }
  }).call(this);
  this.imageSrc = (function () {
    return this.cardText.toLowerCase() + "of" + this.suit.toLowerCase() + ".jpg";
  }).call(this); 
}

Then you can fill your deck like so: 

for(var suit = 0; suit < this.suits.length; suit++){
  for(var card = 0; card < this.cards.length; card++){
    this.deck.push(new Card({suit: this.suits[suit], faceValue: this.cards[card]}));
  }
}

In addition, you have several global variables being assigned to anonymous functions, and (unless you have a good reason for doing so) they really should be just plain functions.

Example:

Player = function(name, currentDeck, wonDeck){
    // ...
}

Change to...

function Player(name, currentDeck, wonDeck) {
    // ...
}

This has to do with compile-time processes and lexical scoping issues due do the effect of "hoisting".

I also noticed that your logic for "going to war" seemed a bit strange as I remembered the game from when I played it years ago. Upon looking up the rules, in the event of a tie during war, the war process repeats until there is a winner. Your code, as it stands, only handles one tie, and in the event of a second tie, gives the cards to player2. You should have a recursive function that handles the "Wars" to ensure accurate results.

Improve this answer
\$\endgroup\$
0

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.