1
\$\begingroup\$

I have built a super simple tip calculator to help me try and get a hold of the javascript knowledge I've gained. I just wanted to see if there was any feedback I could get for it...

Basic functionality is: If a letter or incorrect character is input in either the bill or the number of persons input, a message should appear at the bottom advising this.

Otherwise if everything is okay, the tip per person is shown at the bottom, followed by the total cost per person!

Here is the JS:

const button = document.querySelector('button'),
    bill = document.querySelector('#amount'),
    serviceRating = document.querySelector('#service'),
    numberPersons = document.querySelector('#people'),
    pTip = document.querySelector('.tip'),
    pTotal = document.querySelector('.total'),
    p = document.querySelectorAll('p'),
    regex = /(^0|[a-zA-Z]+)/i;

let tipAmount = 0,
    totalCostPP = 0;

button.addEventListener('click', () => {
    if (regex.test(bill.value)) {
        pTip.textContent = 'Please give a valid bill value'
    } else if (regex.test(numberPersons.value)) {
        pTip.textContent = 'Please enter how many people were there'
    } else {
        tipAmountPP = tipPP(bill.value, serviceRating.value, numberPersons.value);
        totalCostPP = totalPP(tipAmountPP, bill.value, numberPersons.value);
        for (paragraph of p) {
            paragraph.classList.toggle('hidden');
        }
        pTip.textContent = `Tip per person: $${tipAmountPP.toFixed(2)}`;
        pTotal.textContent = `Total per person: $${totalCostPP.toFixed(2)}`;
    }
});

const tipPP = (bill, service, people) => {
    let tip = (bill / 100) * service;
    return (tip / people);
}

const totalPP = (tip, bill, people) => {
    return (bill / people + tip);
}

I haven't included the HTML and CSS as I didn't see it necessary as everything is functioning with each other just now.

Anyway, appreciate any help you can give, little or large!

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Overall this is good code, but there are several things that could be improved.

Encapsulation

The code should be encapsulated in an IIFE in order to isolate it from other scripts in the page, which, for example, could be using the same variable names.

Also the HTML should be surrounded by an element with a unique identifier (e.g. <div id="tip-calculator"> ... </div>) to allow selecting common elements, such as button or p, but also .tip or .total, which can appear elsewhere on the page outside the context of the calculator and adjust the selectors to represent that.

const button = document.querySelector('#tip-calculator button'),
    bill = document.querySelector('#tip-calculator #amount'), 
    // ...

or

const tipCalculator = document.querySelector('#tip-calculator'),
    button = tipCalculator.querySelector('button'),
    bill = tipCalculator.querySelector('#amount'), 
    // ...

Conventions

It is convention to use one const/let declaration per variable.

(Potential) bugs

The regex used to validate the bill value only forbids the letters A to Z. Any other non-digits or other errors (such as more than one decimal separator) are not caught. The service rating and number of people are not validated at all.

You should be explicitly converting the string values from the input fields into numbers. Currently you are lucky that you are only using multiplication and division on the value, so that the strings are automatically converted, but if you, for example, would addition you'd have an error ("1" + "2" === "12" not 3).

You are toggling the visibility of the paragraphs on each successful calculation, that so every second calculation does not show a result.

Other possible improvements

You are not using the variables tipAmount and totalCostPP outside the event handler (or even the final else block) and their value doesn't change in there, so it would be better to declare them as const inside that block instead of outside the function.

In order to simplify the validation and conversion of the number values you should be using <input type="number"> with appropriate min, max and step attributes. It automatically forces the user only to enter valid numbers and offers the valueAsNumber property which gives you the value already converted to a number.

\$\endgroup\$
1
  • \$\begingroup\$ Thankyou very much for all this, really helpful, will look into this just now. Appreciate the positive words too! \$\endgroup\$ Commented Jul 14, 2020 at 10:04

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