6
\$\begingroup\$

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?

\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ Crap. The code there is a 2-minutes older version than what I have on that answer. It was a copy-paste problem. I trully hate the typeof obj.someMethod === "function". But you are right on that point. Using String(something) is a way, but something.toString() is the right way. Other than that, I agree with what you said. I just don't get the Java-ish part. I'm not a Java programmer. (Even though I've reviewed some Java code) \$\endgroup\$ Aug 28, 2015 at 17:02
  • \$\begingroup\$ I've noticed now that even the answer has the wrong code, but the code on the stack snippet is working perfectly and should be the code in the answer and to be reviewed. \$\endgroup\$ Aug 28, 2015 at 17:06
  • \$\begingroup\$ @IsmaelMiguel By the rules of the site, I can only review the code in the question. Sorry, but them's the rules. Anyway: To me, something.toString() signals that something is expected to implement a useful toString (rare in JS), whereas String(something) is a "hard" coercion, similar to + '' but much more explicit. Same result, but I stick to String(). As for the Java part, I don't know your background, I just know that a lot of this looks more like Java than JavaScript. A lot of busywork for little-to-no benefit in a language as loose as JS. \$\endgroup\$
    – Flambino
    Aug 28, 2015 at 17:14
  • \$\begingroup\$ I know the rules, I just was explaining myself. It was an error of my part. It could have been avoided if I had added the stack snippet. And you are right about your point about using the String() constructor. \$\endgroup\$ Aug 28, 2015 at 17:49
3
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ Sadly, there is no built-in way to make a copy of an object. That really is the only non-over engeneered way. Returning this.strengths is as bad as returning the arguments object in a function call. It allows to do exactly what you aren't supposed to. It is a bug to do that. I don't need tho expose the whole object, but I decided to. If you implement a GUI where you check the strengths and weaknesses of a "player", you need those. And your isWeakFor is bugged. The fastest way is to verify if a "player" is not a strength. \$\endgroup\$ Aug 27, 2015 at 19:57
  • \$\begingroup\$ My point with isWeakFor is that the game object can delegate that logic to the player object, and then you've eliminated a need to return the weaknesses object. \$\endgroup\$
    – ErikR
    Aug 27, 2015 at 21:14
  • \$\begingroup\$ I think you mean user.winsTo(enemy)? \$\endgroup\$ Aug 27, 2015 at 21:21
  • \$\begingroup\$ I mean in the game object's match method where it calls var enemyWeakness = enemy.getWeaknesses()[userName] || {enemy: enemy, soft: false}; etc. That's to only place where I see getWeaknesses is called.The game object should just call enemy.isWeakFor(userName) to get the soft boolean value. \$\endgroup\$
    – ErikR
    Aug 27, 2015 at 21:29
  • \$\begingroup\$ But I don't want just the soft value \$\endgroup\$ Aug 27, 2015 at 21:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.