6
\$\begingroup\$

I’m a designer learning to code and I’ve built a simple form calculator for ordering cakes.

I would love it if you could show me how to make the code more idiomatic. How can it be more consistent, concise and simplified? How can I align it with best practices? What’s the right way to group and order it? In what other ways can I make it better?

It doesn’t need to support anything older than ECMAScript 6.

// Cake-ordering calculator

const calculator = document.forms[0]
const total = calculator.elements.total

const occasionOptions = {
  party: 20,
  birthday: 25,
  anniversary: 50,
  wedding: 100
}

const sizeOptions = {
  six: 1,
  eight: 1.5,
  ten: 2,
  twelve: 2.5
}

const extrasOptions = {
  inscription: 10,
  decoration: 25,
  special: 50
}

calculator.addEventListener("click", calculateTotal)

function cake() {
  const cakes = Array.from(calculator.elements["cake"]).slice(0, 3)
  const raphael = calculator.elements.raphael

  function isChecked(checkbox) {
    return checkbox.checked
  }

  let count = cakes.filter(isChecked).length
  if (count) {
    count = count * 0.5 + 0.5
  }
  if (raphael.checked) {
    count += 1
  }
  return count
}

function occasion() {
  let occasionCost = 0
  const occasion = calculator.elements.occasion

  for (let i = 0; i < occasion.length; i++) {
    if (occasion[i].checked) {
      occasionCost = occasionOptions[occasion[i].id]
      break
    }
  }
  return occasionCost
}

function size() {
  let sizeIndex = 1
  const size = calculator.elements.size

  for (let i = 0; i < size.length; i++) {
    if (size[i].checked) {
      sizeIndex = sizeOptions[size[i].id]
      break
    }
  }
  return sizeIndex
}

function extras() {
  let extrasCost = 0
  const extras = calculator.elements.extras

  for (let i = 0; i < extras.length; i++) {
    if (extras[i].checked) {
      extrasCost = extrasCost + extrasOptions[extras[i].id]
    }
  }
  return extrasCost
}

function calculateTotal() {
  let totalCost = cake() * occasion() * size() + extras()
  total.value = "$" + totalCost.toLocaleString("en")
}

// Display "extras" fieldset when "wedding" occasion is selected

const occasions = Array.from(calculator.elements.occasion)
const fieldset = Array.from(document.getElementsByTagName("fieldset"))

fieldset[3].style.display = "none"

occasions.forEach(occasion => {
  occasion.addEventListener("click", () => {
    if (occasion.id == "wedding") {
      fieldset[3].style.setProperty("display", "inherit")
    } else {
      fieldset[3].style.setProperty("display", "none")
    }
  })
})

// Display cost after size has been selected

const sizes = calculator.elements.size

total.style.display = "none"

for (let i = 0; i < sizes.length; i++) {
  sizes[i].onclick = function() {
    if (this.checked) {
      total.style.setProperty("display", "inherit")
    } else {
      total.style.setProperty("display", "none")
    }
  }
}

// Disable all fieldsets except the first one

const disabledFieldsets = document.querySelectorAll(
  "fieldset:not(:first-of-type)"
)
for (let i = 0; i < disabledFieldsets.length; i++) {
  disabledFieldsets[i].disabled = true
}

// Enable fieldsets sequentially on selection

document.querySelectorAll("fieldset").forEach(fieldset => {
  fieldset.addEventListener("change", function() {
    let nextFieldset = this.nextElementSibling

    while (nextFieldset && !nextFieldset.disabled) {
      nextFieldset = nextFieldset.nextElementSibling
    }

    if (nextFieldset) {
      nextFieldset.disabled = false
    }
  })
})

// Reset form after all inputs in the first fieldset are deselected

const cakeOptions = document.querySelectorAll(
  "fieldset:first-of-type input[type=checkbox]"
)
let isChecked = false

cakeOptions.forEach(function(resetWhenAllUnchecked) {
  resetWhenAllUnchecked.addEventListener("click", function(e) {
    if (this.checked) {
      isChecked = true
    } else {
      if (
        isChecked &&
        !document.querySelectorAll(
          "fieldset:first-of-type input[type=checkbox]:checked"
        ).length
      ) {
        calculator.reset()
        fieldset[3].style.setProperty("display", "none")
        total.style.setProperty("display", "none")
      }
    }
  })
})
<form>
<fieldset>
  <legend>Select Cakes</legend>
  <label><input type="checkbox" name="cake" id="leonardo">Leonardo</label>
  <label><input type="checkbox" name="cake" id="donatello">Donatello</label>
  <label><input type="checkbox" name="cake" id="michelangelo">Michelangelo</label>
  <label><input type="checkbox" name="cake" id="raphael">Raphael</label>
  <p>If you select more than one cake, the other cakes are discounted 50%.</p>
  <p><small>Does not apply to Raphael.</small></p>
</fieldset>	

<fieldset>
  <legend>Choose Occasion</legend>
  <label><input type="radio" name="occasion" id="party" required>Party</label>
  <label><input type="radio" name="occasion" id="birthday">Birthday</label>
  <label><input type="radio" name="occasion" id="anniversary">Anniversary</label>
  <label><input type="radio" name="occasion" id="wedding">Wedding</label>
</fieldset>

<fieldset>
  <legend>Choose Size</legend>
  <label><input type="radio" name="size" id="six" required>6-inch</label>
  <label><input type="radio" name="size" id="eight">8-inch</label>
  <label><input type="radio" name="size" id="ten">10-inch</label>
  <label><input type="radio" name="size" id="twelve">12-inch</label>
</fieldset>

<fieldset>
  <legend>Select Extras</legend>
  <label><input type="checkbox" name="extras" id="inscription">Inscription</label>
  <label><input type="checkbox" name="extras" id="decoration">Decoration</label>
  <label><input type="checkbox" name="extras" id="special">Special Frosting & Icing</label>
</fieldset>

<input type="text" name="total" readonly>
<input type="submit" value="Submit">
</form>

\$\endgroup\$
0

2 Answers 2

3
+100
\$\begingroup\$

Feedback

I see originally the code utilized document.querySelector("form"); to get the form element, but has since been changed to utilize document.forms[0]. I had planned to mention that using that property can be quicker (since a function doesn't need to be called) but now I don't have to.

Suggestions

Spread operator

Because is utilized, the spread syntax can be used to add the HTML elements. For example, instead of using Array.from():

const cakes = Array.from(calculator.elements["cake"]).slice(0, 3)

Create an array and use the spread operator to put the elements into an array:

const cakes = [...calculator.elements["cake"]].slice(0, 3);

The same is also true for occasions and fieldsets.

Arrow functions

Another feature that can be used to simplify things is arrow functions, which is actually used in the click handler for the occasion options. For example, the function isChecked:

function isChecked(checkbox) {
  return checkbox.checked
}

Can be simplified to an arrow function:

const isChecked = checkbox => checkbox.checked;

This could actually be done to all functions if so desired, but beware that hoisting won't apply to arrow functions declared as a function expression.

Wait for DOM to be ready

I thought I mentioned this in my answer to your previous post but realize now that I hadn't (so I had to update my answer): it is best to wait for the DOM to be ready before performing DOM queries. One way to do this is to use document.addEventListener to add a callback function for the DOMContentLoaded event:

document.addEventListener("DOMContentLoaded", function(event) {
    //code to execute now that the DOM is ready
});

Cache DOM References

As I mentioned in my answer to your previous post, DOM references can be stored in variables once to avoid repeated lookups. In the code here, cakes and raphael are declared within the cakes function, so every time that function runs it queries the DOM for those elements.

Function names

The function cake might be better named something like getCakeCount or getCakeMultiplier, since it is returning a number.

Variable names

The forEach iterator at the end has a currentValue argument named resetWhenAllUnchecked :

cakeOptions.forEach(function(resetWhenAllUnchecked) {

resetWhenAllUnchecked sounds like a boolean value. A more appropriate name would be cakeOption.

Early returns from functions

The function occasion could be simplified from:

function occasion() {
  let occasionCost = 0
  const occasion = calculator.elements.occasion

  for (let i = 0; i < occasion.length; i++) {
      if (occasion[i].checked) {
        occasionCost = occasionOptions[occasion[i].id]
        break
      }
    }
  return occasionCost
}

Instead of creating occasionCost, updating it if an option is checked and then returning that value, one could return as soon as a found item is checked. That way there is no need to break out of the for loop nor update the return value. Some may argue that it is better to have a single return statement at the end of the function but that is a preference/convention that must be agreed upon.

function occasion() {
  const occasion = calculator.elements.occasion

  for (let i = 0; i < occasion.length; i++) {
    if (occasion[i].checked) {
      return occasionOptions[occasion[i].id];
    }
  }
  return 0;
}

Rewrite

See code below using advice from suggestions above. I feel there are still some simplifications that could be done to the functions toward the end - perhaps that can be an exercise to the reader to do.

document.addEventListener('DOMContentLoaded', _ => {
  //DOM references
  const calculator = document.forms[0]
  const total = calculator.elements.total
  const firstThreeCakeOptions = [...calculator.elements["cake"]].slice(0, 3)
  const raphael = calculator.elements.raphael
  const cakeOptions = document.querySelectorAll(
    "fieldset:first-of-type input[type=checkbox]"
  )
  const occasions = [...calculator.elements.occasion]
  const fieldset = [...document.getElementsByTagName("fieldset")]

  const occasionOptions = {
    party: 20,
    birthday: 25,
    anniversary: 50,
    wedding: 100
  }

  const sizeOptions = {
    six: 1,
    eight: 1.5,
    ten: 2,
    twelve: 2.5
  }

  const extrasOptions = {
    inscription: 10,
    decoration: 25,
    special: 50
  }

  calculator.addEventListener("click", calculateTotal)

  function cake() {
    const isChecked = checkbox => checkbox.checked;

    let count = firstThreeCakeOptions.filter(isChecked).length
    if (count) {
      count = count * 0.5 + 0.5
    }
    if (raphael.checked) {
      count += 1
    }
    return count
  }

  function occasion() {
    const occasion = calculator.elements.occasion

    for (let i = 0; i < occasion.length; i++) {
      if (occasion[i].checked) {
        return occasionOptions[occasion[i].id]
      }
    }
    return 0
  }

  function size() {
    const size = calculator.elements.size

    for (let i = 0; i < size.length; i++) {
      if (size[i].checked) {
        return sizeOptions[size[i].id]
      }
    }
    return 1
  }

  function extras() {
    let extrasCost = 0
    const extras = calculator.elements.extras

    for (let i = 0; i < extras.length; i++) {
      if (extras[i].checked) {
        extrasCost = extrasCost + extrasOptions[extras[i].id]
      }
    }
    return extrasCost
  }

  function calculateTotal() {
    const totalCost = cake() * occasion() * size() + extras()
    total.value = "$" + totalCost.toLocaleString("en")
  }

  // Display "extras" fieldset when "wedding" occasion is selected


  fieldset[3].style.display = "none"

  occasions.forEach(occasion => {
    occasion.addEventListener("click", () => {
      if (occasion.id == "wedding") {
        fieldset[3].style.setProperty("display", "inherit")
      } else {
        fieldset[3].style.setProperty("display", "none")
      }
    })
  })

  // Display cost after size has been selected

  const sizes = calculator.elements.size

  total.style.display = "none"

  for (let i = 0; i < sizes.length; i++) {
    sizes[i].onclick = function() {
      if (this.checked) {
        total.style.setProperty("display", "inherit")
      } else {
        total.style.setProperty("display", "none")
      }
    }
  }

  // Disable all fieldsets except the first one

  const disabledFieldsets = document.querySelectorAll(
    "fieldset:not(:first-of-type)"
  )
  for (let i = 0; i < disabledFieldsets.length; i++) {
    disabledFieldsets[i].disabled = true
  }

  // Enable fieldsets sequentially on selection

  document.querySelectorAll("fieldset").forEach(fieldset => {
    fieldset.addEventListener("change", function() {
      let nextFieldset = this.nextElementSibling

      while (nextFieldset && !nextFieldset.disabled) {
        nextFieldset = nextFieldset.nextElementSibling
      }

      if (nextFieldset) {
        nextFieldset.disabled = false
      }
    })
  })

  // Reset form after all inputs in the first fieldset are deselected


  let isChecked = false

  cakeOptions.forEach(function(resetWhenAllUnchecked) {
    resetWhenAllUnchecked.addEventListener("click", function(e) {
      if (this.checked) {
        isChecked = true
      } else {
        if (
          isChecked &&
          !document.querySelectorAll(
            "fieldset:first-of-type input[type=checkbox]:checked"
          ).length
        ) {
          calculator.reset()
          fieldset[3].style.setProperty("display", "none")
          total.style.setProperty("display", "none")
        }
      }
    })
  })
});
<form>
  <fieldset>
    <legend>Select Cakes</legend>
    <label><input type="checkbox" name="cake" id="leonardo">Leonardo</label>
    <label><input type="checkbox" name="cake" id="donatello">Donatello</label>
    <label><input type="checkbox" name="cake" id="michelangelo">Michelangelo</label>
    <label><input type="checkbox" name="cake" id="raphael">Raphael</label>
    <p>If you select more than one cake, the other cakes are discounted 50%.</p>
    <p><small>Does not apply to Raphael.</small></p>
  </fieldset>

  <fieldset>
    <legend>Choose Occasion</legend>
    <label><input type="radio" name="occasion" id="party" required>Party</label>
    <label><input type="radio" name="occasion" id="birthday">Birthday</label>
    <label><input type="radio" name="occasion" id="anniversary">Anniversary</label>
    <label><input type="radio" name="occasion" id="wedding">Wedding</label>
  </fieldset>

  <fieldset>
    <legend>Choose Size</legend>
    <label><input type="radio" name="size" id="six" required>6-inch</label>
    <label><input type="radio" name="size" id="eight">8-inch</label>
    <label><input type="radio" name="size" id="ten">10-inch</label>
    <label><input type="radio" name="size" id="twelve">12-inch</label>
  </fieldset>

  <fieldset>
    <legend>Select Extras</legend>
    <label><input type="checkbox" name="extras" id="inscription">Inscription</label>
    <label><input type="checkbox" name="extras" id="decoration">Decoration</label>
    <label><input type="checkbox" name="extras" id="special">Special Frosting & Icing</label>
  </fieldset>

  <input type="text" name="total" readonly>
  <input type="submit" value="Submit">
</form>

\$\endgroup\$
2
  • \$\begingroup\$ Hey Sam, I found a problem. If you choose "wedding", then select any of the "extras", and afterwords choose "party", "birthday", or "anniversary", "extras" are still added to the total cost instead of being subtracted. I know how to uncheck the "extras" checkboxes on "wedding" deselection – extras.forEach(extra => extra.checked = false) – but I would like to remember their checked state in case "wedding" is selected again. \$\endgroup\$
    – Tzar
    Aug 24, 2018 at 7:49
  • 1
    \$\begingroup\$ My initial thought is to say one option is to return 0 in the extras() function whenever the occasion is not wedding, but then after considering the Separation of Concerns principle I wonder if that logic should be put into calculateTotal()... \$\endgroup\$ Aug 25, 2018 at 13:12
0
\$\begingroup\$

Here is the rewritten code with @SamOnela’s suggestions applied, and many other little modifications.

document.addEventListener("DOMContentLoaded", _ => {
  //DOM references

  const calculator = document.forms[0]
  const total = calculator.elements.total
  const firstThreeCakeOptions = [...calculator.elements.cake].slice(0, 3)
  const size = calculator.elements.size
  const occasions = [...calculator.elements.occasion]
  const fieldset = [...document.getElementsByTagName("fieldset")]
  const extrasFieldset = fieldset[3]

  // Prices

  const occasionOptions = {
    party: 20,
    birthday: 25,
    anniversary: 50,
    wedding: 100
  }

  const sizeOptions = {
    six: 1,
    eight: 1.5,
    ten: 2,
    twelve: 2.5
  }

  const extrasOptions = {
    inscription: 10,
    decoration: 25,
    special: 50
  }

  // Calculator logic

  calculator.addEventListener("click", calculateTotal)

  function cakeMultiplier() {
    const size = calculator.elements.size
    const isChecked = checkbox => checkbox.checked

    let count = firstThreeCakeOptions.filter(isChecked).length
    if (count) {
      count = count * 0.5 + 0.5
    }
    if (raphael.checked) {
      count += 1
    }
    return count
  }

  function occasionCost() {
    const occasion = calculator.elements.occasion

    for (let i = 0; i < occasion.length; i++) {
      if (occasion[i].checked) {
        return occasionOptions[occasion[i].id]
      }
    }
    return 0
  }

  function sizeCost() {
    for (let i = 0; i < size.length; i++) {
      if (size[i].checked) {
        return sizeOptions[size[i].id]
      }
    }
    return 1
  }

  function extrasCost() {
    const extras = calculator.elements.extras
    let extrasCost = 0

    for (let i = 0; i < extras.length; i++) {
      if (extras[i].checked) {
        extrasCost = extrasCost + extrasOptions[extras[i].id]
      }
    }
    return extrasCost
  }

  function calculateTotal() {
    const totalCost =
      cakeMultiplier() * occasionCost() * sizeCost() + extrasCost()
    total.value = "$" + totalCost.toLocaleString("en")
  }

  // Display "extras" fieldset when "wedding" occasion is selected

  extrasFieldset.style.display = "none"

  occasions.forEach(occasion => {
    occasion.addEventListener("click", () => {
      if (occasion.id == "wedding") {
        extrasFieldset.style.setProperty("display", "inherit")
      } else {
        extrasFieldset.style.setProperty("display", "none")
      }
    })
  })

  // Display cost after size has been selected

  total.style.display = "none"

  size.forEach(i => {
    i.addEventListener("click", function(e) {
      if (this.checked) {
        total.style.setProperty("display", "inherit")
      }
    })
  })

  // Disable all fieldsets except the first one

  function disableFieldsets() {
    const disabledFieldsets = document.querySelectorAll(
      "fieldset:not(:first-of-type)"
    )

    for (let i = 0; i < disabledFieldsets.length; i++) {
      disabledFieldsets[i].disabled = true
    }
  }

  disableFieldsets()

  // Enable fieldsets sequentially on selection

  document.querySelectorAll("fieldset").forEach(fieldset => {
    fieldset.addEventListener("change", function() {
      let nextFieldset = this.nextElementSibling

      while (nextFieldset && !nextFieldset.disabled) {
        nextFieldset = nextFieldset.nextElementSibling
      }

      if (nextFieldset) {
        nextFieldset.disabled = false
      }
    })
  })

  // Reset form after all inputs in the first fieldset are deselected

  let isChecked = false

  document
    .querySelectorAll("fieldset:first-of-type input[type=checkbox]")
    .forEach(function(cakeOption) {
      cakeOption.addEventListener("click", function(e) {
        if (this.checked) {
          isChecked = true
        } else {
          if (
            isChecked &&
            !document.querySelectorAll(
              "fieldset:first-of-type input[type=checkbox]:checked"
            ).length
          ) {
            calculator.reset()
            disableFieldsets()
            extrasFieldset.style.setProperty("display", "none")
            total.style.setProperty("display", "none")
          }
        }
      })
    })
})
<form>
<fieldset>
<legend>Select Cakes</legend>
  <input type=checkbox name=cake id=leonardo>
  <label for=leonardo>Leonardo</label>
  
  <input type=checkbox name=cake id=donatello>
  <label for=donatello>Donatello</label>
  
  <input type=checkbox name=cake id=michelangelo>
  <label for=michelangelo>Michelangelo</label>
  
  <input type=checkbox name=cake id=raphael>
  <label for=raphael>Raphael</label>
  
  <p>If you select more than one cake, the other cakes are discounted 50%.
  <p><small>Does not apply to Raphael.</small>
</fieldset> 

<fieldset>
<legend>Choose Occasion</legend>
  <input type=radio name=occasion id=party required>
  <label for=party>Party</label>
  
  <input type=radio name=occasion id=birthday>
  <label for=birthday>Birthday</label>
  
  <input type=radio name=occasion id=anniversary>
  <label for=anniversary>Anniversary</label>
  
  <input type=radio name=occasion id=wedding>
  <label for=wedding>Wedding</label>
</fieldset>

<fieldset>
<legend>Choose Size</legend>
  <input type=radio name=size id=six required>
  <label for=six>6-inch</label>
  
  <input type=radio name=size id=eight>
  <label for=eight>8-inch</label>
  
  <input type=radio name=size id=ten>
  <label for=ten>10-inch</label>
  
  <input type=radio name=size id=twelve>
  <label for=twelve>12-inch</label>
</fieldset>

<fieldset>
<legend>Select Extras</legend>
  <input type=checkbox name=extras id=inscription>
  <label for=inscription>Inscription</label>
  
  <input type=checkbox name=extras id=decoration>
  <label for=decoration>Decoration</label>
  
  <input type=checkbox name=extras id=special>
  <label for=special>Special Frosting & Icing</label>
</fieldset>

<input type=text name=total readonly>
<input type=submit value=Submit>
</form>

\$\endgroup\$
3
  • \$\begingroup\$ could you please elaborate a bit on the "many other little modifications"? It's a bit difficult to see what you did without a diffing tool. Thanks :) \$\endgroup\$
    – Vogel612
    Aug 21, 2018 at 7:07
  • \$\begingroup\$ @Vogel612 First and foremost, I applied Sam’s suggestions, then I tidied up the JavaScript code by reordering it and renaming a few things, and I also made the HTML a bit more concise and more readable. \$\endgroup\$
    – Tzar
    Aug 21, 2018 at 7:13
  • \$\begingroup\$ The code snippet in my answer should be the starting point. \$\endgroup\$
    – Tzar
    Aug 21, 2018 at 7:17

Your Answer

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

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