Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

In a few weeks I need to give a workshop about JavaScript to colleagues. They do not have much experience with JavaScript so I will explain the basics of JavaScript. At the end of the day I want them to make a small game with the aspects learned during the day.

I was thinking of Rock Paper Scissors. Below you can find the final code for this game:

var RockPaperScissors = (function () {
    var t = 'Tie',
        c = 'Computer wins',
        p = 'Player wins',
        winningMap = [
            [t, c, p],
            [p, t, c],
            [c, p, t]
        ],
        choices = ["Rock", "Paper", "Scissors"];

    var getComputerChoice = function () {
        return Math.floor(Math.random() * 3);
    };

    var getWinner = function (playerChoice, computerChoice) {
        return winningMap[playerChoice][computerChoice];
    };

    function RockPaperScissors(playerChoice) {
        this.playerChoice = playerChoice;
        this.computerChoice = getComputerChoice();
        this.winner = getWinner(this.playerChoice, this.computerChoice);
    }

    RockPaperScissors.prototype.toString = function () {
        return this.winner + " [Computer: " + choices[this.computerChoice] + ", Player: " + choices[this.playerChoice] + "]";
    };

    return RockPaperScissors;

})();

This code is executed everytime a select input box changes (see this as the user input):

$('#choice').on('change', function () {
    var $input = $(this);

    if ($input.val() === '-1') return;

    var rockPaperScissors = new RockPaperScissors($input.val());

    $('#message').html(rockPaperScissors.toString());
});

I've also set up a Fiddle.

Do you think this code is clear for beginners? Is this too advanced? I'm not sure if I should introduce them to prototyping or not.

share|improve this question
    
The definition of the winningMap is very confusing. I suggest a different form of input: var winningMap = {"Rock": "Scissors", Paper: "Rock", Scissors: "Paper"};. It defines which choice can be beaten by a specific selection. If both inputs are the same, it's obvious a tie. –  maja Jun 14 at 12:03
    
And maybe a new-Operator would be understood easier instead of the closure? var RockPaperScissors = new function(){ ... }; –  maja Jun 14 at 12:06
    
@maja - why don't you combine your thoughts in to an actual answer... ? –  rolfl Jun 14 at 12:09
1  
I guess "too advanced" would depend on what your audience already knows. What other languages do they usually code? Or are they complete beginnings, because in that case you should probably start with "Hello, world" (or at least be prepared to start there). –  Flambino Jun 14 at 12:47
4  
I don't see why you would require jQuery for a workshop on javascript. I mean... $input.val() vs input.value? If I knew nothing of javascript, I would pick option 2... –  Bart Jun 14 at 13:49

1 Answer 1

up vote 10 down vote accepted

The definition of the winningMap is very confusing.

I suggest a different form of input:

var winningMap = {Rock: "Scissors", Paper: "Rock", Scissors: "Paper"}; 

It defines which choice can be beaten by a specific selection. If both inputs are the same, it's obvious a tie:

var getWinner = function (playerChoice, computerChoice) {
    if(playerChoice === computerChoice){
        return "Tie";
    }
    if(computerChoice === winningMap[playerChoice]){
        return "Player Wins";
    }
    return "Computer Wins";
};

Notice, that I didn't use the string-definitions. I don't think that it's necessary, and it only makes the code a bit more complicated.

In order to convert the computer's choice to an actual string (Rock, Scissors, Paper), you can use an Array:

var stringMapping = ["Rock", "Scissors", "Paper"];

var getComputerChoice = function () {
    return stringMapping[ Math.floor(Math.random() * 3) ];
};

Maybe a new-Operator would be understood easier

The closure might be confusing for a beginner, maybe you should consider to write the Object this way:

function RockPaperScissors (){ 
     var getComputerChoice = function(){ ... };    //= private variable
     this.getWinner = function(){ ... };           //= public variable
};

var actualObject = new RockPaperScissors();
actualObject.method();
actualObject['method']();

If your colleagues are used to object oriented programming, this style might be more familiar.

Always use brackets

if ($input.val() === '-1') { return; }

I personally don't mind conditions within one line, but dropping the brackets is bad practice - and can lead to strange behaviours, especially in JavaScript.

Furthermore, I wonder why u used the dollar-sign at the beginning of the $input-variable?

Edit:

I share @Barts's opinion from the comments - it's not neccessary to use jQuery in this example. You should show them jQuery once they know the basic principles of JavaScript.

share|improve this answer
1  
+1 for the "use brackets/braces" line. Better to learn the stricter syntax rules-of-thumb first; you'll learn the shortcuts later. –  Flambino Jun 14 at 12:40
    
Oh, the +1 is also for all the other points, but the braces is a pet-peeve of mine :) –  Flambino Jun 14 at 13:07
    
Thank you for your input. The winningMap is definitely better like this. I know there are many ways to write Javascript objects. During the workshop I will briefly explain scoping in Javascript. Since they are experienced C# backend developers this will be the most confusing part and has to be explained into detail. I'm going to keep the closure as is because they will also get a course about Typescript. My colleague will explain them how Typescript compiles to Javascript. So they need to have an understanding of scoping & closures. –  Dieter Goetelen Jun 14 at 15:37
    
I use the $-sign to indicate I'm working with a jQuery element. It's just my prefered way of working with jQuery. –  Dieter Goetelen Jun 14 at 17:05

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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