14
\$\begingroup\$

This functionality is for a Monopoly board game. In particular, when the player lands on Chance or Community Chest, a random card is drawn with a particular set of instruction, a bonus could be paid out, or perhaps the player is fined. The two cards are different in that one is a set of 16 cards and the other a set of 14. How would you make this code more efficient?

Chance

function chanceCard() {
    var x = Math.floor(Math.random() * ((16 - 1) + 1) + 1);
    var title = chancecards['chance' + x].title;
    var type = chancecards['chance' + x].type;
    var bill = chancecards['chance' + x].bill;
    var bonus = chancecards['chance' + x].bonus;
    if (type == "bill") {
        updateBalance("-", bill);
    } else if (type == "bonus") {
        updateBalance("+", bonus);
    } else if (type == "move") {
        var newposition = chancecards['chance' + x].newposition;
        var currentposition = players[player].currentpos;
        if (newposition == 40) { //this if the player has to "advance to go"
            updateBalance("+", 200);
        } else if (newposition < currentposition) { //if the new position is less than the current one it means the player has to go past go
            updateBalance("+", 200);
        }
        players[player].prevpos = players[player].currentpos;
        players[player].currentpos = newposition;
        players[player].startpos = players[player].currentpos;

        movePiece();
        checkForSale();
    } else if (title == "Go back 3 spaces") {
        players[player].prevpos = players[player].currentpos;
        players[player].currentpos -= -3;
        players[player].startpos = players[player].currentpos;

        movePiece();
        checkForSale();

    }
    flipCard("Chance");

}

Community Chest

function chestCard() {
    var x = Math.floor(Math.random() * ((14 - 1) + 1) + 1);
    var title = chestcards['chest' + x].title;
    var chesttype = chestcards['chest' + x].type;
    var bill = chestcards['chest' + x].bill;
    var bonus = chestcards['chest' + x].bonus;
    if (chesttype == "bill") {
        updateBalance("-", bill);
    } else if (chesttype == "bonus") {
        updateBalance("+", bonus);
    } else if (chesttype == "move") {
        var newposition = chestcards['chest' + x].newposition;
        var currentposition = players[player].currentpos;
        if (newposition == 40) { //this if the player has to "advance to go"
            updateBalance("+", 200);
        } else if (newposition < currentposition) { //if the new position is less than the current one it means the player has to go past go
            updateBalance("+", 200);
        }
        players[player].prevpos = players[player].currentpos;
        players[player].currentpos = newposition;
        players[player].startpos = players[player].currentpos;

        movePiece();
        checkForSale();

    }

    flipCard("Community Chest");
}

CreateCards

function createCards() {
    chancecards = {
        chance1: {
            title: "Advance to go",
            type: "move",
            newposition: 40
        },
        chance2: {
            title: "Advance to London",
            type: "move",
            newposition: 39
        },
        chance4: {
            title: "Your ass is going to jail",
            type: "move",
            newposition: 10
        },
        chance9: {
            title: "Advance to Rome",
            type: "move",
            newposition: 24
        },
        chance10: {
            title: "Advance to Charles de Gaulle",
            type: "move",
            newposition: 15
        },
        chance11: {
            title: "Advance to Amsterdam",
            type: "move",
            newposition: 11
        },
        chance6: {
            title: "Go back 3 spaces",
            type: "movex",
            newposition: -3
        },
        chance14: {
            title: "No drink and driving mate1",
            type: "bill",
            bill: 20
        },
        chance15: {
            title: "Get out of Jail free card",
            type: "bill",
            bill: 150
        },
        chance7: {
            title: "Pay school fees",
            type: "bill",
            bill: 150
        },
        chance12: {
            title: "Speeding fine",
            type: "bill",
            bill: 150
        },
        chance5: {
            title: "Bank pays you dividend",
            type: "bonus",
            bonus: 40
        },
        chance13: {
            title: "You have won the competition",
            type: "bonus",
            bonus: 200
        },
        chance16: {
            title: "Your building loan matures",
            type: "bonus",
            bonus: 200
        },
        chance3: {
            title: "You are assessed for street repairs $40 per house $115 per hotel",
            type: "billx"
        },
        chance8: {
            title: "House repairs $25 per house $100 per hotel",
            type: "billx"
        }
    };
    chestcards = {
        chest1: {
            title: "Advance to go",
            type: "move",
            newposition: 40,
            bonus: 200
        },
        chest2: {
            title: "Advance to Cairo",
            type: "move",
            newposition: 1
        },
        chest3: {
            title: "Go to Jail",
            type: "move",
            newposition: 10
        },
        chest4: {
            title: "Pay hospital fees",
            type: "bill",
            bill: 100
        },
        chest5: {
            title: "Pay doctor fees",
            type: "bill",
            bill: 50
        },
        chest6: {
            title: "Pay insurance premium",
            type: "bill",
            bill: 50
        },
        chest7: {
            title: "Bank error. Collect $200",
            type: "bonus",
            bonus: 200
        },
        chest8: {
            title: "Annuity matures. Collect $100",
            type: "bonus",
            bonus: 100
        },
        chest9: {
            title: "You inherit $100",
            type: "bonus",
            bonus: 100
        },
        chest10: {
            title: "From sale of stock you get $50",
            type: "bonus",
            bonus: 50
        },
        chest11: {
            title: "Preference shares: $25",
            type: "bonus",
            bonus: 25
        },
        chest12: {
            title: "You have won second prize in a beauty contest. Collect $10.",
            type: "bonus",
            bonus: 10
        },
        chest13: {
            title: "It is your birthday. Collect $10.",
            type: "bonus",
            bonus: 10
        },
        chest14: {
            title: "You win the lottery. Collect $10",
            type: "bonus",
            bonus: 10
        }
    };
}
\$\endgroup\$
  • \$\begingroup\$ 1. chanceCards() , 2. chestCards() , 3. createCards(). I want to DRY 1 and 2. \$\endgroup\$ – Elton Frederik Jan 22 '14 at 18:09
  • \$\begingroup\$ I can help you out a bit but I want to stay DRY myself. Do you care if I write my answer using a lightweight framework? \$\endgroup\$ – pllee Jan 22 '14 at 20:12
  • \$\begingroup\$ @pllee no i don't mind. \$\endgroup\$ – Elton Frederik Jan 23 '14 at 14:49
  • \$\begingroup\$ @AlienArrays haha yes ofcourse. This is just for educational purpose, I think the best way to learn is to actually build stuff \$\endgroup\$ – Elton Frederik Jan 23 '14 at 14:50
  • 3
    \$\begingroup\$ Please don't make massive edits to your posts which invalidates answers. If you want to show the result, you can edit and add the text below your original question or post it as an answer stating that it is the result of the reviews. If you want more code reviewed, ask a new question. \$\endgroup\$ – Simon Forsberg Jan 30 '14 at 17:51
8
\$\begingroup\$

Card representation

I offer three important tips to improve the representation of the cards:

  1. The draw-a-card-from-the-deck functions should have nothing to do with updating balances, moving pieces, etc. (In technical terms, chanceCard() and chestCard() violate the Single Responsibility Principle.)
  2. JavaScript lets you store code as a kind of data. (In technical terms, "functions are first-class objects in JavaScript.")
  3. Giant switch blocks (like the if-elseif-elseif-… chain in chanceCard()) are a hint to use objects instead. (In technical terms, apply the "Replace Conditionals With Polymorphism" refactoring rule.)

These indicators suggest that the action for the cards should be encoded in the cards themselves. Since you don't want to write a function per card, though, you'll want to classify the cards into several types, and define the functions in their prototypes.

Note that "bill" and "bonus" cards are really just the same thing, except that bills have a negative impact on the player's balance and bonuses have a positive impact.

Code

function updateBalance(player, amount) {
    player.balance += amount;
}

function movePiece(player, direction, destination) {
    if (direction > 0 && destination != "jail" && destination < player.currentpos) {
        // Moving forward to or past Go, but not going to jail
        updateBalance(player, +200);
    }
    player.currentpos = destination;
    checkForSale();
}

function drawCard(player, deck) {
    var card = deck.shift();
    // console.log(card.title);
    card.act(player);
    deck.push(card);
}

function shuffle(deck) {
    // Fisher-Yates shuffle
    for (var i = deck.length - 1; i > 0; i--) {
        var j = Math.floor((i + 1) * Math.random());
        var swap = deck[i];
        deck[i] = deck[j];
        deck[j] = swap;
    }
}

//////////////////////// CARD TYPES ////////////////////////

function AbsMoveCard(title, destination) {
    this.title = title;
    this.destination = destination;
}

AbsMoveCard.prototype.act = function(player) {
    // All logic about current position, bonus for passing Go, and checking
    // for property sale should be included in movePiece().
    movePiece(player, +1, this.destination);
};

function RelMoveCard(title, distance) {
    this.title = title;
    this.distance = distance;
}

RelMoveCard.prototype.act = function(player) {
    // All logic about current position, bonus for passing Go, and checking
    // for property sale should be included in movePiece().
    movePiece(player, this.distance, (player.currentpos + 40 + this.distance) % 40);
}

function MoneyCard(title, amount) {
    this.title = title;
    this.amount = amount;
}

MoneyCard.prototype.act = function(player) {
    updateBalance(player, this.amount);
}

function AssessmentCard(title, perHouse, perHotel) {
    this.title = title;
    this.perHouse = perHouse;
    this.perHotel = perHotel;
}

AssessmentCard.prototype.act = function(player) {
    updateBalance(player, player.houseCount * this.perHouse +
                          player.hotelCount * this.perHotel);
}

/////////////////////////// CARDS ///////////////////////////

var chanceCards = [
    // "Go" should be position 0 rather than 40
    new AbsMoveCard("Advance to go", 0),
    new AbsMoveCard("Advance to London", 39),
    new AbsMoveCard("Your ass is going to jail", "jail"),
    new AbsMoveCard("Advance to Rome", 24),
    new AbsMoveCard("Advance to Charles de Gaulle", 15),
    new AbsMoveCard("Advance to Amsterdam", 11),
    new RelMoveCard("Go back 3 spaces", -3),
    new MoneyCard("No drink and driving mate!", -20),
    new MoneyCard("Get out of Jail free card", -150),
    new MoneyCard("Pay school fees", -150),
    new MoneyCard("Speeding fine", -150),
    new MoneyCard("Bank pays you dividend", +40),
    new MoneyCard("You have won the competition", +200),
    new MoneyCard("Your building loan matures", +200),
    new AssessmentCard("You are assessed for street repairs $40 per house $115 per hotel", -40, -115),
    new AssessmentCard("House repairs $25 per house $100 per hotel", -25, -100),
];

var chestCards = [
    new AbsMoveCard("Advance to go", 0),
    new AbsMoveCard("Advance to Cairo", 1),
    new AbsMoveCard("Go to Jail", "jail"),
    new MoneyCard("Pay hospital fees", -100),
    new MoneyCard("Pay doctor fees", -50),
    new MoneyCard("Pay insurance premium", -50),
    new MoneyCard("Bank error. Collect $200", +200),
    new MoneyCard("Annuity matures. Collect $100", +100),
    new MoneyCard("You inherit $100", +100),
    new MoneyCard("From sale of stock you get $50", +50),
    new MoneyCard("Preference shares: $25", +25),
    new MoneyCard("You have won second prize in a beauty contest. Collect $10.", +10),
    new MoneyCard("It is your birthday. Collect $10.", +10),
    new MoneyCard("You win the lottery. Collect $10", +10),
];

shuffle(chanceCards);
shuffle(chestCards);

Example usage

var player1 = { currentpos: 0, balance: 200, houseCount: 0, hotelCount: 0 };
drawCard(player1, chanceCards);

Additional remarks

It would be more natural to represent Go as position 0 rather than position 40. You could then use the modulo 40 operation to maintain the illusion of a cyclical board.

You designated board position 10 as the jail. However, I would prefer to think of board position 10 as the "Just Visiting" space rather than the "In Jail" portion. One or the other will need special treatment, and since the jail requires special handling anyway, it seems appropriate that it should be the exceptional case. Here, I've used the string "jail" to represent that position, but deeper consideration may be worthwhile.

Picking a random element from the array to doesn't accurately simulate a card draw in a board game. I think it would be more appropriate to shuffle the cards initially, draw from the "top" of the deck, and replace the cards at the "bottom" after use.

\$\endgroup\$
  • \$\begingroup\$ Wow I really appreciate the tips, but it is way beyond the scope of my understanding of JS, I am still a beginner. I will definitely refer back to your tips once I learned more. It would be nice if a collection of Stack contributors do a tutorial of the Monopoly board game. \$\endgroup\$ – Elton Frederik Jan 26 '14 at 13:30
  • \$\begingroup\$ Hi, could you show me how you would call the different prototype methods depending on the card picked? \$\endgroup\$ – Elton Frederik Jan 27 '14 at 22:50
  • \$\begingroup\$ The Example usage already does everything. You don't call the prototype methods explicitly — that's what makes it elegant. Setting AbsMoveCard.prototype.act = function() { … } just means that every object created using var card = new AbsMoveCard(…) automatically gets that act function attached to it. You can then invoke that function with card.act(player). Furthermore, you don't care what kind of card is drawn. Whatever card you pick, just call card.act(player) on it, and the card "knows" how to do the right thing. \$\endgroup\$ – 200_success Jan 27 '14 at 22:58
14
\$\begingroup\$

The first thing I would suggest to DRY your code, is to organize your cards in collections (arrays of objects), rather than nested objects. The reason is: arrays have order, and JavaScript has many built-in methods to work with collections, like sort, map, and many others:

var cards = {
  // Collection of chance cards
  chance: [
    {
      title: 'Advance to go',
      type: 'move',
      position: 40
    },
    {
      title: 'Advance to London',
      type: 'move',
      position: 39
    },
    {
      title: 'Your ass is going to jail',
      type: 'move',
      position: 10
    },
    // more
  ],
  // Collection of chest cards
  chest: [
    // chest cards
  ]
};

That way you can you get rid of chance1, chance2, chance3 and use array indices instead.

Now that you're working with a nicer data structure, let's rethink our steps, and what we can do to solve them:

  1. Grab a random card. Now that we have an array, we can simply shuffle it, and grab an element.
  2. Determine the type of card. We can use a dictionary approach, instead of multiple if..else.
  3. Update the game (balance, position, etc...)

I can't run this code obviously I'm missing many parts, but hopefully it'll give you an idea of how to simplify it and improve readability.

// Naive shuffle implementation
var shuffle = function(xs) {
  return xs.slice(0).sort(function() {
    return .5 - Math.random();
  });
};

function getCard(from) {
  var card = shuffle(cards[from]).pop(); // a random card

  // A dictionary
  var types = {
    bill: function() {
      updateBalance('-', card.bill);
    },
    bonus: function() {
      updateBalance('+', card.bonus);
    },
    move: function() {
      var current = players[player].currentpos;
      var player = players[player]; // cache for convenience

      // We can shorten this statment as you were doing
      // the same thing in both clauses
      if (card.position == 40 || card.position < current) {
        updateBalance("+", 200);
      }

      player.prevpos = player.currentpos;
      player.currentpos = card.position;
      player.startpos = player.currentpos;

      movePiece();
      checkForSale();
    },
    // "Go back 3 spaces" is the only one of type `movex`
    // so we can use that in the same way as the others
    // without the need to compare by string
    movex: function() {
      var player = players[player];
      player.prevpos = player.currentpos;
      player.currentpos -= -3;
      player.startpos = player.currentpos;

      movePiece();
      checkForSale();
    }
  };

  // Run the code for the type if it exists
  if (types[card.type]) {
    types[card.type]();
  }

  // To be able to use the type, and for consistency
  // modify `filpCard` so it takes the types as `chest` and `chance`
  flipCard(from);
}

But if you look up closesly there's a pattern that's still repeats itself a couple times:

player.prevpos = player.currentpos;
player.currentpos = card.position;
player.startpos = player.currentpos;

movePiece();
checkForSale();

This is related to point 3 (Update the game), it's harder to abstract. The main issue I see is that player is coupled to the function that creates the cards, and updateBalance also affects player but it's hidden away in the details. In an object oriented way you'd have Board, Player and Card classes, with their own methods and properties. Your current code as-is is hard to DRY up more. You could separate that repeating pattern into a function, but I'd argue that it will make your logic harder to follow.

\$\endgroup\$
  • 2
    \$\begingroup\$ Your last suggestion is really what needs to happen here. The effect of a card on a player should be one of the properties on card, so you'd have card.applyTo(player) with an applyTo method (for a specific example card) like function applyTo(player) { player.moveTo(HOME) }. Currently this key responsibility of a card has been moved elsewhere. "A George divided against himself cannot stand!" \$\endgroup\$ – Jonah Jan 22 '14 at 22:26
  • \$\begingroup\$ I agree, but at that point I'd rather build from scratch I suppose, in an object oriented way. \$\endgroup\$ – elclanrs Jan 22 '14 at 22:47
  • 1
    \$\begingroup\$ @elclanrs I went out to build this game without extensive planning, I am not very imaginative, but it is good to write bad code and then later realise why it is bad. I will finnish the game with bad code, just to get it working for now then. \$\endgroup\$ – Elton Frederik Jan 23 '14 at 14:57
  • \$\begingroup\$ I'd like to reemphasize your casual mention that shuffling an array by passing a non-deterministic comparator to .sort() is a naïve approach. It's a hack that can yield unsatisfactory results. \$\endgroup\$ – 200_success Jan 26 '14 at 7:37

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.