8
\$\begingroup\$

I would like to kindly ask to review my code. I'd like suggestions, criticisms, and discussions on what is good and what could be done better.

const wordsArray = [ "aardvark", "albatross", "alligator", "alpaca", "ant", "anteater", "antelope", "ape", "armadillo", "donkey", "baboon", "badger", "barracuda", "bat", "bear", "beaver", "bee", "bison", "boar", "buffalo", "butterfly", "camel", "capybara", "caribou", "cassowary", "cat", "caterpillar", "cattle", "chamois", "cheetah", "chicken", "chimpanzee", "chinchilla", "chough", "clam", "cobra", "cockroach", "cod", "cormorant", "coyote", "crab", "crane", "crocodile", "crow", "curlew", "deer", "dinosaur", "dog", "dogfish", "dolphin", "dotterel", "dove", "dragonfly", "duck", "dugong", "dunlin", "eagle", "echidna", "eel", "eland", "elephant", "elk", "emu", "falcon", "ferret", "finch", "fish", "flamingo", "fly", "fox", "frog", "gaur", "gazelle", "gerbil", "giraffe", "gnat", "gnu", "goat", "goldfinch", "goldfish", "goose", "gorilla", "goshawk", "grasshopper", "grouse", "guanaco", "gull", "hamster", "hare", "hawk", "hedgehog", "heron", "herring", "hippopotamus", "hornet", "horse", "human", "hummingbird", "hyena", "ibex", "ibis", "jackal", "jaguar", "jay", "jellyfish", "kangaroo", "kingfisher", "koala", "kookabura", "kouprey", "kudu", "lapwing", "lark", "lemur", "leopard", "lion", "llama", "lobster", "locust", "loris", "louse", "lyrebird", "magpie", "mallard", "manatee", "mandrill", "mantis", "marten", "meerkat", "mink", "mole", "mongoose", "monkey", "moose", "mosquito", "mouse", "mule", "narwhal", "newt", "nightingale", "octopus", "okapi", "opossum", "oryx", "ostrich", "otter", "owl", "oyster", "panther", "parrot", "partridge", "peafowl", "pelican", "penguin", "pheasant", "pig", "pigeon", "pony", "porcupine", "porpoise", "quail", "quelea", "quetzal", "rabbit", "raccoon", "rail", "ram", "rat", "raven", "reindeer", "rhinoceros", "rook", "salamander", "salmon", "sandpiper", "sardine", "scorpion", "seahorse", "seal", "shark", "sheep", "shrew", "skunk", "snail", "snake", "sparrow", "spider", "spoonbill", "squid", "squirrel", "starling", "stingray", "stinkbug", "stork", "swallow", "swan", "tapir", "tarsier", "termite", "tiger", "toad", "trout", "turkey", "turtle", "viper", "vulture", "wallaby", "walrus", "wasp", "weasel", "whale", "wildcat", "wolf", "wolverine", "wombat", "woodcock", "woodpecker", "worm", "wren", "yak", "zebra" ];

const inputs = document.querySelector('.input');
const wordsContainer = document.querySelector('.wordsContainer')
let clickSum = 0;
let wordsSum = 0;
let currentWord = 0;
let timeStart = true;
const time = 30; // seconds

function displayWords(array) {
  wordsContainer.innerHTML = array.map( (value) => {return `<span class='word'>${value}</span>`}).join('');
}

function addActive() {
  const spans = document.querySelectorAll('.word');
  spans.item(currentWord).classList.add('active');
}

function removeActive() {
  document.querySelector('.active').classList.remove('active');
}

function onFlyCheck() {
  const span = document.querySelector('.active');
  const wordSpell = span.textContent.split('');
  const inputSpell = this.value.split('');
  const output = wordSpell.map( (value) => `<span>${value}</span>`);

  if (inputSpell.length > wordSpell.length) return;

  inputSpell.map( (value, index) => {
    if (wordSpell[index] === value) output[index] = `<span class="correct">${wordSpell[index]}</span>`;
    else output[index] = `<span class="incorrect">${wordSpell[index]}</span>`
  });
  span.innerHTML = output.join('');
}

function commitWord(e) {
  if (e.keyCode === 32 || e.keyCode === 13) {
    checkWord(this.value.trim());
    this.value = "";
  } 
}

function checkWord(word) {
  const active = document.querySelector('.active');
  active.innerHTML = wordsArray[currentWord];
  
  if (word === wordsArray[currentWord]) {
    active.classList.add('correct');
    wordsSum++;
    clickSum += (wordsArray[currentWord].length);
  } else {
    active.classList.add('incorrect')
  }

  currentWord++;
  removeActive();
  addActive();
  scrollWords();
}

function startTime() {
  if (timeStart) countDown();
  timeStart = false
}

function countDown() {
  setTimeout( endGame, time*1000);
}


function endGame() {
  const cpm = clickSum / time * 60;
  const score = document.querySelector('.score');
  inputs.disabled = true;

  score.innerHTML = `Congratulations! Your score is ... <br>
    <p>${cpm} key press per minute </p>
    <p>${wordsSum} correct word(s). </p> 
    <p> Refresh page (F5) to start again </p>`
}

function shuffle(array) {
  for (let i = array.length; i; i--) {
    let j = Math.floor(Math.random()*i);
    [array[i-1], array[j]] = [array[j], array[i-1]];
  }
}

let offset = 0;

function scrollWords() {
  const active = document.querySelector('.active');

  if (active.offsetTop -offset > active.offsetHeight) {
    offset += active.offsetHeight;
    wordsContainer.style.top =`-${offset}px`;
  }
}

inputs.addEventListener('keyup', onFlyCheck);
inputs.addEventListener('keyup', commitWord);
inputs.addEventListener('keypress', startTime);

shuffle(wordsArray);

displayWords(wordsArray);
addActive();
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>Type speed calculator</title>
</head>
<body>
  <div class="wrapper">
    <div class="container">
      <div class="wordsContainer">Loading sentence...</div>
    </div>
    <input type="text" class="input">
    <div class="score"></div>
  </div>
  
  <script type="text/javascript" src="scripts.js"></script>
</body>

<style>
  body {
    background-color: #ff6c00;
    font-size: 2rem;
  }
  .wrapper{
    width: 50%;
    margin: 75px auto;
    display: block;
  }
  .wordsContainer{
    position: relative;
    transition: all 0.2s; 
  }
  .container{
    text-align: center;
     /* padding: 1rem 2rem 0;  */
    width:100%;
    height: 10rem;
    overflow: hidden;
    margin: 1rem 2rem;
    display: inline-block;
    background-color: #ffc500;
    border-radius: 10px;
  }
  input{
    margin: 0;
    outline: none;
    width: 100%;
    font-size: 1.5rem;
    text-align: center;
    border-radius: 5px;
    border: 1px solid black;
  }
  .currentWord{
    background-color: white;
  }
  .word{
    display: inline-block;
    padding: 0.5rem 1rem;
  }
  .active{
    background-color: white;
    border-radius: .25em;
  }
  .incorrect{
    color: red;
  }
  .correct{
    color: green;
  }
  .score{
    margin: 125px auto;
    text-align: center;
  }
</style>
</html>

\$\endgroup\$
4
\$\begingroup\$

forEach their own

  inputSpell.map( (value, index) => {
    if (wordSpell[index] === value) output[index] = `<span class="correct">${wordSpell[index]}</span>`;
    else output[index] = `<span class="incorrect">${wordSpell[index]}</span>`
  });

Avoid using Array#map for side effects. If you don't care about the return value, use Array#forEach instead; it's more clear to the reader, and doesn't build up a temporary results array.

Minimizing the scope of the conditional could also be nice, depending on your taste:

inputSpell.forEach((value, i) => {
  const word = wordSpell[i];
  output[i] = `<span class="${word === value ? 'correct' : 'incorrect'}">${word}</span>`
});

index is a fine name; I've used i here since the function is so small and it's also a standard name.


Remove the magic

function commitWord(e) {
  if (e.keyCode === 32 || e.keyCode === 13) {

32 and 13 are "magic numbers"; factoring them out into named constants would be good. Plenty of people know that 13 is the enter key, but e.keyCode === KEYS.Enter requires no thought at all.

Also, .active is a class name that appears a handful of times. Once you see it two or three times, it's a good candidate for moving into a variable -- that prevents you from misspelling it and makes it easier to change in the future.


Implicits and organization

function displayWords(array) {
  wordsContainer.innerHTML = array.map( (value) => {return `<span class='word'>${value}</span>`}).join('');
}

Elsewhere you've used the implicit-return syntax, and it works here too. A more descriptive name than array would also be good.

function displayWords(words) {
  wordsContainer.innerHTML = words.map(word => `<span class="word">${word}</span>`).join('');
}

let offset = 0;

This is adrift near the bottom between a bunch of function declarations. Ideally it would go up top with the rest of the variables.


To jump into the HTML for a second, this:

<script type="text/javascript" src="scripts.js"></script>

can safely drop the type attribute; it's assumed by essentially every browser.

Also, the <style> should go in the <head>, either directly as-is or via a <link rel="stylesheet">.


Cohesion

A class may help organize your state more clearly.

Switching over would require

  • prefixing most things with this
  • changing some selections on document to be on the wordsContainer (optional, but in the spirit of reusability)

but it would provide some good encapsulation, more clearly showing what is/affects the game state and what is just utility methods. It would also likely be nicer to work with as you grow more features.

Here's a partial translation to a class approach for the sake of discussion:

const KEYS = Object.freeze({
  Enter: 13,
  Space: 32,
});

function shuffle(array) {
  for (let i = array.length; i; i--) {
    let j = Math.floor(Math.random()*i);
    [array[i-1], array[j]] = [array[j], array[i-1]];
  }
}

class TypingSpeed {
  constructor({ inputSelector, containerSelector }) { 
    this.$input = document.querySelector(inputSelector);
    this.$container = document.querySelector(containerSelector);
    this.wordClass = 'word';
    this.words = TypingSpeed.getWords();
    this.offset = 0;

    this.$input.addEventListener('keyup', this.onFlyCheck.bind(this));
    this.$input.addEventListener('keyup', thus.commitWord.bind(this));
    this.$input.addEventListener('keypress', this.startTime.bind(this));

    shuffle(this.words);

    this.displayWords(this.words);
    this.addActive();
  }

  // Pulled into a method to (a) clear up the constructor
  // and (b) you may want to eg. load from a json file
  static getWords() {
    return [.......];
  }

  displayWords(words) {
    this.$container.innerHTML = words.map(word => `<span class="${this.wordClass}">${word}</span>`).join('');
  }

  addActive() {
    const spans = this.$container.querySelectorAll(`${this.wordClass}`);
    spans.item(this.currentWord).classList.add('active');
  }

  ...
}

Really this looks nice in general. React and friends have their place, but it's fun to see straightforward use of querySelector, template string HTML, etc in a lightweight project like this.

\$\endgroup\$
  • \$\begingroup\$ Thank you BenC for your detail feedback. Class approach is not clear to me, could you advise any good materials? \$\endgroup\$ – Kamil Aug 29 '17 at 14:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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