15
\$\begingroup\$

Just looking for a little feedback on the below tip calculator I've coded rounding the output to nearest 2 decimal places.

Just wondered if it was readable and code friendly?

bill = float(input("What was the total bill? $"))
tip = float(input("How much tip would you like to give? 10, 12, or 15  "))
no_of_people = float(input("How many people to split the bill?"))

cost_of_tip = bill / 100 * tip
total_cost = (bill + cost_of_tip) / no_of_people
final_per_person = round(total_cost, 2)
final_amount = "{:.2f}".format(final_per_person)

print(f"Each person should pay: ${final_amount}")
\$\endgroup\$
11
  • 1
    \$\begingroup\$ Looks good for beginner code! Very clear. I don't think you need to both round the sum as well as display it with two decimals :2f though, you're kind of doing the same thing twice. \$\endgroup\$
    – ades
    Jan 12 at 19:00
  • 23
    \$\begingroup\$ I once worked for a company that spent a few hundred thousand dollars fixing the "we used float for money` mistake. \$\endgroup\$
    – corsiKa
    Jan 12 at 22:50
  • 4
    \$\begingroup\$ Floats are fine to split a dinner bill though. \$\endgroup\$ Jan 12 at 22:51
  • 6
    \$\begingroup\$ One thing that is not accounted for -- in either question or answers so far -- is that dividing money is tricky business. For example, imagine that I decide to split a $10 bill with 2 other friends: the answer is NOT that we should each pay $3.33 dollars, but instead that 2 should pay $3.33 and one should pay $3.34. Are you interested on feedback on this logic, or are you happy with 3.33 because bill splitting was just an excuse to write some code? \$\endgroup\$ Jan 13 at 16:13
  • 1
    \$\begingroup\$ To make certain not to split cents, use floor to round down. If the sum of final_per_person adds up to less than total_cost then report that x cents are outstanding, or distribute the missing cents randomly among the persons. \$\endgroup\$ Jan 14 at 9:10

4 Answers 4

16
\$\begingroup\$

Confusing variable name. Is the tip an amount or percentage? I assumed the former based on variable name, but a commenter has corrected me.

Keep calculation and presentation logically separate. You don't need to create a variable for the rounded amount. Just compute the per person cost in the natural way. When you are ready to display that value in a human-readable form, format it as needed.

# Get user input.
bill = float(input('Bill: '))
tip_pct = float(input('Tip percentage: '))
npeople = int(input('People: '))  # int() seems more appropriate.

# Calculation.
per_person = bill * (1 + tip_pct / 100.0) / npeople

# Presentation.
message = f'Per person: ${per_person:.2f}'
print(message)

Next step: start putting your code in functions. As you try to write programs longer than a few lines, adopt a strict discipline of putting all code in functions and never relying on global variables (constants are fine). There are many reviews on this site emphasizing such points. The logical divisions illustrated above (collecting input, performing calculations, presenting results) will continue to be relevant as you start using functions.

Another next step: validating and generalizing the collection of user input. Currently, you ask the user for 3 numbers, but people make mistakes. There are many reviews on this site illustrating how to write a simple function taking a message (eg, "Enter the bill") and returning a validated value, with the opportunity to recover from user errors.

\$\endgroup\$
13
  • 2
    \$\begingroup\$ You can infer from the output that the tip is a percentage, not a fixed amount. \$\endgroup\$
    – ades
    Jan 12 at 18:57
  • 6
    \$\begingroup\$ @ades When the user is prompted with "How much tip would you like to give? 10, 12, or 15 ", it's not at all obvious that they are asked for a percentage. Once they receive the output f"Each person should pay: ${final_amount}", yes, they can infer it was a percentage. But that's a bit late. \$\endgroup\$
    – Stef
    Jan 13 at 14:59
  • \$\begingroup\$ If you know how tips work in the US, it's clear this must be a percentage. It would make no sense for $10, $12, or $15 to be the only tip options. \$\endgroup\$
    – nasch
    Jan 14 at 20:42
  • 1
    \$\begingroup\$ The receipts I've seen all ask for a tip amount, not percentage. If anyone is using the software, copying receipt values into the input, they'll go very wrong. Also, it doesn't make too much sense to ask for "10, 12 or 15" when you're not checking the value anyway. Renaming the prompt to "How much tip .. give (percentage)" would definitely improve UI. \$\endgroup\$ Jan 15 at 12:59
  • \$\begingroup\$ @nasch who says this is used in the USA? \$\endgroup\$
    – njzk2
    Jan 15 at 19:22
14
\$\begingroup\$

There's a bug in the logic: If I select 0% tip, it is quite possible that adding the rounded amounts each person pays don't add up to the bill total. You should probably use ceil instead of round, to ensure you round up, never down.

Of course giving a negative tip percentage also makes you not pay the bill total, but that would clearly be user error. It's good to guard against user error, but it is not strictly a bug in the code.

I would suggest though that on input you make it clear that you are looking for a percentage when asking about the tip. "How much tip would you like to give? 10, 12, or 15 " could be interpreted as a dollar amount rather than a percentage.

\$\endgroup\$
10
\$\begingroup\$

Get into the habit of localisation instead of hard-coding currency formats and symbols.

There's not much of a point to asking for a tip among a list of options if you aren't going to validate it. Even if you were to validate it, I would enforce "between 0 and (perhaps) 100".

The tip amount itself isn't used; so just get the total cost directly.

Don't round - let locale.currency choose the number of decimal positions.

"number of people" isn't a float; it's an int.

Suggested

from locale import localeconv, setlocale, LC_ALL, currency

setlocale(LC_ALL, '')
symbol = localeconv()['currency_symbol']

bill = float(input(f"What was the total bill? {symbol}"))
tip = float(input("How much tip would you like to give? %"))
no_of_people = int(input("How many people to split the bill? "))

total_cost = bill * (1 + tip/100)
final_per_person = total_cost / no_of_people
final_amount = currency(final_per_person)

print(f"Each person should pay: {final_amount}")
\$\endgroup\$
7
  • 4
    \$\begingroup\$ I think the localization overshoots the target here and does so inconsequently. What about localizing the prompts? \$\endgroup\$ Jan 13 at 10:05
  • 2
    \$\begingroup\$ @RichardNeumann Which is it - too much ("overshooting") or too little (not enough localisation)? Localisation is not all-or-nothing - it's certainly possible to over-engineer it, but it exists on a continuum. IMO the offered approach is a useful taste for the OP to see how locale-agnostic formatting is done. \$\endgroup\$
    – Reinderien
    Jan 13 at 16:16
  • 4
    \$\begingroup\$ Fair enough. I think that the (partial) localization is too much for the task at hand and too little for actual localization, though. \$\endgroup\$ Jan 13 at 16:23
  • \$\begingroup\$ @RichardNeumann how is it only partial localisation? \$\endgroup\$ Jan 14 at 15:46
  • \$\begingroup\$ @theonlygusti because if all you do is localize the currency, then it's still only suitable for English speaking locales. \$\endgroup\$
    – nasch
    Jan 14 at 20:44
6
\$\begingroup\$

Separate your concerns

Split input parsing from your calculation logic. Read and parse the input in one step and do the calculation in a separate function.

Use fitting data types.

Using float as a data type can have nasty side-effects. Consider using decimal.Decimal instead. Also the three parameters to calculate the bill belong together, since they are only meaningful in their combination for the given use case. Hence, they should be contained in a common data structure.

Use the if __name__ == '__main__': guard

To prevent your script from executing when being imported for re-use in other scripts.

Suggested change

from decimal import Decimal
from typing import NamedTuple


class SplitBill(NamedTuple):
    """A split bill with a tip."""

    bill: Decimal
    tip: int
    people: int

    @property
    def tip_cost(self) -> Decimal:
        """Return the cost of the tip."""
        return self.bill * self.tip / 100

    @property
    def total_cost(self) -> Decimal:
        """Return the total cost with tip."""
        return self.bill + self.tip_cost

    @property
    def cost_per_person(self) -> Decimal:
        """Return the cost for each person."""
        return round(self.total_cost / self.people, 2)


def read_split_bill() -> SplitBill:
    """Interactively reads a split bill."""

    return SplitBill(
        Decimal(input("What was the total bill? $")),
        int(input("How much tip would you like to give? 10, 12, or 15  ")),
        int(input("How many people to split the bill?"))
    )


def main() -> None:
    """Reads the values and calculates a split bill with a tip."""

    split_bill = read_split_bill()
    print(f"Each person should pay: ${split_bill.cost_per_person}")


if __name__ == '__main__':
    main()
\$\endgroup\$
8
  • 2
    \$\begingroup\$ This is good advice, but maybe not suitable for a beginner? \$\endgroup\$ Jan 13 at 14:20
  • 6
    \$\begingroup\$ A class for this is bad \$\endgroup\$ Jan 14 at 15:48
  • \$\begingroup\$ Can you explain the need for creating a class for this? Can you justify that the class, representing a bill that includes a tip (expressed as a percentage), is in charge of reading the input form the user? \$\endgroup\$
    – njzk2
    Jan 15 at 19:25
  • \$\begingroup\$ @theonlygusti Funny, that I gave arguments as to why the class is a good idea and you just counter with a bland statement without giving a reason. Why is a class for this bad? Why do you think that my arguments for a class are invalid or insufficient, Where is your review? \$\endgroup\$ Jan 15 at 20:16
  • 1
    \$\begingroup\$ @njzk2 Indeed, having the class be responsible for reading is suboptimal. A free function for this may be better. As to why I considered a class in the first place, see my post. Within the scope of the problem, the values are only meaningful in combination and hence belong together. \$\endgroup\$ Jan 15 at 20:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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