Looping through multiple elements and calculating price discount for each-CodePudding

I am trying to find the percentage difference between the old and new price of each individual <div>. At the minute, my code is only pulling through the percentage difference of the first <div> and looping the same percentage difference to the rest of the <div>s. Does anyone know how I can update this to target each individual <div> rather than just looping through the first one? I would like to do this in pure JavaScript; no jQuery please.

const priceDifference = () => {
  const regPrice = document.querySelector('.old').innerText;
  const salePrice = document.querySelector('.new').innerText;

  const priceNumReg = parseFloat(regPrice.substring(1));
  const priceNumSale = parseFloat(salePrice.substring(1));
  const finalPrice = priceNumReg - priceNumSale;

  const percentage = document.querySelectorAll('.percentage');
  const percent = (finalPrice / priceNumReg) * 100;

  for (i = 0; i < percentage.length; i  ) {
    if (percentage[i].textContent.trim() === '') {
      percentage[i].textContent  = ' ('   percent.toFixed(0)   '% OFF)';
    }
  }
};

priceDifference();

<div >
  <div >$14.00</div>
  <div >$6.00</div>
  <div ></div>
</div>

<div >
  <div >$12.00</div>
  <div >$5.00</div>
  <div ></div>
</div>

<div >
  <div >$18.00</div>
  <div >$3.00</div>
  <div ></div>
</div>

CodePudding user response：

You have to loop through each .price element, not each .percentage, since you’re trying to do a calculation based on the scope of a single <div >. Do all the calculations inside the loop, say for the current priceElement (which is one of your <div > elements). Now you can use priceElement.querySelector, instead of document.querySelector. document.querySelector finds the first matching element in the entire document, whereas priceElement.querySelector will find the first matching element inside that scope: priceElement.

So the function would look more like this:

const priceDifference = () => {
  const price = document.querySelectorAll(".price");

  for (i = 0; i < price.length; i  ) {
    const priceElement = price[i];

    const regPrice = priceElement.querySelector(".old").innerText;
    const salePrice = priceElement.querySelector(".new").innerText;
    const percentage = priceElement.querySelector(".percentage");

    const priceNumReg = parseFloat(regPrice.substring(1));
    const priceNumSale = parseFloat(salePrice.substring(1));

    const finalPrice = priceNumReg - priceNumSale;
    const percent = (finalPrice / priceNumReg) * 100;
    
    if (percentage.textContent.trim() === '') {
      percentage.textContent  = ' ('   percent.toFixed(0)   '% OFF)';
    }
  }
};

This code works, but there’s still a bit of work that needs to be done, e.g. the missing declaration for i and innerText vs. textContent, but this code can also be shortened like this:

const parseCurrencyValue = ({ textContent }) => Number.parseFloat(textContent.replace(/^[^- 0-9.]*/u, "")),
  priceDifference = () => Array.from(document.querySelectorAll(".price"))
  .forEach((priceElement) => {
    const [
        regularPrice,
        salePrice
      ] = [ ".old", ".new" ]
        .map((selector) => parseCurrencyValue(priceElement.querySelector(selector))),
      discount = 1 - (salePrice / regularPrice),
      percentageElement = priceElement.querySelector(".percentage");

    if (percentageElement.textContent.trim() === "") {
      percentageElement.insertAdjacentText("beforeend", ` (${Math.round(discount * 100)} % off)`);
    }
  });

priceDifference();

.percentage {
  text-transform: uppercase;
}

<div >
  <div >$14.00</div>
  <div >$6.00</div>
  <div ></div>
</div>

<div >
  <div >$12.00</div>
  <div >$5.00</div>
  <div ></div>
</div>

<div >
  <div >$18.00</div>
  <div >$3.00</div>
  <div ></div>
</div>

I’ve grouped the consts, used some destructuring and template literals, and avoided some code repetition. I’ve used a map over the selectors .new and .old since they need to be treated identically; this is how you would generally avoid code repetition: put everything that is the same into its own set of functions, and iterate only over the stuff that changes, passing it as arguments to the function. The discount calculation can also be simplified.

But most importantly:

You should use Array.from(some NodeList).forEach to iterate over the result of querySelectorAll. This is a bit more robust since you don’t need to mess around with for loop syntax anymore; the syntax is more expressive this way.

You should split the process of parsing text as a price into its own function; that’s parseCurrencyValue in my example. I’ve also used a more robust regular expression that’ll work for strings like "$ 12.99", "USD 12.99", "12.99 $", etc., by removing anything that isn’t part of a number from the start, then using Number.parseFloat.

You should not put “OFF” literally into a string. You’re trying to communicate that something is 50 percent off, so the correct spelling would be “off”. Transforming this into all-caps should be done in CSS; this way it’s more accessible and screen readers don’t get confused by what “O.F.F.” might be.

There might also be some semantic HTML improvements, but this requires a bit more context. Ideally, the data is structured in a table, not in some arbitrary <div>s.