Rock, Paper, Scissors, Lizard and Spock with OOP

Question

Today I saw an extremely interesting question on the Rock, Paper, Scissors, Lizard and Spock game. I've decided to take my swing at it and I've written the following OOP implementation:

function RockPaperScissors(name, hard_action, soft_action) {
    this.name = (name + '').toLowerCase();
    this.hard_action = hard_action + '';
    this.soft_action = (soft_action || hard_action) + '';
    this.strengths = {};
    this.weaknesses = {};
}

RockPaperScissors.prototype = {
    getName: function(){
        return this.name;
    },
    addStrength: function(strength) {
        if( !(strength instanceof RockPaperScissors) ) {
            throw new TypeError('A strength must be an instance of RockPaperScissors');
        }

        this.strengths[strength.getName()] = strength;
    },
    getStrengths: function() {
        var strengths = {};
        for(var k in this.strengths)
        {
            strengths[k] = this.strengths[k];
        }
        return strengths;
    },
    addWeakness: function(weakness, soft) {
        if( !(weakness instanceof RockPaperScissors) ) {
            throw new TypeError('A weakness must be an instance of RockPaperScissors');
        }

        this.weaknesses[weakness.getName()] = {
            enemy: weakness,
            soft: !!soft
        };
    },
    getWeaknesses: function() {
        var weaknesses = {};
        for(var k in this.weaknesses)
        {
            weaknesses[k] = this.weaknesses[k];
        }
        return weaknesses;
    },
    getAction(soft){
        return soft? this.soft_action : this.hard_action;
    },
    winsTo: function(enemy) {
        if( !(enemy instanceof RockPaperScissors) ) {
            throw new TypeError('An enemy must be an instance of RockPaperScissors');
        }

        return (enemy.getName() in this.strengths);
    }
};

function RockPaperScissorsGame(){
    this.score = {
        won: 0,
        lost: 0,
        tied: 0
    };
    this.games = 0;
}

RockPaperScissorsGame.prototype = {
    match: function(user, enemy){
        if( !(user instanceof RockPaperScissors) ) {
            throw new TypeError('The user must be an instance of RockPaperScissors');
        }
        if( !(enemy instanceof RockPaperScissors) ) {
            throw new TypeError('The enemy must be an instance of RockPaperScissors');
        }

        this.games++;

        if( user == enemy ) {
            this.score.tied++;
            return 'Tie';
        }

        var userName = user.getName();
        var enemyName = enemy.getName();

        if( user.winsTo(enemy) ) {
            this.score.won++;
            var enemyWeakness = enemy.getWeaknesses()[userName] || {enemy: enemy, soft: false};

            return userName + ' ' + user.getAction(enemyWeakness.soft) + ' ' + enemyName;

        } else {
            this.score.lost++;
            var userWeakness = user.getWeaknesses()[enemyName] || {enemy: enemy, soft: false};

            return enemyName + ' ' + enemy.getAction(userWeakness.soft) + ' ' + userName;
        }
    },
    getMatches: function(){
        return this.games;
    },
    getWins: function(){
        return this.won;
    },
    getLosses: function(){
        return this.lost;
    },
    getTies: function(){
        return this.tied;
    }
};

So far so good. The setup is minimal, allows to extend it to as many as you want (you can add a tornado, a hurricane, a thunder .... anything!).

Here, there's the concept of hard and soft actions. Soft actions may be, for example, the paper wrapping the rock. Hard actions are, for example, the scissor beheading the lizard. You can define if a weakness is soft or hard.

To make it easier, I've also implemented a way to have multiple games and easily start a new one.

---

It is a good idea, but I'm not 100% happy with its quality. There's something that makes me uneasy.

In terms of overall quality and OOP, what am I doing wrong? What readability improvements can be made?

Answer 1

First of all, you've got a syntax error:

getAction(soft){

should be:

getAction: function (soft) {

---

Anyway: It's pretty clean, but it a little too... Java'ish, for lack of a better word. Or at least it seems to be adhering too close to practices more applicable to other languages.

You're not using "private" (i.e. closure) variables, so many of the getters and setters aren't necessary. For instance, if I want the name of the user, I'd just write user.name. I wouldn't bother with user.getName().

Related to that, though there's no setter for the name property, that doesn't stop me from just assigning something.

All object properties are essentially public in JavaScript, and trying to avoid that is typically more hassle than it's worth. I find it easier to just roll with it for the most part.

Of course, you may want to keep some things "private", which is where closures come in. However, it's a little outside the scope of just a review, so I'll leave that be.

Point is, without all the getters/setters, you end up with a lot less code. And less code means less potential for bugs. Bugs like this:

getWins: function(){
    return this.won; // always returns undefined
},

The property this.won doesn't exist; you're looking for this.score.won. The same error is repeated for the other getters (except getMatches).

Related to all of this is the object copying you're doing with returning strengths and weaknesses. Again, it's a good idea - but is it worth it? Again, if I want to mess with the internal weaknesses obj, I can just go user.weaknesses... and manipulate the object directly anyway.

The getters only provide safety if I agree to use them. I'd rather agree to tread carefully in return for direct access to properties (which I already have, but still).

There's also a lot of type checking going on, which I often find a little too strict. If you pass an object to, say, winsTo that's duck-typing the correct API, there's no reason it should fail. Conversely, if it's completely incompatible, well, that'll probably manifest itself soon enough anyway. Yeah, I know, that's pretty loose (even lazy!), but this isn't a power plant control system, so...

Often times, a better solution to type-checking is feature detection. Instead of checking an object's ancestry, check whether it supports whatever it is you need:

if(typeof obj.someMethod === "function") {
  // ok, obj supports someMethod, so we can proceed
}

---

Lastly some style and structural things:

• As ErikR points out, the names aren't great. RockPaperScissors is pretty vague. It's more a concept than object.
  On that note, RockPaperScissorsGame, while a clearer name, isn't an impartial game. It's more of a scorecard, really - the user's score card, specifically. Values like won and lost are all from the perspective of one of the players. It'd be completely the opposite for the enemy player. Seems iffy to me.

• You've got under_scored names like soft_action, but the JS convention is to use camelCase, i.e. softAction.

• The something + '' trick isn't pretty. You can just use String(something) to coerce something to a string.

• I'm not a fan of the asymmetry in strengths and weaknesses. For instance, if user beats enemy, you have to ask enemy how it happened by looking up the user in the enemy's weaknesses. That seems odd to me. I might prefer a central "table" object of sorts that keep track of what beats what and how.

Answer 2

First of all, the RockScissorsPaper class describes a player in the game, so maybe reflect that in the name of the class, e.g. RockScissorsPaperPlayer. In that respect the name RockScissorsPaperGame name is good because it gives clues to the reader as to how the object is used.

Secondly, if I read getWeakness and getStrengths correctly, you are just returning a copy of an object. Surely there must be a more idiomatic way to create a shallow copy of a JS object that doesn't require explicit looping?

Or, how about just returning this.strengths and hope that the caller doesn't modify it?

Or, why do you do even need to expose getStrengths? I don't see where you call it.

And for getWeaknesses you don't need to expose the entire object - you just need to know if a player has a weakness for a specific user name. In the class RockScissorsPaper, just expose this function:

'isWeakFor' : function (userName) {
                return (this.weaknesses[userName] || { soft: false }).soft
              }

Note - my JS is a little rusty, but this should illustrate the idea.

Update:

Per our discussion in the notes, I am suggesting changing this code:

match: function(user, enemy){
    ...
    if( user.winsTo(enemy) ) {
        this.score.won++;
        var enemyWeakness = enemy.getWeaknesses()[userName] || {enemy: enemy, soft: false};

        return userName + ' ' + user.getAction(enemyWeakness.soft) + ' ' + enemyName;

    } else {
        this.score.lost++;
        var userWeakness = user.getWeaknesses()[enemyName] || {enemy: enemy, soft: false};

        return enemyName + ' ' + enemy.getAction(userWeakness.soft) + ' ' + userName;
    }
}

with this:

match: function(user, enemy){
    ...
    if( user.winsTo(enemy) ) {
        this.score.won++;
        var soft = enemy.isSoftFor(userName)
        return userName + ' ' + user.getAction(soft) + ' ' + enemyName;

    } else {
        this.score.lost++;
        var soft = user.isSoftFor(enemyName)
        return enemyName + ' ' + enemy.getAction(soft) + ' ' + userName;
    }
}

and it avoids having to return the weaknesses object from the player object.

Perhaps my implementation of isSoftFor isn't quite correct, but you can make it work.