3
\$\begingroup\$

This will be my last submission of this project for review as it's being submitted this evening. I have updated my code based on feedback from other users. I would appreciate any and all feedback from the community. Thanks in advance.

Original submission

Follow Up

HTML

<!doctype html>
<html>
    <head>
        <title></title>
        <link rel="stylesheet" type="text/css" href="styles.css">
    </head>
    <body>
        <div id="player1CurrentCard" class="card">
            <div class="warDeck">
                <div class="warCardsHolder">
                    <div class="text">
                        <p>War Card</p>
                    </div>
                </div>
                <div class="warCardsHolder">
                    <div class="text">
                        <p>War Card</p>
                    </div>
                </div>
            </div>
            <div class="cardHolder"></div>
            <div class="text">
                <p>Player 1 Card</p>
            </div>
            <div id="player1CurrentDeck" class="currentDeck">
                <div class="currentCardsHolder"></div>
                <div class="text">
                    <p>Current Deck</p>
                </div>
            </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 class="text">
                        <p>War Card</p>
                    </div>
                </div>
                <div class="warCardsHolder">
                    <div class="text">
                        <p>War Card</p>
                    </div>
                </div>
            </div>
            <div class="text">
                <p>Player 2 Card</p>
            </div>
            <div id="player2CurrentDeck" class="currentDeck">
                <div class="currentCardsHolder"></div>
                <div class="text">
                    <p>Current Deck</p>
                </div>
            </div>
            <div id="player2WonDeck" class="wonDeck">
                <div class="wonCardsHolder"></div>
                <div class="text">
                    <p>Won Deck</p>
                </div>
            </div>
        </div>
        <button id="dealCards">Deal Cards</button>
        <script src="scripts/classes.js"></script>
        <script src="scripts/main.js"></script>
        <script src="scripts/unitTests.js"></script>
    </body>
</html>

**CSS*

.card{
    position: relative;
    float: left;
    width: 350px;
    height: 500px;
    text-align: center;
}

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

.currentDeck{
    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, .currentDeck .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: 15% 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, .currentCardsHolder{
    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);
}

.warCardsHolder:nth-of-type(2), .warCardsHolder:last-of-type{
    top: 125px;
}

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

main.js

var dealCards = document.getElementById("dealCards");
var cardHolder = document.getElementsByClassName("cardHolder");
var currentCardsHolder = document.getElementsByClassName("currentCardsHolder");
var wonCardsHolder = document.getElementsByClassName("wonCardsHolder");
var warCardsHolder = document.getElementsByClassName("warCardsHolder");

var Player1 = new Player("Player 1");
var Player2 = new Player("Player 2");
Deck.StartGame(Player1, Player2);
function GameLogic(player1, player2){
    if((player1.wonDeck.length === 0) && (player1.currentDeck.length === 0)){
        player1.GameOver(player2);
        Player1Wins(player1);
    }
    else if((player2.wonDeck.length === 0) && (player2.currentDeck.length === 0)){
        player2.GameOver(player1);
        Player2Wins(player2);
    }
    else{
        if(player1.currentDeck.length === 0){
            Deck.ShuffleDeck(player1.wonDeck, player1.currentDeck);
        }
        if(player2.currentDeck.length === 0){
            Deck.ShuffleDeck(player2.wonDeck, player2.currentDeck);
        }
        player1.GetCurrentCard();
        player2.GetCurrentCard();
        warCardsHolder[1].textContent = "";
        warCardsHolder[3].textContent = "";
        cardHolder[0].textContent = player1.currentCard.cardText +" of " +player1.currentCard.suit;
        cardHolder[1].textContent = player2.currentCard.cardText +" of " +player2.currentCard.suit;
        currentCardsHolder[0].textContent = player1.currentDeck.length;
        currentCardsHolder[1].textContent = player2.currentDeck.length;
        wonCardsHolder[0].textContent = player1.wonDeck.length;
        wonCardsHolder[1].textContent = player2.wonDeck.length;
        if(player1.currentCard.faceValue === player2.currentCard.faceValue){
            player1.warDeck.push(player1.currentCard);
            player2.warDeck.push(player2.currentCard);
            player1.GoToWar(player2);
        }
        else{
            if(player1.currentCard.faceValue > player2.currentCard.faceValue){
                player1.wonDeck.push(player1.currentCard);
                player1.wonDeck.push(player2.currentCard);
            }
            else{
                player2.wonDeck.push(player1.currentCard);
                player2.wonDeck.push(player2.currentCard);
            }
        }
    }
}
function PlayerWins(player){
    if(player.name === "Player 1"){
        warCardsHolder[1].textContent = "";
        warCardsHolder[3].textContent = "";
        cardHolder[0].textContent = "";
        cardHolder[1].textContent = "";
        currentCardsHolder[0].textContent = player.currentDeck.length;
        currentCardsHolder[1].textContent = "";
        wonCardsHolder[0].textContent = player.wonDeck.length;
        wonCardsHolder[1].textContent = "";
    }
    else{
        warCardsHolder[1].textContent = "";
        warCardsHolder[3].textContent = "";
        cardHolder[0].textContent = "";
        cardHolder[1].textContent = "";
        currentCardsHolder[0].textContent = "";
        currentCardsHolder[1].textContent = player.currentDeck.length;
        wonCardsHolder[0].textContent = "";
        wonCardsHolder[1].textContent = player.wonDeck.length;
    }
    dealCards.disabled = true;
}
dealCards.onclick = function(){
    GameLogic(Player1, Player2);
}

classes.js

function Player(name, currentDeck, wonDeck, warDeck){
    this.name = name;
    this.currentDeck = currentDeck !== undefined && currentDeck instanceof Array ? currentDeck : [];
    this.wonDeck = wonDeck !== undefined && wonDeck instanceof Array ? wonDeck: [];
    this.warDeck = warDeck !== undefined && warDeck instanceof Array ? warDeck: [];
}
Player.prototype.GetCurrentCard = function(){
    this.currentCard = this.currentDeck.pop();
}
Player.prototype.GoToWar = function(opponent){
    var tied = false;
    console.log("War");
    do{
        this.CanContinueWar(opponent);
        opponent.CanContinueWar(this);
        if(this.warDeck.length !== 0){
            warCardsHolder[1].textContent = this.warDeck[this.warDeck.length - 1].cardText +" of " +this.warDeck[this.warDeck.length - 1].suit;
        }
        if(opponent.warDeck.length !== 0){
            warCardsHolder[3].textContent = opponent.warDeck[opponent.warDeck.length - 1].cardText +" of " +opponent.warDeck[opponent.warDeck.length - 1].suit;
        }
        currentCardsHolder[0].textContent = this.currentDeck.length;
        currentCardsHolder[1].textContent = opponent.currentDeck.length;
        wonCardsHolder[0].textContent = this.wonDeck.length;
        wonCardsHolder[1].textContent = opponent.wonDeck.length;
        if(this.warDeck[this.warDeck.length - 1].faceValue === opponent.warDeck[opponent.warDeck.length - 1].faceValue){
            tied = true;
        }
        console.log("Tied");
    }
    while(tied === true);       
    if(this.warDeck[this.warDeck.length - 1].faceValue > opponent.warDeck[opponent.warDeck.length - 1].faceValue){
        console.log(this.name +" wins war");
        this.WonWar(opponent);
    }
    else{
        console.log(opponent.name +" wins war");
        opponent.WonWar(this);
    }
}
Player.prototype.CanContinueWar = function(opponent){
    Deck.DealWarCards(this, 2);
    if(this.currentDeck.length < 2){
        if(this.wonDeck.length < 2){
            if((this.currentDeck.length === 1) && (this.wonDeck.length === 1)){
                console.log(this.name +" continuing war");
                Deck.ShuffleDeck(this.wonDeck, this.currentDeck);
            }
            else if((this.currentDeck.length === 0) && (this.wonDeck.length === 1)){
                console.log(this.name +" loses war");
                Deck.ShuffleDeck(this.wonDeck, opponent.wonDeck);
                opponent.WonWar(this);
                this.GameOver(opponent);
            }
            else if((this.currentDeck.length === 1) && (this.wonDeck.length === 0)){
                console.log(this.name +" loses war");
                Deck.ShuffleDeck(this.currentDeck, opponent.wonDeck);
                opponent.WonWar(this);
                this.GameOver(opponent);
            }
            else{
                console.log(this.name +" loses");
                opponent.WonWar(this);
                this.GameOver(opponent);
            }
        }
        Deck.ShuffleDeck(this.wonDeck, this.currentDeck);
    }
}
Player.prototype.WonWar = function(opponent){
    this.wonDeck = this.wonDeck.concat(this.warDeck, opponent.warDeck);
    this.warDeck = [];
    opponent.warDeck = [];
}
Player.prototype.GameOver = function(opponent){
    console.log(opponent.name +" wins!");
    opponent.wonDeck = opponent.wonDeck.concat(opponent.warDeck, this.currentDeck, this.wonDeck, this.warDeck);
    opponent.warDeck = [];
    this.currentDeck = [];
    this.wonDeck = [];
    this.warDeck = [];
    PlayerWins(opponent);
}
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)};
            break;
        }
    }).call(this);
}
Deck = {
    suits: ["Clubs", "Diamonds", "Hearts", "Spades"],
    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(new Card({suit: this.suits[suit], faceValue: this.cards[card]}));
            }
        }
    },
    ShuffleDeck: function(unshuffledDeck, shuffledDeck){
        while(unshuffledDeck.length){
            var index = Math.floor(Math.random() * unshuffledDeck.length); 
            shuffledDeck.push(unshuffledDeck.splice(index, 1)[0]);    
        }
        unshuffledDeck = [];
    },
    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]);
        }
    },
    DealWarCards: function(player, num){
        for(var i = 0; i < num; i++){
            player.GetCurrentCard();
            player.warDeck.push(player.currentCard);
        }
        console.log(player);
    },
    StartGame: function(player1, player2){
        this.BuildDeck();
        this.ShuffleDeck(this.deck, this.shuffledDeck);
        this.DistributeCards(player1.currentDeck, player2.currentDeck);
    }
}
\$\endgroup\$
2
\$\begingroup\$

The one thing I find hard to deal with when it comes to OOP is how tied up the logic is to objects, and objects are to objects. For instance, why does the Deck class aware about the players. Shouldn't it just be dealing with cards?

Another problem with this is testability. Say you encountered a bug mid-game and wanted to reproduce it. How do you go about extracting that information from the game? How do you reach that state where the bug occurred? How do you test this code?

Card games are like state machines. They have a definite state at any point in time, and actions players take mutate that state. You can simply define the state structure like so:

const game = {
  phase: 'start', // start, in-game, end, etc.
  winner: null,
  deck: [],
  players: [
    { name: 'player1', wonDeck: [], warDeck: [] },
    { name: 'player2', wonDeck: [], warDeck: [] },
  ]
}

From here, you can tell from looking at the data what cards are in the deck, how many players there are, who are the players, what cards they have. You can easily serialize this data structure, use it for logging, debugging, reporting.

Now let's take a state transition. For instance, shuffling the deck would be like:

const previousState = {
  phase: 'start', // start, in-game, end, etc.
  winner: null,
  deck: [ ...values... ],
  players: [
    { name: 'player1', hand: [/* empty hand */] },
    { name: 'player2', hand: [/* empty hand */] },
  ]
}

function shuffleDeck(state){
  return {
    // Copy everything over
    ...state,
    // But override deck
    deck: state.deck.reduce((shuffled, card) => {
      // For each card in the previous state's deck, insert them in random
      // positions in the array and return that array.
      shuffled.splice(Math.floor(Math.random() * shuffled.length), 0, card);
      return shuffled;
    }, []);
  }
}

const newState = shuffleDeck(state);

As you can see, shuffling the deck is a new state constructed from the old state, with the deck modified. From a testability point of view, you can easily craft the old state from a bunch of objects and arrays, feed it to the function and you get your result. You do not need to create instances of Deck, of Player, no mocking, no stepping through, none of that voodoo. In fact, this is already a test case. What's lacking is the assertions.

Given this advice, the game would progress like:

  1. Generate initial state (a blank state like the first snippet)
  2. Wait for user input
  3. Mutate state (the second snippet)
  4. Render results of 3
  5. Repeat steps 2-4 until winner found

As for the rest of the code, there's... a few problems:

Your domain object (Player) and game logic should not be aware of the DOM. In MVC, only logic stays in the controller. Rendering is done in a view layer which should only receive already-processed data.

dealCards.onclick = function(){
  GameLogic(Player1, Player2);
}

Use addEventListener. Although onclick works, you're technically overriding the onclick property. Any previously added handlers would be replaced.

console.log(this.name +" loses war");

You should remove debugging code. Although it's understandable that this is a small app, it's still a bad habit. I've seen people years into web development that still make this simple mistake.

if(this.currentDeck.length < 2){
    if(this.wonDeck.length < 2){
        if((this.currentDeck.length === 1) && (this.wonDeck.length === 1)){

You have a lot of "magic numbers" in your code. What does 2 signify? What does 1 signify? Can I change this to 42? Why? Why not?. It is advised that you store these in properly named variables, named after their purpose.


Also you mentioned of a "potential employer" in the earlier revisions. If this employer is what I think it is, they'll be looking at code structure, maintainability and testability. These things you should look out for in your code.

Also, they'd want to reduce the time it takes to develop such apps. This starts with not repeating what other developers have already created. Consider venturing into using a framework, any will do. This will take out the hassle of DOM manipulation so that all you need to worry about is your data and logic. Vue.js is a good candidate for small and easy to start with.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your feedback and advice. If you don't mind me asking, who do you think the potential employer is? \$\endgroup\$ – Robert Feb 25 '17 at 16:06
  • \$\begingroup\$ @Robert What I was trying to say is that there are employers who care if you can write code, and there are those who care how you write code. If your employer is of the latter kind, you'll have to look beyond just making working code. \$\endgroup\$ – Joseph Feb 25 '17 at 19:55
  • \$\begingroup\$ I understood what you meant. \$\endgroup\$ – Robert Feb 25 '17 at 21:08

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.