Any suggestions to improve?


#1

https://www.codecademy.com/courses/javascript-beginner-en-ZA2rb/0/5?curriculum_id=506324b3a7dffd00020bf661


I'm beginning to learn javascript and the only exposure I've had is going through the (awesome) courses that codecademy offer. I have just finished the 'Code your own adventure 2' course. I have created a little mini game. My code works as expected, however I'm just wondering if anybody has any suggestions that could improve the below code? i.e. more efficient, quicker, cleaner? I'd rather get in good habits at this early stage than discover years down the line the my code is sloppy. I would appreciate any feedback given.


var user = prompt("You are playing a football game. You are the central midsfielder. You are 30 yards away from goal with only two defenders in front of you. You have a teammate making a great run in behind the defenders. The goalkeeper also appears to be off his line. Do you PASS, DRIBBLE or SHOOT?").toUpperCase();
var userEffort = Math.random();
var userSkill = Math.random();
var userLuck = Math.random();
var mindOnGame = true;
var goodDay = true;
var luckyDay = true;
var gameWon = "You have won the game for your team!";
var gameLost = "You have lost the game for your team :(";

if (userEffort > 0.5) {
    mindOnGame = true;
} else {
    mindOnGame = false;
}
if (userSkill > 0.5) {
    goodDay = true;
} else {
    goodDay = false;
}
if (userLuck > 0.5) {
    luckyDay = true;
} else {
    luckyDay = false;
}

switch (user) {
case "PASS":
    console.log("You try to pass to your teammate");
    if (userEffort || userSkill === true) {
        console.log("You made a great pass to your teammate!");
        if (userEffort && userSkill === true) {
            console.log("Your teammate crosses you the ball");
            if (userEffort && userSkill && userLuck === true) {
                console.log("Your score an acrobatic over head kick!!!");
                console.log(gameWon);
            } else {
                console.log("You headed the ball into the back of the net!");
                console.log(gameWon);
            }
        } else if (userLuck === true) {
            console.log("Your teammate passes you the ball on the six yard box!");
            console.log("You shoot for goal and score!");
            console.log(gameWon);
        } else {
            console.log("Your teammate looses the ball");
            console.log(gameLost);
        }
    } else {
        console.log("You passed it straight to the defenders!");
        console.log(gameLost);
    }
    break;
case "DRIBBLE":
    console.log("You try to dribble passed the defenders");
    if (userEffort || userSkill === true) {
        console.log("You took on both the defenders!");
        if (userEffort && userSkill === true) {
            console.log("You shoot for goal!");
            if (userEffort && userSkill && userLuck === true) {
                console.log("GOAL!!! Right in the top corner!");
                console.log(gameWon);
            } else if (userLuck === true) {
                console.log("The keeper dives over the ball! GOAL!");
                console.log(gameWon);
            } else {
                console.log("The keeper saves it");
                console.log(gameLost);
            }
        } else {
            console.log("You fall over the ball and lose possession");
            console.log(gameLost);
        }
    } else {
        console.log("The defenders tackle you and you lose possession");
        console.log(gameLost);
    }
    break;
case "SHOOT":
    console.log("You try to shoot over the goalkeeper");
    if (userEffort && userSkill && userLuck === true) {
        console.log("The ball flies into the top corner!");
        console.log(gameWon);
    } else if ((userEffort && userLuck === true) || (userSkill && userLuck === true)) {
        console.log("The keeper falls and the ball goes into the back of the net!");
        console.log(gameWon);
    } else {
        console.log("The shot goes way wide");
        console.log(gameLost);
    }
    break;
default:
    console.log("You don't know what to do and take too long! The defenders tackle you.");
}


#2

Speed is entirely irrelevant here, because no large amounts of data are involved.
That leaves correctness, readability, scalability.

if (userEffort > 0.5) {
    mindOnGame = true;
} else {
    mindOnGame = false;
}

The result of your condition is a boolean, consider just assigning the result of that to your variable instead.

var mindOnGame = true;
var goodDay = true;
var luckyDay = true;

Those lines have no effect, you overwrite their values

userEffort || userSkill === true

consider what the value on the left side of === evaluates to, and what the whole expression evaluates to. And if you mean that both userEffort and userSkill would be compared to true, then neither || or === have that behaviour, and you probably haven't been given any reason to think that they do!


#3

Running the code through google's closure compiler with --warning_level VERBOSE also reveals a problem (same problem repeated many times)

stdin:30: WARNING - condition always evaluates to false
left : number
right: boolean
    if (userEffort || userSkill === true) {
                      ^

stdin:32: WARNING - condition always evaluates to false
left : number
right: boolean
        if (userEffort && userSkill === true) {
                          ^

stdin:34: WARNING - condition always evaluates to false
left : number
right: boolean
            if (userEffort && userSkill && userLuck === true) {
                                           ^

stdin:41: WARNING - condition always evaluates to false
left : number
right: boolean
        } else if (userLuck === true) {
                   ^

stdin:56: WARNING - condition always evaluates to false
left : number
right: boolean
    if (userEffort || userSkill === true) {
                      ^

stdin:58: WARNING - condition always evaluates to false
left : number
right: boolean
        if (userEffort && userSkill === true) {
                          ^

stdin:60: WARNING - condition always evaluates to false
left : number
right: boolean
            if (userEffort && userSkill && userLuck === true) {
                                           ^

stdin:63: WARNING - condition always evaluates to false
left : number
right: boolean
            } else if (userLuck === true) {
                       ^

stdin:81: WARNING - condition always evaluates to false
left : number
right: boolean
    if (userEffort && userSkill && userLuck === true) {
                                   ^

stdin:84: WARNING - condition always evaluates to false
left : number
right: boolean
    } else if ((userEffort && userLuck === true) || (userSkill && userLuck === true)) {
                              ^

stdin:84: WARNING - condition always evaluates to false
left : number
right: boolean
    } else if ((userEffort && userLuck === true) || (userSkill && userLuck === true)) {
                                                                  ^

0 error(s), 11 warning(s), 100.0% typed

#4

Thank you for taking the time to help me out - really appreciate it.

I have amended my code below.

var user = prompt("You are playing a football game. You are the central midsfielder. You are 30 yards away from goal with only two defenders in front of you. You have a teammate making a great run in behind the defenders. The goalkeeper also appears to be off his line. Do you PASS, DRIBBLE or SHOOT?").toUpperCase();
var userEffort = Math.random();
var userSkill = Math.random();
var userLuck = Math.random();
var mindOnGame = userEffort
var goodDay = userSkill
var luckyDay = userLuck
var gameWon = "You have won the game for your team!";
var gameLost = "You have lost the game for your team :(";

if (userEffort > 0.5) {
    mindOnGame = true;
} else {
    mindOnGame = false;
}
if (userSkill > 0.5) {
    goodDay = true;
} else {
    goodDay = false;
}
if (userLuck > 0.5) {
    luckyDay = true;
} else {
    luckyDay = false;
}

switch (user) {
case "PASS":
    console.log("You try to pass to your teammate");
    if (mindOnGame || goodDay) {
        console.log("You made a great pass to your teammate!");
        if (mindOnGame && goodDay) {
            console.log("Your teammate crosses you the ball");
            if (mindOnGame && goodDay && luckyDay) {
                console.log("Your score an acrobatic over head kick!!!");
                console.log(gameWon);
            } else {
                console.log("You headed the ball into the back of the net!");
                console.log(gameWon);
            }
        } else if (luckyDay) {
            console.log("Your teammate passes you the ball on the six yard box!");
            console.log("You shoot for goal and score!");
            console.log(gameWon);
        } else {
            console.log("Your teammate looses the ball");
            console.log(gameLost);
        }
    } else {
        console.log("You passed it straight to the defenders!");
        console.log(gameLost);
    }
    break;
case "DRIBBLE":
    console.log("You try to dribble passed the defenders");
    if (mindOnGame || goodDay) {
        console.log("You took on both the defenders!");
        if (mindOnGame && goodDay) {
            console.log("You shoot for goal!");
            if (mindOnGame && goodDay && luckyDay) {
                console.log("GOAL!!! Right in the top corner!");
                console.log(gameWon);
            } else if (luckyDay) {
                console.log("The keeper dives over the ball! GOAL!");
                console.log(gameWon);
            } else {
                console.log("The keeper saves it");
                console.log(gameLost);
            }
        } else {
            console.log("You fall over the ball and lose possession");
            console.log(gameLost);
        }
    } else {
        console.log("The defenders tackle you and you lose possession");
        console.log(gameLost);
    }
    break;
case "SHOOT":
    console.log("You try to shoot over the goalkeeper");
    if (mindOnGame && goodDay && luckyDay) {
        console.log("The ball flies into the top corner!");
        console.log(gameWon);
    } else if ((mindOnGame && luckyDay) || (goodDay && luckyDay)) {
        console.log("The keeper falls and the ball goes into the back of the net!");
        console.log(gameWon);
    } else {
        console.log("The shot goes way wide");
        console.log(gameLost);
    }
    break;
default:
    console.log("You don't know what to do and take too long! The defenders tackle you.");
}

I will answer your suggestions one at a time:

The result of your condition is a boolean, consider just assigning the result of that to your variable instead. - Could you help me understand the benefit of assigning the result rather than a boolean? The if statements in my cases rely on the variables being true or false. Without the variable being true or false would I not have to do as follows: if((variable1 > 0.5) || (variable2 > 0.5))? Maybe I've misunderstood (which please tell me if I have) but that seems like more code to type than necessary.

Those lines have no effect, you overwrite their values. - I simply put those variables there to declare the variable as global. Could I instead put var mindOnGame?

consider what the value on the left side of === evaluates to, and what the whole expression evaluates to. And if you mean that both userEffort and userSkill would be compared to true, then neither || or === have that behaviour, and you probably haven't been given any reason to think that they do! - I see where I went wrong here - thank you for pointing that out. I have no idea where I got my line of thinking from.


#5

The result is boolean.

mindOnGame = Math.random() > 0.5  // result will be true | false

Then don't initiate them. Or better yet, initiate them with the expression above, that way you turn .. 6 lines into one, at no readability cost, only benefit.


#6

Saw something else as well, it's insignificant for most purposes but is worth taking note of:

userLuck > 0.5

userLuck refers to the result of Math.random()
That result will be in the range [0, 1)
So 0 is a possible outcome, but 1 is not.
Thus, half will be 0.5 or greater, and the other half will be less than 0.5
The above should therefore read:

userLuck < 0.5

For your code, I agree, it's close enough. But edge cases are common sources of bugs, considering where things start and stop is important

An example where it matters is in dividing an array into two.
You would divide it roughly at length / 2, but where exactly should it cut for odd/even? Perhaps one of them need a +1 or -1


#7

I see, thank you. I have changed the below code:

var userEffort = Math.random();
var userSkill = Math.random();
var userLuck = Math.random();
var mindOnGame = userEffort
var goodDay = userSkill
var luckyDay = userLuck

if (userEffort > 0.5) {
    mindOnGame = true;
} else {
    mindOnGame = false;
}
if (userSkill > 0.5) {
    goodDay = true;
} else {
    goodDay = false;
}
if (userLuck > 0.5) {
    luckyDay = true;
} else {
    luckyDay = false;
}

To this:

var mindOnGame = Math.random() < 0.5;
var goodDay = Math.random() < 0.5;
var luckyDay = Math.random() < 0.5;

Already I'm amazed by how there are many ways to achieve the same result.

Once again thank you for taking the time to help.


#8

I don't suggest implementing the following, what you did is fine for that size.

But take note of that your program resembles a flow-chart, and that the nodes could instead be represented as data where they refer to each other. You might have a small piece of code that just navigates between them and/or they could contain some of their own code, passing on program control between them.

There isn't really any difference between code and data (and indeed our computers store them in the same storage, calling a function is to say to execute a location in memory). Code is just executable data. When there's a lot of similar code repeated over and over you can often write it in a single place in a more general form, and put the non-executable data in a data structure.


#9

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.