Reviewing a newbies code as they work through a web dev course

https://github.com/melvincayas/math-whizard

Browsing through index.html

Looks pretty clean, consistent code style. Theres a handful of container elements that appear to be toggled on and off depending on the game state.

There's a list of script tags at the bottom. One way to refactor this would be to import these files as ES6 modules by making the tags <script type="module" ...>. Since theres no 3rd party modules being imported, the effect is the same.

// index.html
<script type="module" src="./scripts/index.js"></script>

// index.js
import sounds from './scripts/utilities/sounds.js'
import answerInput from './scripts/utilities/answerInput.js'
...

That said, you wouldn't want to do this if you needed to import something like lodash, as those libraries aren't meant to be imported in the browser. Each import will make a network request to fetch the javascript, so importing a tool like lodash would wind up making a couple hundred of requests for other packages that it imports. This is where tools like rollup, parcel, webpack help out since they can scan your source code, and pull together all the code thats being used and put it together in one big file.

CSS

.all-btns seems to be applied to many (but not all buttons). If you wanted this to target all buttons without having to specify the class to each button, use the button { ... } And you can override as needed in each individual button.

On a larger scale project, one of my nits is color codes. Its easy to add as many colors as you can think of, but it becomes difficult to keep track of which colors you're using for what (see here for an example from W3). So you can either give all your custom colors names using CSS variables, or you can give your colors "purpose" and name them that way. Variables are a little easier to use with SASS as well.

:root {
  --btn-colors: #00ff00
}

.submit-btn {
  color: var(--btn-colors)
}

Browsing the js files

index.js

Everything seems to be available globally, which for the purposes of this project is more than good enough. If you wanted to refactor it, you'd want to import those functions instead of depending on them being available globally. The advantage being that it explicitly defines where those functions come from, instead of having to search for them. Also within VSCode you'd be able to go to the definition of the function with a keyboard shortcut.

Nit: function naming - functions should have verbs, checkForValidOperators or validateOperators would also work.

if (!validOperators()) return;

Checking the implementation of the function:

const validOperators = () => {
	if (filterCheckBox().length === 0) {
		error.play();
		errorText.innerText = "Please select an operator.";
		errorText.classList.remove("hidden");
		return false;
	}

	if (filterCheckBox().length > 0) {
		errorText.innerText = "";
		errorText.classList.add("hidden");
		return true;
	}
};

• Another good reason to use imports when possible is that its easy to know where variables come from (like filterCheckBox)

• This function is doing too much at once - it is both doing the checking, and tweaking the view, which wasn't clear based on the name. One way of refactoring this is to split the responsibility. One function does the view updates, the other does the checking. The purpose of the refactor here being explicit about what the functions are doing.

  const didUserChooseOperators = () => {
    return filterCheckBox().length > 0
  }
  
  const checkGameSettings = () => {
  	if (didUserChooseOperators()) {
  	  // hide errors
  	} else {
  	  // show error message
  	}
  }