Rock, paper, and scissors game

Question

I followed a tutorial that goes through different practical examples and challenges using vanilla Javascript, HTML and CSS. I just completed a simple rock, paper, scissors game; however the tutorial does not really focus on code quality but rather on understanding first. Before I move on to the next challenge I want to identify any issues or bad practices so I can begin developing good coding habits.

// Challenge 3: Rock, Paper, Scissors
function rpsGame(yourChoice) {
  const choices = ['rock', 'paper', 'scissors'];
  let botChoice = choices[randToRpsIndex()];
  let results = isWinner(yourChoice, botChoice);
  modifyFrontEnd(yourChoice, botChoice, results);

  function randToRpsIndex() {
    return Math.floor(Math.random() * 3);
  }

  function isWinner(yourChoice, botChoice) {

    let winners = {
      'rock': 'scissors',
      'paper': 'rock',
      'scissors': 'paper'
    }

    if (botChoice === yourChoice) {
      return [true, true];
    }
    if (botChoice === winners[yourChoice]) {
      return [true, false];
    } else {
      return [false, true]
    }
  }

  function modifyFrontEnd(yourChoice, computerChoice, results) {

    let yourChoiceObj = document.getElementById(yourChoice),
      botChoiceObj = document.getElementById(computerChoice);

    let flexBoxDiv = document.getElementById('flex-box-rps-div');

    // Clear the div
    flexBoxDiv.innerHTML = "";

    // If both choices are the same clone the image
    if (yourChoiceObj == botChoiceObj) {
      botChoiceObj = yourChoiceObj.cloneNode(true);
    }

    yourChoiceObj.id = 'your-choice', botChoiceObj.id = 'bot-choice';

    yourChoiceDiv = document.createElement('div'), botChoiceDiv = document.createElement('div'), messageDiv = document.createElement('div');

    let [yourScore, botScore] = results;
    messageText = document.createElement('h2');

    scores = {
      yourScore,
      botScore
    };
    choiceDivs = {
      yourChoiceDiv,
      botChoiceDiv
    };

    modifyStyle(scores, choiceDivs, messageText);

    yourChoiceDiv.append(yourChoiceObj);
    botChoiceDiv.append(botChoiceObj);
    messageDiv.append(messageText);

    flexBoxDiv.append(yourChoiceDiv, messageDiv, botChoiceDiv);

  }

  function modifyStyle(scores, divs, messageText) {
    messageText.style.fontSize = "20px";

    let {
      yourScore,
      botScore
    } = scores, {
      yourChoiceDiv,
      botChoiceDiv
    } = divs;
    // If player wins
    if (yourScore == true && botScore == false) {
      yourChoiceDiv.style.boxShadow = "10px 10px 10px green";
      botChoiceDiv.style.boxShadow = "10px 10px 10px red";
      messageText.style.color = "green";
      messageText.textContent = "You Won!";
    }

    // If player loses
    else if (yourScore == false && botScore == true) {
      yourChoiceDiv.style.boxShadow = "10px 10px 10px red";
      botChoiceDiv.style.boxShadow = "10px 10px 10px green";
      messageText.style.color = "red";
      messageText.textContent = "You Lost!";
    }

    // If player draws
    else if (yourScore == true && botScore == true) {
      yourChoiceDiv.style.boxShadow = "10px 10px 10px blue";
      botChoiceDiv.style.boxShadow = "10px 10px 10px blue";
      messageText.style.color = "blue";
      messageText.textContent = "You Tied!"
    }

  }
}

.flex-box-container-1,
.flex-box-rps {
  display: flex;
  border: 1px solid black;
  padding: 10px;
  flex-wrap: wrap;
  flex-direction: row;
  justify-content: space-around;
}

.flex-box-container-1 div {
  padding: 10px;
  border: 1px solid black;
  display: flex;
  align-items: center;
}

.container-1 {
  border: 1px solid black;
  align-items: center;
}

.container-2 {
  border: 1px solid red;
  width: 75%;
  margin: 0 auto;
  /*Centralized*/
  text-align: center;
}

.flex-box-container-2 {
  display: flex;
  flex-wrap: wrap;
  justify-content: space-around;
  border: solid blue;
}

.flex-box-container-2 img {
  border: 1px solid green;
  padding: 1px;
  margin: 5px;
}

.flex-box-rps img:hover {
  box-shadow: 0px 10px 50px rgba(37, 50, 223, 1);
}

<!doctype html>

<html lang="en">

<head>
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width, initial-scale=1">
  <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/bootstrap@5.1.3/dist/css/bootstrap.min.css">
  <link rel="stylesheet" href="static/style.css">
  <title>Javascript on Steroids</title>
</head>

<body>
  <div class="container-3">
    <h2>Challenge 3: Rock, Paper, Scissors</h2>
  </div>
  <div class="flex-box-rps" id="flex-box-rps-div">
    <img id="rock" src="https://www.pngkit.com/png/detail/173-1734891_cartoon-rock-clip-art-clipart-clip-art.png" height=150 width=150 alt="" onclick="rpsGame(this.id)">
    <img id="paper" src="https://www.seekpng.com/png/detail/412-4124702_note-clipart-piece-paper-cartoon-document.png" height=150 width=150 alt="" onclick="rpsGame(this.id)">
    <img id="scissors" src="https://cdn5.vectorstock.com/i/1000x1000/36/54/scissors-cartoon-vector-21843654.jpg" height=150 width=150 alt="" onclick="rpsGame(this.id)">
  </div>

  <script src="static/script.js">
  </script>


</body>

</html>

Answer 1

TL;DR

Quality code is easy to reason. Whilst you are in the midst of development you will have a good mental model of what the various parts do, but if you come back to this later, or someone else does, the ability to reason about what the code is trying to achieve, and how it goes about that, is going to be more effort.

The time it takes for someone to build that mental model will be shorter if it's clear from the structure and naming conventions what you are doing.

If you (or someone) is feeling pressure to make a change (fix a bug, add a feature), then a sketch understanding of the code is highly likely to lead to poor quality changes.

I think a key understanding in software development is that whatever you write is likely to need future changes (unless you specifically know it's throw-away), and you'll do yourself and your fellow devs a favour, by making the code easy to reason. It doesn't have to be clever. It often doesn't need to be 'efficient' from an execution perspective. It usually needs to work. It needs to continue working, or be clear about why its not, when subject to change (or even misuse by users).

---

The details...

randToRpsIndex

randToRpsIndex() can take choices.length as a parameter, or as it's a local function, use choices.length in place of the literal 3. I'd prefer the former, as it will make it easier to refactor later if needed. As is, should you decide to extend choices you'd have to remember to change the literal 3.

Either:

function randToRpsIndex(max) {
    return Math.floor(Math.random() * max);
}

Or:

function randToRpsIndex() {
    return Math.floor(Math.random() * choices.length);
}

choices and winners

It's possible to derive choices from winners, which will reduce the risk of errors during later refactoring/updates.

const winners = {
  'rock': 'scissors',
  'paper': 'rock',
  'scissors': 'paper'
}

const choices = Object.keys(winners)

There are other approaches here, such as a single winners definition, then using Object.keys(winners) where you currently use choices, or maybe a local function choices() that internally calls Object.keys... so that you are making it clear to the reader what they keys of winners represents.

The main point here is that if you maintain the same data in two places, there is a risk of divergence as a result of partial changes by someone who doesn't realise the duplication.

isWinner() return value

Personally I dislike returning tuples (use of an array to convey multiple values), as they aren't self documenting. You might be better returning an object:

return { userDidWin: true, botDidWin: false }

And then modifying modifyFrontEnd() accordingly. I think there is still a semantic problem with this, as both winners is normally considered a 'draw', and so it would seem preferable to express that with an enum.

enum {
 Win,
 Lose,
 Draw
}

And then change the semantics of isWinner to something like userResult and again, refactor modifyFrontEnd to do the write thing with the UI.

Further, this object is really 'game state' and maybe you can define a single object that clearly specifies what state the game is in, update that in response to inputs, and use the state to populate the UI.

modifyFrontEnd and modifyStyle

It's a lot simpler to set up the UI at the outset, and then apply CSS styles to the respecting elements depending on the game state (for example pending, win, lose, draw), and then define the CSS styles to give the visual representation. This can easily deal with showing/hiding elements as necessary.

This would make it much easier to understand the UI based on the initial representation, and makes the UI modifications on game state change far easier to understand.

Local functions

It's definitely good practice to modularise code, and functions are one of the simplest ways to turn multiple expressions into discrete statements. With good naming, we don't need to look at the implementation to understand what is going on (at least in a first pass).

The use of local functions (local to the enclosing function) does help to avoid pollution (and potential conflict) with module/global scope symbols of the same name. Given the complexity (as implemented) it's fast approaching the point where other modularisation might be more fitting: either extracting those local functions out in a wider scope (module, file), or packaging the whole thing into a class or similar.

In an ideal world, we want a function to clearly express its intention, be explicit about the input parameters, and its return value(s). The more complex the implementation the more likely we are to want to choose an appropriate level of encapsulation for the parts.

Rather than make specific recommendations on the current code, it would be more appropriate to make any other changes and then see what the shape/complexity of the code is. With practice, this becomes second nature and you'll do it as you go along. It's also good practice to review your code once it's functional, and reconsider the structure to see how it can be simplified (which is, in part, what you've come here for). A second pair of eyes is always invaluable, or at least walking away from it for a few hours if possible, so that you allow your mental model to degrade and then it's easier to see what effort is required to regain it.