I'm currently learning Javascript and working through a rock, scissors, paper tutorial. There's already a couple of queries on this site based on the same tutorial at Codeacademy. However, my query is based on my perspective and so I would appreciate some feedback from the SO munity.

My code is as follows:-

var userChoice = prompt("Are you picking rock, paper or scissors?");
var puterChoice = Math.random();
if (puterChoice < 0.34) {
    puterChoice = "rock";
} else if(puterChoice <= 0.67) {
    puterChoice = "paper";
} else {
    puterChoice = "scissors";
}

console.log ("Computer picks" + " " +puterChoice);
var pare = function (choice1, choice2) {
    if (choice1 === choice2) {
        console.log ("You're both psychic. It's a tie!");
    }    


    if (choice1=="rock") {
        if (choice2 =="scissors") {
            console.log ("Rock wins. You bad boy, you.");
        }
        if (choice2=="paper") {
         console.log ("Paper wins. You noob") ;
        }
    }

    if (choice1=="paper") {
        if (choice2=="rock") {
            console.log ("Paper wins. You bad boy, you.");
        }
        if (choice2=="scissors") {
            console.log ("Scissors wins. You noob.") ;
        }
    }

    if (choice1=="scissors") {
        if (choice2=="rock") {
            console.log ("Rock wins, you noob");
            }
        if (choice2=="paper") {
            console.log ("Scissors wins. You bad boy, you.");
        }
    }

    };

    pare (userChoice,puterChoice);

I've always thought that when using conditionals, it should only be either of the below cases:

1. single condition, then just if's are fine.
2. more than one condition, then it must be if and end with an else OR if , else if etc and end with an else.

As a result, I initially used an else in the second nested, conditional statement here if (choice2=="paper") . Then realised the logic was wrong because it printed that string as well, if it was a tie.

So I thought about it and realised it would work if I used an if instead of the else. And it does work. However, I am just not sure if my code is just wrong...or less than ideal. Is it?

Additionally, to make it look better (and not just full of if's) I was thinking about swapping out the if in if (choice1=="paper") with an else if. However, I wouldn't know how to end the next condition - if (choice1=="scissors") { . How would you do it?

Pardon the long queries - I'm just trying to make sure I understand it well.

Thanks

I'm currently learning Javascript and working through a rock, scissors, paper tutorial. There's already a couple of queries on this site based on the same tutorial at Codeacademy. However, my query is based on my perspective and so I would appreciate some feedback from the SO munity.

My code is as follows:-

var userChoice = prompt("Are you picking rock, paper or scissors?");
var puterChoice = Math.random();
if (puterChoice < 0.34) {
    puterChoice = "rock";
} else if(puterChoice <= 0.67) {
    puterChoice = "paper";
} else {
    puterChoice = "scissors";
}

console.log ("Computer picks" + " " +puterChoice);
var pare = function (choice1, choice2) {
    if (choice1 === choice2) {
        console.log ("You're both psychic. It's a tie!");
    }    


    if (choice1=="rock") {
        if (choice2 =="scissors") {
            console.log ("Rock wins. You bad boy, you.");
        }
        if (choice2=="paper") {
         console.log ("Paper wins. You noob") ;
        }
    }

    if (choice1=="paper") {
        if (choice2=="rock") {
            console.log ("Paper wins. You bad boy, you.");
        }
        if (choice2=="scissors") {
            console.log ("Scissors wins. You noob.") ;
        }
    }

    if (choice1=="scissors") {
        if (choice2=="rock") {
            console.log ("Rock wins, you noob");
            }
        if (choice2=="paper") {
            console.log ("Scissors wins. You bad boy, you.");
        }
    }

    };

    pare (userChoice,puterChoice);

I've always thought that when using conditionals, it should only be either of the below cases:

1. single condition, then just if's are fine.
2. more than one condition, then it must be if and end with an else OR if , else if etc and end with an else.

As a result, I initially used an else in the second nested, conditional statement here if (choice2=="paper") . Then realised the logic was wrong because it printed that string as well, if it was a tie.

So I thought about it and realised it would work if I used an if instead of the else. And it does work. However, I am just not sure if my code is just wrong...or less than ideal. Is it?

Additionally, to make it look better (and not just full of if's) I was thinking about swapping out the if in if (choice1=="paper") with an else if. However, I wouldn't know how to end the next condition - if (choice1=="scissors") { . How would you do it?

Pardon the long queries - I'm just trying to make sure I understand it well.

Thanks

• javascript
• if-statement
• coding-style

Share Improve this question Follow asked Jan 23, 2014 at 21:26 kravoonkravoon 1311 silver badge44 bronze badges 3

• 1 You don't have to end with an else. if and else if and nothing else is just fine. – basilikum Commented Jan 23, 2014 at 21:35
• I see. And is this quite mon, if the situation deserves it? – kravoon Commented Jan 24, 2014 at 0:23
• Yes, it's perfectly normal. – basilikum Commented Jan 24, 2014 at 6:26

Add a ment  | 

3 Answers 3

Sorted by: Reset to default 2

This is pletely code style, so you should strive for 'clean code'.

The most naive alternative would be to use a switch. For example:

var pare = function (choice1, choice2) {

    switch(true){
        case choice1 === choice2:
            console.log ("You're both psychic. It's a tie!");
            break;
        case choice1 ===  "rock":
            if(choice2 === "scissors"){
                console.log ("Rock wins. You bad boy, you.");
            } else{
                console.log ("Paper wins. You noob") ;
            }
            break;
        case choice1 === "paper":
                if (choice2=="rock") {
                    console.log ("Paper wins. You bad boy, you.");
                } else{
                    console.log ("Scissors wins. You noob.") ;
                }
            break;
        case choice1 === "scissors":
            if (choice2=="rock") {
                console.log ("Rock wins, you noob");
            } else{
                console.log ("Scissors wins. You bad boy, you.");
            }
            break;
    }
};

That's just one method though. Whatever you choose, you should try to make the code cleaner and more elegant.

This version might look better:

var pare = function(choice1, choice2){
    var options = ['rock','paper','scissors'];
    var result = options.indexOf(choice1) - options.indexOf(choice2);
    switch(result){
        case 1:
        case -2:
            //choice1 wins
            break;
        case 0: 
            //tie
            break;
        case -1:
        case 2:
            //choice2 wins
            break;
    }
}

But this version is not so immediately understood - so there's a solution which is better than both this and the first one- a solution which is cleaner and is much easier for the reader to understand - you should try to find one.

Definitely use else if, so that the second condition test only runs if the first was false

if ('hi' === 'hi) {
    console.log('true');
} else{
    console.log('this will never log')
}

Can also be written with a Ternary operator:

'hi' === 'hi' ? console.log('true') : console.log('this will never log');

Alternative ternary option --

console.log('hi' === 'hi' ? 'true' : 'this will never log');

if(choice1 == choice2){
    console.log ("You're both psychic. It's a tie!");
}
if(choice1 == "rock"){
    if(choice2 == "scissors"){
        console.log ("Rock wins. You bad boy, you.");
    }
    else {
        console.log ("Paper wins. You noob") ;
    }
}
...

Assuming choice1 and choice2 are both "rock", the function checks if(choice1 == choice2). This is true, so it prints the 'tie' message.

Because the next conditional is also an if, it then checks if choice1 is "rock". It is, so it enters the nested block, checks if choice2 is "scissors" - it's not, so it moves to the else block, and prints the 'paper wins' message.

When it is a tie, choice1 == choice2 you don't really need to check anything further. else if and else conditionals are only checked if the conditions before are not satisfied.

So you could change it to:

if(choice1 == choice2){
    console.log ("You're both psychic. It's a tie!");
}
else if(choice1 == "rock"){
    if(choice2 == "scissors"){
        console.log ("Rock wins. You bad boy, you.");
    }
    else {
        console.log ("Paper wins. You noob") ;
    }
}
else if(choice1 == "paper"){
    ...
}
else{ //choice1 == "scissors"
    ...
}

You can also break out of functions early using a return statements. Or, as Etai suggests, you could use switch to check the conditions.