Monopoly game cards

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'

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:

• Grab a random card. Now that we have an array, we can simply shuffle it, and grab an element.

• Determine the type of card. We can use a dictionary approach, instead of multiple if..else.

• 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.

StackExchange Code Review Q#39831, answer score: 14