Online card game

I built an online card game last year. Initially I was proud of how tidy the code was, but recently I was thinking about how my code is almost entirely functions calling other functions.

It would be great to get feedback from other developers - does my code follow good practices? If it isn't following good coding practices, or it jumps around too much, please let me know what method/s I can use to write better code in the future.

```
'use strict';

document.getElementById('clear-cards').style.display = 'inline-block';

window.onbeforeunload = function() {
return "Are you sure you want to leave the game?";
};

var count = 0;
var lastPlayer = 'computer';

var humanCard;
var humanCardNumber;
var computerCard;
var computerCardNumber;

var humanCards = [];
var computerCards = [];

var human = {
population: 100,
foodGained: 0,
soldiersGained: 0,
soldiers: function soldiers() {
return Math.round((this.population / 10) + this.soldiersGained);
},
food: function food() {
return Math.round(this.population * 2 + this.foodGained);
}
};

var computer = {
population: 100,
foodGained: 0,
soldiersGained: 0,
soldiers: function soldiers() {
return Math.round((this.population / 10) + this.soldiersGained);
},
food: function food() {
return Math.round(this.population * 2 + this.foodGained);
}
};

// GET CARDS
function getCards(cards) {
while (cards.length \n\t\t\t';
j++;
}
}
document.getElementById('cards').style.display = 'inline-block';
};

// runs when user clicks on card
function playCard(type, number, index) {
var amount;
switch (type) {
case 'recruit':
human.soldiersGained += number;
break;
case 'ambush':
computer.soldiersGained -= number;
computer.population -= number;
break;
case 'rampage':
computer.foodGained -= computer.food() / 100 * number;

Initially I was proud of how tidy the code was, but recently I was thinking about how my code is almost entirely functions calling other functions.

Why the "but"? Code calling code is how programming works. If anything I'd say you're not doing enough of it. But I'll get to that.

Overall, what I'm seeing is a lot of repetition. Many of your functions have big switch statements - that structure scatters the game's logic all over the place. If you add a new type of card, you'll have to add it everywhere. And it's hard to get a sense of what an existing card does when it's effects are so scattered.

It's doubly repeated because you handle the user's and the computer's turns separately. As far as I can tell, they're both just players. Either side can play a card, which will affect them and/or their opponent. Doesn't really matter who's the human and who's the computer; from either perspective there's just the player and the opponent.

Looking at your getCards code I'm also wondering if it's what you want. Technically you're playing with an infinite deck. The different cards have different odds of being drawn, but each card type can appear an infinite number of times. Since the drawing is random, you could end up drawing the same exact card an infinite number of times. Unlikely, yes, but if this was a real card game, there'd be a deck, and even if all the cards were clustered together, at some point you would simply run out of one type, and start seeing another type. Not so here.

But whether you want to use a "deck" that you shuffle, or continue to just have the random chance of any card at any time is your call of course.

Anyway, I'd model the cards as objects. Maybe something like:

var cards = {};

cards.Recruit = function () {
  this.name = "Recruit";
  this.value = Math.ceil(Math.random() * 6);
  this.play = function (player, opponent) {
    player.soldiersGained += this.value;
  };
};

cards.Ambush = function () {
  this.name = "Ambush";
  this.value = Math.ceil(Math.random() * 3);
  this.play = function (player, opponent) {
    opponent.soldiersGained -= this.value;
    opponent.population -= this.value;
  };
}

// etc.

I'm skipping some stuff here such as loading the correct SVG, but I'll leave that to you - that too can be done in the constructor.

I've also eschewed making an actual prototypal (class) hierarchy out of this. Technically, you could have a base Card prototype or class that the specific cards derive from. But in the code above, the cards only share structure, not behavior (i.e. similarly named properties, but different code), which makes inheritance a little meh. However, some things might very well be shared across all cards (such as the logic for determining the SVG to load), so for that it might make sense to have a common prototype.

Anyway, with the above you can create a new card like so:

var someCard = new cards.Ambush();

and play it with:

someCard.play(human, computer); // or .play(computer, human)

This way, the card's stats and effects are encapsulated in a object. There's no need to switch everywhere, it's easy to add new cards, and it's easy to see what a card does.

On a minor note, I'd make a little helper method for creating random numbers in some range, since you're repeating Math.random() a lot.

As for the UI, you keep finding and re-finding elements with getElementById. That's ok, but it's also repetitive. I'd find the elements once, store them as variables, and simple access them when necessary.

StackExchange Code Review Q#155282, answer score: 5