3
\$\begingroup\$

Basically, I am creating a random math generator.

the first line has a random number from 1 to 99 also, the second line has a random number from 1 to 99

and we also generate a random operator (*, +, -),

and the last line bold is the result of the calculation.


on click, we call a function that generates a div with all the logic,

and we used <template> to do something like react but with HTML only.


I see that there is some repetitive things, but not know how to improve, it will be helpful if you help!

enter image description here

class Card {
#CONTAINER = document.querySelector("#container");
#TEMPLATE = document
    .querySelector("template#card")
    .content.querySelector("div")
    .cloneNode(true);

  constructor() {
    this.container = this.#CONTAINER;
    this.template = this.#TEMPLATE;

    this.container.appendChild(this.template);
  }

  get element() {
    return this.template;
  }
}

document.querySelector("#btn").addEventListener("click", () => {
  generateCard();
});

for (let i = 0; i < 5; i++) {
  generateCard()
}

function generateCard() {
  let card = new Card().element;

  let cardOutputs = {
    first: card.querySelector(".first-number"),
    second: card.querySelector(".second-number"),
    result: card.querySelector(".result-number"),
  };

  let arrRandom = [];

  let operationsArray = ["+", "-", "*"];

  let randomOperation =
    operationsArray[Math.round(Math.random() * (operationsArray.length - 1))];

  Object.values(cardOutputs).forEach((output, index) => {
    if (cardOutputs.result !== output) {
      arrRandom.push(Math.round(Math.random() * 100));
      output.textContent = addZeroPrefix(arrRandom[index]);
    } else {
      if (randomOperation === "+") {
        addiction();
      }

      if (randomOperation === "-") {
        if (arrRandom[0] > arrRandom[1]) {
          subtraction();
        } else {
          addiction();
        }
      }

      if (randomOperation === "*") {
        multiplication();
      }

      card.scrollIntoView({
        behavior: "smooth",
      });

      function addiction() {
        card.querySelector(".operator").textContent = "+";
        output.textContent = addZeroPrefix(arrRandom.reduce((a, b) => a + b));
      }

      function subtraction() {
        card.querySelector(".operator").textContent = "-";
        output.textContent = addZeroPrefix(arrRandom.reduce((a, b) => a - b));
      }

      function multiplication() {
        card.querySelector(".operator").textContent = "*";
        output.textContent = addZeroPrefix(arrRandom.reduce((a, b) => a * b));
      }
    }
  });

  function addZeroPrefix(num) {
    if (num < 10) {
      return `0${num}`;
    }

    return num;
  }
}
<!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>
  <script src="https://cdn.tailwindcss.com"></script>
</head>

<body class="bg-blue-200 h-screen grid grid-rows-[1fr_auto]">
  <div id="container" class="container mx-auto py-10 px-4 flex flex-wrap justify-center gap-4  grid-cols-3 overflow-auto"></div>

  <div class="grid place-items-center w-full px-4">
    <button id="btn" class="py-4 w-full max-w-screen-md shadow-sm text-2xl bg-white hover:scale-95 hover:shadow-2xl transition rounded-lg mb-4">generate</button>
  </div>

  <template id="card">
      <div class="h-min p-6 rounded-xl bg-white hover:-translate-y-2 hover:shadow-lg hover: shadow hover:rotate-2 hover:scale-105 transition text-2xl">
        <div>
          <div class="flex gap-4">
            <div class="first-number flex-grow">errpr</div>
            <div class="operator">error</div>
          </div>

          <div class="flex gap-4 border-b-2">
            <div class="second-number flex-grow">error</div>
            <div>=</div>
          </div>

          <div class="result-number font-bold">error</div>
        </div>
      </div>
    </template>

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

</html>

expand the code snippet to full screen to see the button

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Review

Your code is way too complex

The Card object is not a card, rather it is a utility function to copy a template. There is no need for it or you could have implemented it as a singleton with static functions and properties to create cards.

The generate card function has complicated what is just a simple substitution task. There are about 50 lines of code for a task than can be done in a quarter of that.

The HTML template contains unneeded content, Eg <div class="operator">error</div> that has typos, eg <div class="first-number flex-grow">errpr</div>

You can use eval (it is not evil) to evaluate the result of the operation on the two random values. This will greatly reduce the complexity of the source code. see rewrite.

Use String.padStart or String.padEnd to pad strings. This will simplify the function

function addZeroPrefix(num) {
  if (num < 10) {
    return `0${num}`;
  }
  return num;
}

to

const addZeroPrefix = num => (num + "").padStart(2, "0");

Avoid indirect calls You use a function to call a function.

document.querySelector("#btn").addEventListener("click", () => { generateCard(); });

This is redundant and just makes code noisier and thus harder to maintain and read. The line can just be

document.querySelector("#btn").addEventListener("click", generateCard);

Random Bug

You state that the values are from 1 - 99 yet you generate random values via Math.round(Math.random() * 100); which will create values 0 to 100 with the values 0 and 100 being half as likely as any of the other values.

To create a random value within a range and a even distribution you can use something like.

const rnd = (min, max) => Math.floor(Math.random() * (max - min) + min);
const randNumber = rnd(1, 99);

Rewrite

Rewrite uses a static object Card to create a card from a template via the function Card.create. There is no need to instantiate the class.

There are helper functions at the top to simplify and reduce the verbosity of DOM APIs.

The function rnd and rndItem create random values.

generateCard uses Card to create a card and then adds it to the page.

I have remove the scroll into view as I could not see why it is needed. To add a card the button needs to be in view, and if the button can be see the next card will also be in view.

I removed all the CSS styling as you did not provide the CSS (only a indirect reference "https://cdn.tailwindcss.com") and added some basic CSS just for the answer.

Using templates forces you to use classes to reference elements. Ideally element ids are best for referencing elements as it means you do not need to change code if you change the CSS and class names. Personally I would have created the card in code rather than via a template

const qry = (qStr, el = document.body) => el.querySelector(qStr);
const append = (el, ...sibs) => sibs.reduce((p, sib) => (p.appendChild(sib), p), el);
const rnd = (min, max) => Math.floor(Math.random() * (max - min) + min);
const rndItem = items => items[rnd(0, items.length)];
class Card {
    static create() {
        const card = qry("div", qry("#cardTemplate").content).cloneNode(true);
        const num1 = rnd(1, 99) + "", num2 = rnd(1, 99) + "", operator = rndItem("+-*");
        const result = (eval(num1 + operator + num2) + "").padStart(2, "0");
        qry(".first",    card).textContent = num1.padStart(2, "0");
        qry(".second",   card).textContent = num2.padStart(2, "0");
        qry(".result",   card).textContent = result;
        qry(".operator", card).textContent = operator;
        return card;
    }
}
function generateCard() { append(container, Card.create()); }
generateBtn.addEventListener("click", generateCard);
<div id="container"></div>
<div><button id="generateBtn">Generate</button></div>
<template id="cardTemplate">
    <div>
        <div>
            <div class="first"></div>
            <div class="operator"></div>
        </div>
        <div>
            <div class="second"></div>
            <div>=</div>
        </div>
        <div>
            <div class="result"></div>
        </div>
    </div>
</template>

\$\endgroup\$
2
  • \$\begingroup\$ you are genius, you literally make the code better! I need to learn all these topics. like .reduce(), .padStart() etc... you make me even better! thanks @blindman67 \$\endgroup\$ Commented Sep 4, 2022 at 10:19
  • 1
    \$\begingroup\$ yeah very cool developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/…, also the qry function is literally genius! hope you do a good sunday! \$\endgroup\$ Commented Sep 4, 2022 at 10:25
2
\$\begingroup\$

Split the loop

The forEach loop in generateCard does two very different things:

  • If the current object is not the result, then generate a random number and write it as the text content into the object
  • If the current object is the result, then compute the result of the random operation based on previously generated random numbers

This assumes that the result object will be the last one in the iteration.

This is not a good use case for a loop. It would be better to split the loop, and do the necessary operations in a simple sequence of steps:

  1. Generate two random numbers
  2. Compute the result of the random operation on the random numbers

Use else if or switch for mutually exclusive conditions

In this code:

  if (randomOperation === "+") {
    // ...
  }

  if (randomOperation === "-") {
    // ...
  }

  if (randomOperation === "*") {

Only one of the conditions will be true, so these should be chained together using else if or a switch statement.

Use const when possible

Many of the variables declared with let are never reassigned. So it's good to make them const.

Use better names

arrRandom and operationsArray include the term "array" in the name, which is unnecessary. I suggest the more natural, descriptive names randomNums and operations for these.

\$\endgroup\$
3
  • \$\begingroup\$ thanks a lot! really, if I can tomorrow or 2 days in the future (date based on StackOverflow policy) I will add you a bounty of 50/100. very well answered!, for now, accepted and upvote) \$\endgroup\$ Commented Sep 3, 2022 at 20:21
  • 2
    \$\begingroup\$ Hey thanks, I appreciate the sentiment, and it's really not necessary! Keep up the good coding, see you around! (And you may still get more and better answers too!) \$\endgroup\$
    – janos
    Commented Sep 3, 2022 at 21:14
  • \$\begingroup\$ thanks, @janos, now I saw a better answer, but thanks a lot! I will ask my friend to upvote also your answer so you don't lose points, sorry. you do a good job \$\endgroup\$ Commented Sep 4, 2022 at 10:20

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.