7
\$\begingroup\$

I wanted to share my first experience with JavaScript. I've been taking JS courses for the past 2 months and finally got the first chance to code a small guess-my-number project.

(Everything is completely coded by me and took 4 hours in total. I wanted to test my skills.)

Am I on the right path? Is the code clean enough and what can I possibly improve?

Code is also available as a Codepen Link.

'use strict';

// Generating a random value and storing it in a variable

let secretNumber = Math.floor(Math.random() * 10 + 1);
let score = 20;
let highScore = 0;
document.querySelector('.check').addEventListener('click', function () {
  const guess = Number(document.querySelector('.guess').value);

  document.querySelector('.score').textContent = score;

  // If there is no value in the input

  if (!guess) {
    document.querySelector('.message').textContent = 'Please insert a number';
  } else if (score < 1) {
    document.querySelector('.message').textContent = 'You lost';
    document.querySelector('body').style.backgroundColor = '#FF0000';
  }
  // If the guess is right
  else if (guess === secretNumber) {
    document.querySelector('.message').textContent = 'You Won!!';
    document.querySelector('body').style.backgroundColor = '#008000';
    document.querySelector('.qmark').textContent = secretNumber;
    document.querySelector('.highScore').textContent = score;
  }

  // If the guess is higher than the secret number
  else if (guess > secretNumber) {
    document.querySelector('.message').textContent = 'Too high';
    score--;
  }

  // If the guess is lower than the secret number
  else if (guess < secretNumber) {
    document.querySelector('.message').textContent = 'Too Low!';
    score--;
  }

  // Setting highScore
  else if (score > highScore) {
    highScore = score;
    document.querySelector('.highScore').textContent = highScore;
  }
});

// Reset the game

document.querySelector('.reset').addEventListener('click', function () {
  score = 20;
  secretNumber = Math.floor(Math.random() * 10 + 1);
  document.querySelector('.message').textContent = 'Start Guessing...';
  document.querySelector('.score').textContent = score;
  document.querySelector('.qmark').textContent = '?';
  document.querySelector('.guess').value = '';
  document.querySelector('body').style.backgroundColor = '#043030';
});
* {
  font-family: 'Jura', sans-serif;
  color: azure;
  margin: 0;
  padding: 0;
  width: auto;
}

body {
  background-color: #043030;
  height: 999px;
}

.boxContainer {
  display: flexbox;
  height: 999px;
}

.boxup {
  border-bottom: 6px white solid;
  flex-direction: column;
  height: 400px;
}

.reset {
  font-size: xx-large;
  float: left;
  margin: 30px;
  padding: 10px;
  border-radius: 10px;
  background-color: rgb(1, 7, 7);
}

.numberbtwn {
  float: right;
  margin: 50px;
  font-size: larger;
}

.title {
  position: absolute;
  top: 15%;
  left: 35%;
  font-size: 60px;
}

.qmark {
  background-color: rgb(1, 7, 7);
  position: absolute;
  width: 200px;
  height: 150px;
  left: 45%;
  top: 33%;
  text-align: center;
  font-size: 110px;
}
.boxdown {
  display: flex;
  height: 590px;
}

.left {
  flex-direction: row;
  width: 50%;
  height: 100%;
}

input {
  margin-left: 150px;
  margin-top: 150px;
  margin-right: 368px;
  width: 300px;
  height: 150px;
  background-color: rgb(4, 48, 48);
  border: white 7px solid;
  top: 20%;
  font-size: 150px;
  text-align: center;
}

/* Hiding the up and down arrow from the browsers */

input::-webkit-outer-spin-button,
input::-webkit-inner-spin-button {
  -webkit-appearance: none;
  margin: 0;
}

.check {
  font-size: 40px;
  padding: 30px;
  margin: 50px;
  margin-left: 200px;
  margin-right: 442px;
  border-radius: 20px;
  background-color: rgb(1, 7, 7);
}

.right {
  flex-direction: row;
  width: 50%;
  height: 100%;
}

.message {
  margin: 140px;
  font-size: 50px;
}

p {
  margin: 100px;
  font-size: 30px;
}
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Document</title>
    <link rel="stylesheet" href="stylesheet.css" />
    <link rel="preconnect" href="https://fonts.googleapis.com" />
    <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
    <link
      href="https://fonts.googleapis.com/css2?family=Jura&display=swap"
      rel="stylesheet"
    />
  </head>
  <body>
    <div class="boxContainer">
      <div class="boxup">
        <button class="reset">RESET</button>
        <p class="numberbtwn">Between 1-20</p>
        <h1 class="title">Guess My Number!</h1>
        <div class="qmark">?</div>
      </div>

      <div class="boxdown">
        <div class="left">
          <input class="guess" type="number" />
          <br />
          <button class="check">Check!</button>
        </div>

        <div class="right">
          <p class="message">Start Guessing...</p>
          <p>🔥🔥 Score:<span class="score"> </span></p>
          <p>🏆🏆 Highscore:<span class="highScore"> </span></p>
        </div>
      </div>
    </div>
    <script src="script.js"></script>
  </body>
</html>

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

So a few things I think might be worth considering:

  1. Single writer, multiple reader principle. You have a number of instances where you are writing the textContent of the .message element. While it may not be an issue in smaller programs like this, it can become a real headache trying to debug things when you are writing to them all over the place. I'd suggest making a function that takes the new text content as an argument that you can call. It will make your code more readable and easier to debug. Something along the lines of

    function updateMessage(textContent) = {
       document.querySelector('.message').textContent = textContent
    }
    

    You can call it multiple times, but there is only one place that writes it. This also makes it easier to edit/provide logic if you want to expand on this feature. I would do the same for writing to the other elements.

  2. I think you can get rid of some of your comments - you've done a good job with your variable names. Over commenting can just lead to visual noise, and lead to more things that need to get edited as you are adding/removing feature. I typically strive to make sure that my variable/function names can tell the story instead of needing to rely on comments for that. This makes sure the code is readable and you don't have to worry about out of date comments leading to confusion.

  3. This is a little nitpicky, but it might be good to put a max/min value on the input. You show that guessing range is between 1-20, but I can guess 50. I didn't even see the "Between 1-20" for my first two attempts to use the UI.

  4. Math.floor(Math.random() * 10 + 1) gives a range of 1-10. So the range you are telling users is misleading.

  5. Looking at your class names, it looks like you aren't following a convention. In one case, it looks like you are using camel case for 'boxContainer', but then you just leave every class name lower case. I personally prefer camelCase or kebab-case, but the important thing is that you set and follow a style guide for consistency. In case you aren't familiar with this, here is a good article: Pascal case vs. camel case: What's the difference?

Things that were good:

  1. Your variable names were good, which is a habit that you need to maintain. Bad variable names inevitably lead to bad times. Good names decrease troubleshooting time and increase readability.
  2. I thought the style sheet looked clean, but I'm not the best CSS dev, if someone else comes in to give you some critique, you should listen to them.
  3. CSS can be a real pain - I absolutely swear by flex-box (note, that site is fantastic for most of your CSS reference needs!).
\$\endgroup\$
0
2
\$\begingroup\$

One of the first things I noticed was the number of querySelector requests in your function. Most of the calls were used once or twice, but the DOM call to the .message element was called six times. In case you need to update these elements in the future, it will probably be helpful to set references to these elements outside of the eventListener function.

const guessInput = document.querySelector('.guess');
const scoreElement = document.querySelector('.score');
const messageElement = document.querySelector('.message');
const qmarkElement = document.querySelector('.qmark');

I left the .check and .reset elements alone since they were only referenced once, but references to these could be added in the same way if needed. Additionally, I changed document.querySelector('body') to document.body.


As I was testing this game in the browser, I noticed that the game did not have a way to tell that the game was over since making additional guesses would still respond as if the game was playing. I introduced a gameOver variable which is initially false, set to true when the game is won or lost, and set back to false when the .reset button is pressed.


The .check eventListener function has a lot of if..else blocks which need a bit of optimization. It looks like you want only one of these blocks to execute so I removed the else from all of the else if statements. I also added return; statements at the end of the if (!guess) and if (guess === secretNumber) blocks.

For the other blocks, I omitted a return statement as the other checks could also be useful. For instance, if the guess was wrong and the score was reduced, it might be beneficial to check the new score whether score < 1.


Full code:

'use strict';

// Generating a random value and storing it in a variable
let secretNumber = Math.floor(Math.random() * 20 + 1);
let score = 20;
let highScore = 0;
let gameOver = false;
const guessInput = document.querySelector('.guess');
const scoreElement = document.querySelector('.score');
const messageElement = document.querySelector('.message');
const qmarkElement = document.querySelector('.qmark');
document.querySelector('.check').addEventListener('click', function () {
  if (gameOver) {return;}
  const guess = Number(guessInput.value);

  // If there is no value in the input
  if (!guess) {
    messageElement.textContent = 'Please insert a number';
    return;
  }

  // If the guess is right
  if (guess === secretNumber) {
    messageElement.textContent = 'You Won!!';
    document.body.style.backgroundColor = '#008000';
    qmarkElement.textContent = secretNumber;
    document.querySelector('.highScore').textContent = score;
    gameOver = true;
    return;
  }

  // If the guess is higher than the secret number
  if (guess > secretNumber) {
    messageElement.textContent = 'Too high';
    score--;
  }

  // If the guess is lower than the secret number
  if (guess < secretNumber) {
    messageElement.textContent = 'Too Low!';
    score--;
  }

  if (score < 1) {
    messageElement.textContent = 'You lost';
    document.body.style.backgroundColor = '#FF0000';
    gameOver = true;
  }

  // Set the scoreElement to the updated score
  scoreElement.textContent = score;
});

// Reset the game
document.querySelector('.reset').addEventListener('click', function () {
  score = 20;
  secretNumber = Math.floor(Math.random() * 10 + 1);
  messageElement.textContent = 'Start Guessing...';
  scoreElement.textContent = score;
  qmarkElement.textContent = '?';
  guessInput.value = '';
  document.body.style.backgroundColor = '#043030';
  gameOver = false;
});
* {
  font-family: 'Jura', sans-serif;
  color: azure;
  margin: 0;
  padding: 0;
  width: auto;
}

body {
  background-color: #043030;
  height: 999px;
}

.boxContainer {
  display: flexbox;
  height: 999px;
}

.boxup {
  border-bottom: 6px white solid;
  flex-direction: column;
  height: 400px;
}

.reset {
  font-size: xx-large;
  float: left;
  margin: 30px;
  padding: 10px;
  border-radius: 10px;
  background-color: rgb(1, 7, 7);
}

.numberbtwn {
  float: right;
  margin: 50px;
  font-size: larger;
}

.title {
  position: absolute;
  top: 15%;
  left: 35%;
  font-size: 60px;
}

.qmark {
  background-color: rgb(1, 7, 7);
  position: absolute;
  width: 200px;
  height: 150px;
  left: 45%;
  top: 33%;
  text-align: center;
  font-size: 110px;
}
.boxdown {
  display: flex;
  height: 590px;
}

.left {
  flex-direction: row;
  width: 50%;
  height: 100%;
}

input {
  margin-left: 150px;
  margin-top: 150px;
  margin-right: 368px;
  width: 300px;
  height: 150px;
  background-color: rgb(4, 48, 48);
  border: white 7px solid;
  top: 20%;
  font-size: 150px;
  text-align: center;
}

/* Hiding the up and down arrow from the browsers */

input::-webkit-outer-spin-button,
input::-webkit-inner-spin-button {
  -webkit-appearance: none;
  margin: 0;
}

.check {
  font-size: 40px;
  padding: 30px;
  margin: 50px;
  margin-left: 200px;
  margin-right: 442px;
  border-radius: 20px;
  background-color: rgb(1, 7, 7);
}

.right {
  flex-direction: row;
  width: 50%;
  height: 100%;
}

.message {
  margin: 140px;
  font-size: 50px;
}

p {
  margin: 100px;
  font-size: 30px;
}
<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Document</title>
    <link rel="stylesheet" href="stylesheet.css" />
    <link rel="preconnect" href="https://fonts.googleapis.com" />
    <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
    <link
      href="https://fonts.googleapis.com/css2?family=Jura&display=swap"
      rel="stylesheet"
    />
  </head>
  <body>
    <div class="boxContainer">
      <div class="boxup">
        <button class="reset">RESET</button>
        <p class="numberbtwn">Between 1-20</p>
        <h1 class="title">Guess My Number!</h1>
        <div class="qmark">?</div>
      </div>

      <div class="boxdown">
        <div class="left">
          <input class="guess" type="number" />
          <br />
          <button class="check">Check!</button>
        </div>

        <div class="right">
          <p class="message">Start Guessing...</p>
          <p>🔥🔥 Score:<span class="score"> </span></p>
          <p>🏆🏆 Highscore:<span class="highScore"> </span></p>
        </div>
      </div>
    </div>
    <script src="script.js"></script>
  </body>
</html>

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Lots of notes taken. Appreciate your time :) \$\endgroup\$
    – Chetinn
    Commented Apr 3, 2022 at 23:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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