mabbashm110/codeacademyjsapi

Code Academy JavaScript API

codeacademyjsapi's Issues

Semantic error - function reference, not function invocation!

codeacademyJSAPI/project-1-rock-paper-scissors-x99/project-1-rock-paper-scissors-x99/js/game-logic.js

Line 159 in ae1b1fd

if (ensureValues){

This is a function reference, not a function invocation! so the condition is always true.

At line 140 you set the constant ensureValues to be a function reference (a truthy value).

You need to invoke the method here, so that it gets evaluated...

    if (ensureValues()) {
       ...

Simplify

codeacademyJSAPI/project-1-rock-paper-scissors-x99/project-1-rock-paper-scissors-x99/js/game-logic.js

Lines 26 to 31 in ae1b1fd

	if (gameOptions.indexOf(playerMoves) > -1){
	return true;
	}
	else{
	return false;
	}

The evaluation of an expression in a boolean context (such as this) yields a boolean value.

This code can be simplified to:

const ensureGameOptions = (playerMoves) =>{
    return gameOptions.indexOf(playerMoves) > -1;
};

... and, remembering that function expressions with a single parameter do not require the parentheses, and also using the "implicit return" form, can also drop the braces, can be simplified further:

const ensureGameOptions = playerMoves => gameOptions.indexOf(playerMoves) > -1;

However, I would also suggest renaming the function and variables to make it clearer as to what they are. Note also, you can assign the array to a constant, since it should never change during the run of the program...

const moveTypes = ["rock", "paper", "scissors"];

const isValidType = type => moveTypes.indexOf(type) > -1;

As a final improvement, using the .includes() array method would improve readability:

const isValidType = type => moveTypes.includes(type);

Abstract test conditions into well-named helper functions.

codeacademyJSAPI/project-1-rock-paper-scissors-x99/project-1-rock-paper-scissors-x99/js/game-logic.js

Lines 36 to 45 in ae1b1fd

	if ((playerName && playerMove1 && playerMove1Val && playerMove2 && playerMove2Val && playerMove3 && playerMove3Val)
	&& (ensureGameOptions(playerMove1))
	&& (ensureGameOptions(playerMove2))
	&& (ensureGameOptions(playerMove3))){
	//console.log("Not null")
	//Check values less than 1, greater than 99, totals greater than 99.
	if ((playerMove1Val && playerMove2Val && playerMove3Val) > 1 &&
	(playerMove1Val + playerMove2Val + playerMove3Val) <= 99 &&
	(playerMove1Val && playerMove2Val && playerMove3Val) <= 100)
	{

This is "quite a mouthful" and difficult to verify for correctness at a glance. Also, notice because you have used "positive" tests, each test adds a level of indentation.

My suggestion would be to reverse the sense of the tests, so you can exit the function as soon as you determine that the input is bad (this is known as "fail fast"). This has the benefit of reducing the levels of indentation required. Also, abstract the tests into helper methods, to aid readability of the code.

Note too, that I've chosen shorter names for the parameters to the function.

For example:

const setPlayerMoves = (playerName, m1type, m1value, m2type, m2value, m3type, m3value) => {
    if (!m1type || !m1value || !m2type || !m2value || !m3type || !m3value) {
        return null;
    }
    if (!validTypes(m1type, m2type, m3type) || !validValues(m1value, m2value, m3value)) {
        return null;
    }
    // at this point, we know the types and values are good... 
    // ...
};

const validTypes = (t1, t2, t3) => isValidType(t1) && isValidType(t2) && isValidType(t3);

const validValues = (v1, v2, v3) => ...