1
\$\begingroup\$

Simple console tip calculator app in JavaScript. I would like to get some feedback on improving my code. Thank you beforehand.

Source code:

//header
const headerMessage = () => {
  console.log('Tip Calculator')
}

//service quality
const serviceQuality = (billEntered, userOption ) => {
  const userInput = Number(userOption);
  switch(userInput){
    case 1:
      return (billEntered * 0.3)
      break;
    case 2:
      return(billEntered * 0.2)
      break;
    case 3:
      return(billEntered * 0.1)
      break;
    case 4:
      return(billEntered * 0.05)
      break;
   }
}

const tipCalulcator = () => {
  headerMessage()
  const enterBill = prompt('Please tell me bill amount: $ ');
  console.log(`Bill amount was $ ${enterBill}`)
  console.log(`How was your service ? Please enter number of the options`)
  const enterOption = prompt(`  1) Outstanding (30%) 
  2) Good (20%)
  3) It was ok (10%)
  4) Terrible (5%)`)

  const result = serviceQuality(enterBill, enterOption);
  console.log(`Tip amount ${result}`)
  const total = Number(enterBill) + result;
  console.log(`Overall paycheck ${total}`)
}

tipCalulcator();

\$\endgroup\$
1
  • \$\begingroup\$ Return is not a function. It does not need parenthesis, and you should put a space between it and the expression being returned. \$\endgroup\$ Commented Jan 10, 2020 at 14:30

3 Answers 3

3
\$\begingroup\$

A couple of things to consider:

  1. When dealing with a switch statement, if a case utilizes the return keyword then you do not need to also include a break. When the return is hit, no further code in that function will be reached.

  2. You should handle unexpected inputs; for example, serviceQuality should account for non-numeric inputs, or a number that is not 1-5. Look into try/catch statements and using a default case on your switch.

  3. Because you are not reusing headerMessage, this function is not really required.

  4. Your variable names are fairly unintuitive; consider changing them so that it's more obvious what each thing is such as enterBill -> billAmount and enterOption -> serviceOption. It might be worth using those same names as the serviceQuality arguments too.

\$\endgroup\$
1
  • \$\begingroup\$ To add: Spacing between return and the expressions \$\endgroup\$ Commented Jan 10, 2020 at 14:29
1
\$\begingroup\$

There are a few suggestions I could make, but the one that really stands out is that your serviceQuality function is misnamed. It doesn't return a service quality, it returns a calculated tip amount.

\$\endgroup\$
0
\$\begingroup\$

Additionally to the other comments, i would think about the following:

Magic values vs. constants

The percentage for the tips (30%, 20%, 10%, 5%) are "magic numbers" that are used in the text and in the calculation. In bigger programs, when you have to change such a value (30% to 31%) you can nearly bet, that one of the occurances is missed. The replacement gets even harder, because here the 30% is about the tip, but in other places of the programm (if it gets bigger) you may use the same value for a different purpose. -> That means you can not just search and replace them.
==> Therefor i would use constants for those values. That allows to use the same value at multiple places, but it is still easy changeable. Also the code is much easier to read when there is a "TIP_FOR_OUTSTANDING_QUALITY" instead of a "30%".

Splitting code

Also i would split the code in multiple parts. Currently the "serviceQuality" method is calculating AND converting the user input. When you want to add input validation or more complex convertings, then this gets messy. Therefor i would take the user input. convert it, and then use the clean and save number to feed the serviceQuality method.
You could move it even further by having a method asking for the input, a method for the conversion, one for the calculation and one for the output.

Resumee

My suggestions result in a lot more code. But i think that code is easier to read and to maintain. And the most time is consumed by bugfixes and changes of code. Only a small part is really used for the initial creation.

Or as a rule of thumb: Code is read 100 times more often than it is written -> It makes sense to put quite some effort into the readability :-)

\$\endgroup\$
3
  • \$\begingroup\$ How do you have magic variables? That's impossible. If you define it as a variable, it isn't "magic". \$\endgroup\$ Commented Jan 10, 2020 at 14:26
  • 1
    \$\begingroup\$ Since these numbers are not defined as variables, they are called "magic numbers" \$\endgroup\$ Commented Jan 10, 2020 at 14:34
  • \$\begingroup\$ :-) Stupid me, you are right. Its corrected now. Thanks \$\endgroup\$
    – JanRecker
    Commented Jan 10, 2020 at 15:13

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.