Better card validity check in JS card game

Question

I am writing a card game in Javascript. The game is played with a 32 card deck.

When a card is selected to be played by a user, i do a validity check on the server to check whether it matches the games rules.

These are

• A card may be played when it matches either the color or the value of the last card on the stack
• If a 7 is played, the next player must either draw 2 cards or counter with another 7. If he draws, he may play another card that complies the rule above.
• A jack may be played anytime when there is no 7 to be countered or the player has drawn. It does not have to match the color or value of the last card. When a player plays a jack, he may decide the color of the next card to be played.

I created a class of game that, amongst others, implements a function iscardLegal()

Game.prototype.isCardLegal = function(card,player){

    if(player.flags.forcedDraw > 0 && card.value !== '7'){
        player.setTask('forced draw');
        return false;
    }

    // A jack may be played anytime if there is no 7 on top of the stack
    if(card.value === 'J'){
        return true;
    }

    if(this.forcedColor && this.forcedColor !== card.color){
        player.setTask('color mismatch');
        return false;
    }

    // If there is a forced color due to the playing of a jack, we must ignore the stack#s top card color
    if(this.forcedColor && this.forcedColor === card.color){
        return true;
    }

    if(this.lastCardOnStack().color !== card.color && this.lastCardOnStack().value !== card.value){
            player.setTask('invalid card');
            return false;
    }

    return true;

};

I am unhappy with this function because I end up with a chunk of if rules that need to be in a certain order to match all the rules. If I imagine writing a slightly more complex game, I think this would be unmaintainable.

I am looking for ideas and / or examples of better implemenations of this functionality.

Answer 1

Some things to consider:

1. It seems strange to ask Game whether a given card is legal. In my mind, from an OO perspective, it makes sense to ask the card if it is legal, given some context.
2. The unspoken interface of this function, is "I return a boolean which indicates whether a card is valid" and "I modify the player by setting their action if invalid". This really seems to be beyond the scope of checking whether a card is "valid". It seems like it might be valuable to separate these concepts. One way to do this, that I could imagine, might be having a CardValidity object returned back that has fields like isValid (true/false) and invalidReason (e.g. "color mismatch"). The game would then be in charge of dealing with setting the next player action.

Note: the above are just my opinion and may be of no value

Let's assume you're going to ignore the above, and just want to make the isCardLegal function manage all the state it currently manages. I could imagine having a Fluent Interface that manages each possible outcome. The end implementation might look like this:

Game.prototype.isCardLegal = function(card,player){

    ValidityRuleEngine(card, player, this).
      invalidOn(player.flags.forcedDraw > 0 && card.value !== '7', 'forced draw').
      validOn(card.value === 'J').
      invalidOn(this.forcedColor && this.forcedColor !== card.color, 'color mismatch').
      validOn(this.forcedColor && this.forcedColor === card.color).
      invalidOn(this.lastCardOnStack().color !== card.color && this.lastCardOnStack().value !== card.value, 'invalid card').
      checkValidityAndUpdatePlayer();

};

IMO, this is a step in the right direction, since the rules are very explicit now. Of course the ValidityRuleEngine would return itself for each of the calls to invalidOn and validOn, checking if an end condition is met (the ValidityRuleEngine would have to manage this state as well). validOn would just set the end condition to true if its argument is true, invalidOn would set to false. checkValidityAndUpdatePlayer() would do exactly what it says: return the validity (the end condition) and update the next playerTask.

One additional step you could take, at this point is to create functions that "name" each of the conditions. Like such:

var forcedDrawAndCardIsntSeven = function (gameContext) {
    return gameContext.flags.forcedDraw > 0 && gameContext.card.value !== 7;
};

var cardIsAJack = function(gameContext) {
    return gameContext.card.value == 'J';
};

...

Then invalidOn and validOn could be changed to accept a function rather than a boolean, and pass along the game context (which they should construct from values passed in ValidityRuleEngine). That would change the logic to look like:

Game.prototype.isCardLegal = function(card,player){

    ValidityRuleEngine(card, player, this).
      invalidOn(forcedDrawAndCardIsntSeven, 'forced draw').
      validOn(cardIsAJack).
      invalidOn(forcedColorThatDoesntMatchCard, 'color mismatch').
      validOn(forcedColorThatMatchesCard).
      invalidOn(lastCardDoesntMatchCurrent, 'invalid card').
      checkValidityAndUpdatePlayer();

};

In my opinion, this is nice for a few reasons:

1. It's very explicit. It's clear as to what cases are invalid, what the reason why they are invalid, and which cases are valid.
2. It's maintainable. If cases for validity/invalidity need to be added or removed, the line is simply removed from the fluent calls (along with the function for that rule, assuming that approach is even used).
3. It's extendable. If new entities are taken into consideration (maybe dice, or a coin flip), simply pass it into the rules engine. Frankly, the rules engine doesn't even need to be built here! It can be specified elsewhere.

Overall, I hope some of this helps. If readers have any feedback for this review itself, it would be much appreciated.

Answer 2

This is my new attempt

Game.prototype.isCardLegal = function(card,player){

    var requirements = this.getRequirements();

    if(this.cardsToDraw() > 0 && card.value !== '7'){
        player.setTask('forced draw');
        return false;
    }

    if (card.color !== requirements.color && card.value !== requirements.value && card.value !== 'J') {
        player.setTask('invalid card');
        return false;
    }

    return true;

};

When a card is played the game state will be updated with the required color / value. When the played card is a jack or a 7, the respective requirements are set accordingly. So I don't have to distinguish between the color of the last card palyed and a possible forced color any more, as in both cases the game state property will be updated.

I'm still looking for ideas, though.

Answer 3

I think it is worth another answer to my own question to show my implementation of a 'ruleEngine' as @mjgpy3 has suggested.

I really liked the idea of being able to pipe all the rules in correct order, however it bothered me that I would have to set up a call to invalidOn after each "rule".

Here is what I did:

 function ValidityCheck(){
  this.result = true;
  this.reason = '';

  this.set = function(status,reason){
    this.result = status;
    this.reason = reason || '';
  }
}

var ruleEngine = (function ruleEngine(){

  var game;

  function cardsToDraw(card){

    if(!game.validityCheck.result) return game.validityCheck;

    var result = new ValidityCheck();

    if(game.cardsToDraw() > 0 && card.value !== '7'){
      result.set(false, 'cards to draw');
    }

    return result;

  }

  function cardMatch(card){

    if(!game.validityCheck.result) return game.validityCheck;

    var result = new ValidityCheck();

    var requirements = game.getRequirements();

    if (card.color !== requirements.color && card.value !== requirements.value && card.value !== 'J') {
      result.set(false, 'card mismatch')
    }

    return result;
  }

  var rules = {
    'cards to draw': cardsToDraw,
    'card match': cardMatch
  };

  function check(rule,move){

    if(!game) throw new Error('No game was set');

    game.validityCheck = rules[rule](move.card);

    return this;
  }

  function setGame(gameData){
    game = gameData;
  }

  return{
    check: check,
    setGame: setGame
  }

  })();

  module.exports = ruleEngine;

This allows me to do this:

ruleEngine.setGame(game);

function checkCard(move){

  ruleEngine
  .check('cards to draw',move)
  .check('card match',move);

  if(game.validityCheck.result){
    // make move
  }
  else{
    // return reason
  }

}

Which feels pretty nice, compared to my first solution. Still plenty of room to improve, though.

I wonder if it would be better to go the other way round and set up a rule engine for a game. Then I could implement a setRule method and would get a strongly encapsulated game engine that can be passed to other games as well.

Also, currently a failed check will set up a property on the game itself. Is this a good idea?