5
\$\begingroup\$

Could I code differently to slim down the point of this Python source code? The point of the program is to get the user's total amount and add it to the shipping cost. The shipping cost is determined by both country (Canada or USA) and price of product: The shipping of a product that is $125.00 in Canada is $12.00.

input ('Please press "Enter" to begin')

while True: print('This will calculate shipping cost and your grand total.')

totalAmount = int(float(input('Enter your total amount: ').replace(',', '').replace('$', '')))
Country = str(input('Type "Canada" for Canada and "USA" for USA: '))

usa = "USA"
canada = "Canada"
lessFifty = totalAmount <= 50
fiftyHundred = totalAmount >= 50.01 and totalAmount <= 100
hundredFifty = totalAmount >= 100.01 and totalAmount <= 150
twoHundred = totalAmount

if Country == "USA":
    if lessFifty:
        print('Your shipping is: $6.00')
        print('Your grand total is: $',totalAmount + 6)
    elif fiftyHundred:
        print('Your shipping is: $8.00')
        print('Your grand total is: $',totalAmount + 8)
    elif hundredFifty:
        print('Your shipping is: $10.00')
        print('Your grand total is: $',totalAmount + 10)
    elif twoHundred:
        print('Your shipping is free!')
        print('Your grand total is: $',totalAmount)

if Country == "Canada":
    if lessFifty:
        print('Your shipping is: $8.00')
        print('Your grand total is: $',totalAmount + 8)
    elif fiftyHundred:
        print('Your shipping is: $10.00')
        print('Your grand total is: $',totalAmount + 10)
    elif hundredFifty:
        print('Your shipping is: $12.00')
        print('Your grand total is: $',totalAmount + 12)
    elif twoHundred:
        print('Your shipping is free!')
        print('Your grand total is: $',totalAmount)

endProgram = input ('Do you want to restart the program?')
if endProgram in ('no', 'No', 'NO', 'false', 'False', 'FALSE'):
    break
\$\endgroup\$
0

7 Answers 7

10
\$\begingroup\$

I would not hard-code the fee logic, but instead store it as pure data. It's easier to maintain, even allowing to load it from a file. Then, it boils down to a range-based lookup, which is quite classical (cf HLOOKUP in spreadsheet software, with so called "approximate search").

In Python, we can perform such a search via bisect, relying on lexicographic order (and infinity as an unreachable upper bound).

Separated core logic would look like :

from bisect import bisect

#For each country, list of (x,y) = (cost_threshold, fee)
#For first x such cost <= x, pick y for fee.
inf = float("inf")
shippingFees = { 'USA' :    [ (50, 6), (100, 8),  (150, 10), (inf, 0) ],
                 'CANADA' : [ (50, 8), (100, 10), (150, 12), (inf, 0) ]
               }
#Make sure it is sorted (required for lookup via bisect)
#FIXME : Actually it would be better to assert it is already sorted,
#        since an unsorted input might reveal a typo.
for fee in shippingFees.values() : fee.sort()

def getShippingFee(amount, country):
   fees = shippingFees[country.upper()] #raise KeyError if not found.
   idx = bisect(fees, (amount,) )
   return fees[idx][1]

Update

Here is a sample of "working application" using the helper function, assuming you have saved the code snippet above as prices.py (which should be stored in a module, but that's another story).

NB : I have dropped the exit part, since I don't like to type no when I can hit CTRL+C.

#!/usr/bin/python2
""" Your description here """

from prices import getShippingFee

print('This will calculate shipping cost and your grand total.')

while True:

   #TODO : proper input processing, python3 compatible.
   totalAmount = float(raw_input('Enter your total amount: ').replace(',', '').replace('$', ''))
   country = raw_input('Type "Canada" for Canada and "USA" for USA: ').strip().upper()

   try :  #TODO : proper country check.
      shippingFee = getShippingFee(totalAmount, country)
      grandTotal = totalAmount + shippingFee
      if shippingFee :
         print('Your shipping cost is: %.2f' % shippingFee)
      else :
         print('Your shipping is free!')
      print('Your grand total is: %.2f' % grandTotal)

   except KeyError :
      print ("Sorry, we don't ship to this hostile country : %s" % country)
\$\endgroup\$
3
  • \$\begingroup\$ Could you explain more into your script? I don't understand how to make it work into a working application. \$\endgroup\$ Commented Jan 31, 2013 at 12:36
  • \$\begingroup\$ This is a great answer especially in a production environment where the code is liable to be changed by someone other than the original author. Someone could easily guess how to add another country or modify the fees structure. \$\endgroup\$
    – sotapme
    Commented Feb 1, 2013 at 10:42
  • 1
    \$\begingroup\$ @sotapme Thank you ! The main benefit is that the very same data structure can be reused, to generate shipping cost table in web page for instance. This follows DRY principle : Every piece of knowledge must have a single, unambiguous, authoritative representation within a system. \$\endgroup\$
    – YvesgereY
    Commented Feb 1, 2013 at 19:04
5
\$\begingroup\$
  • Your example is broken. I don't think while True: print(...) is what you are looking for.
  • I cannot see why you are eliminating all the commas from the input totalAmount and use int(float(...)) anyway.
  • Your elif twoHundred: will fail for 0.

I suggest to put input validation into a for loop with some error handling. Just as a recommendation.

# Get total amount and country
while True:
    try:
        total_amount = input('Enter your total amount: ')
        total_amount = total_amount.replace('$', '').replace(',', '.')
        total_amount = float(total_amount)
        break
    except ValueError:
        print('Invalid floating point value')

More importantly, it's considered best practice to compare lowercase strings to allow the user to not care about case sensitivity. You are currently not using the usa and canada variables.

usa = "usa"
canada = "canada"

if Country.lower() == usa:
    ...
if Country.lower() == canada:
    ...

if endProgram.lower() in ('no', 'false'):
    break

And Dennis' answer helps as well.

#!/usr/bin/env python3

print('This will calculate shipping cost and your grand total.')
input('Please press "Enter" to begin ')

# Get total amount and country
while True:
    try:
        total_amount = input('Enter your total amount: ')
        total_amount = total_amount.replace('$', '').replace(',', '.')
        total_amount = float(total_amount)
        break
    except ValueError:
        print('Invalid floating point value')

country = input('Type "Canada" for Canada and "USA" for USA: ')

# evaluation of user data
usa = "usa"
canada = "canada"
less_fifty = total_amount <= 50
fifty_hundred = total_amount > 50.0 and total_amount <= 100
hundred_fifty = total_amount > 100.0 and total_amount <= 150

if country == usa:
    if less_fifty:
        shipping = 6
    elif fifty_hundred:
        shipping = 8
    elif hundred_fifty:
        shipping = 10
    else:
        shipping = None

elif country == canada:
    if less_fifty:
        shipping = 8
    elif fifty_hundred:
        shipping = 10
    elif hundred_fifty:
        shipping = 12
    elif twoHundred:
        shipping = None

# print output
if shipping is None:
    print('Your shipping is free!')
    shipping = 0
else:
    print('Your shipping is:    $', shipping)
print('Your grand total is: $', total_amount + shipping)

endProgram = input ('Do you want to restart the program? ')
if endProgram.lower() in ('no', 'false'):
    break
\$\endgroup\$
1
  • \$\begingroup\$ Damn, a duplicate. I don't think it's on purpose, he was told to go here, and his previous question has been automatically migrated too. Voting to close this one, please answer to the other question which has been around for a longer time. \$\endgroup\$ Commented Jan 31, 2013 at 14:20
3
\$\begingroup\$

Just to add that you can do a <= x < y in Python, which to me is a lot easier to write and read than setting up three variables just to say what range the total amount is within:

shipping = None
if total_amount <= 50:
    shipping = 6
elif 50 < total_amount <= 100:
    shipping = 8
elif 100 < total_amount <= 150:
    shipping = 10
if country == 'canada' and shipping:
    shipping += 2
\$\endgroup\$
2
\$\begingroup\$
  • First thing is you don't perform much test on the user input. That's fine if that's just a little tool for yourself but I you plan to make it available for other users, it might be a good idea to have a few more checks.

  • You are storing "USA" and "Canada" in variables which looks like a good idea. However, you don't use it later on. You could do something like : Country = str(input('Type "', canada, '" for Canada and "', usa, '" for USA: ')) and then if Country == canada: and if Country == usa:

  • A string can't be both "Canada" and "Usa". Thus, if Country == canada: could become elif Country == canada:

  • As this "print('Your shipping is: $X.00') print('Your grand total is: $',totalAmount + X)" is repeted many times. It would probably be a could thing to make it a function taking a shippingFee and totalAmount as arguments and doing all the printing for you. Otherwise, instead of having the printing everywhere, you could just update a shippingFee variable and make the printing at the end.

  • I'm not sure that storing the result of the comparison in variables is a really good idea, especially if you give them names containing written numbers : if you want to change one thing, you'll end up changing everything. On top of that, your conditions are dependant on each other : if you've already checked that totalAmount <= 50, if your elif condition, you already know that totalAmount > 50. Thus, your logic could be : if totalAmount <= 50: foo elif totalAmount <=100: bar elif totalAmount <= 150: foobar else: barfoo

  • Other variables have non conventional names. For instance, country would be better than Country

  • It might be interesting to check the amount before considering the country.

In conclusion, the code could be like (it doesn't behave exactly like your and I haven't tested it):

usa = "USA"
canada = "Canada"

input ('Please press "Enter" to begin')
while True:
    print('This will calculate shipping cost and your grand total.')

    totalAmount = int(float(input('Enter your total amount: ').replace(',', '').replace('$', '')))
    country = str(input('Type "', canada, '" for Canada and "', usa, '" for USA: '))
    if country not in (canada, usa):
        print "Invalid country"
        continue

    shippingFee = 0
    if totalAmount<50:
        shippingFee = 6
    elif totalAmount < 100:
        shippingFee = 8
    elif totalAmount < 150:
        shippingFee = 10

    if shippingFee != 0 and country == canada:
        shippingFee = shippingFee+2

    if shippingFee: # this could be using a ternary operator but that's a detail
        print('Your shipping is: Free')
    else:
        print('Your shipping is: $',shippingFee)
    print('Your grand total is: $',totalAmount + shippingFee)

    endProgram = input ('Do you want to restart the program?')
    if endProgram in ('no', 'No', 'NO', 'false', 'False', 'FALSE'):
        break
\$\endgroup\$
2
  • \$\begingroup\$ if shippingFee should probably be if not shippingFee. For consistency, if shippingFee != 0 can also be written as just if shippingFee. Rather than listing all case variations of 'no' and 'false', just use call lower() on input. On a final note, in Python generally underscore_style is preferred over camelCase. \$\endgroup\$ Commented Jan 31, 2013 at 8:57
  • \$\begingroup\$ line 9, in <module> country = str(input('Type "', canada, '" for Canada and "', usa, '" for USA: ')) TypeError: input expected at most 1 arguments, got 5 \$\endgroup\$ Commented Jan 31, 2013 at 9:58
2
\$\begingroup\$

First make sure the script work: proper indenting, shebang, correct logic. When the user enters 170$, you give him free shipping only because twoHundred evaluates to totalAmount which evaluates to true, so it seems simple to just say "otherwise, it's free". You also don't handle errors. I decided to handle the "wrong country" error, but many things could go wrong when inputing the total amount. As other said, first separate printing from application logic:

#!/usr/bin/env python3
#-*- coding: utf-8 -*-

input ('Please press "Enter" to begin')

while True:
  print('This will calculate shipping cost and your grand total.')
  totalAmount = int(float(input('Enter your total amount: ') \
    .replace(',', '').replace('$', '')))
  Country = str(input('Type "Canada" for Canada and "USA" for USA: '))

  lessFifty = totalAmount <= 50
  fiftyHundred = totalAmount >= 50.01 and totalAmount <= 100
  hundredFifty = totalAmount >= 100.01 and totalAmount <= 150

  if Country == "USA":
    if lessFifty:      shippingCost = 6
    elif fiftyHundred: shippingCost = 8
    elif hundredFifty: shippingCost = 10
    else:              shippingCost = 0
  elif Country == "Canada":
    if lessFifty:      shippingCost = 8
    elif fiftyHundred: shippingCost = 10
    elif hundredFifty: shippingCost = 12
    else:              shippingCost = 0
  else:
    print("Unknown country.")
    break

  print('Your shipping cost is: ${:.2f}'.format(shippingCost))
  print('Your grand total is: ${:.2f}'.format(totalAmount + shippingCost))

  endProgram = input ('Do you want to restart the program?')
  if endProgram in ('no', 'No', 'NO', 'false', 'False', 'FALSE'):
    break

You can notice that the if blocks are still repetitive. You can use a dictionary to store the shipping costs.

#!/usr/bin/env python3
#-*- coding: utf-8 -*-

input ('Please press "Enter" to begin')

while True:
  print('This will calculate shipping cost and your grand total.')
  totalAmount = int(float(input('Enter your total amount: ') \
    .replace(',', '').replace('$', '')))
  country = str(input('Type "Canada" for Canada and "USA" for USA: '))

  if country not in ['Canada', 'USA']:
    print("Unknown country.")
    break

  if totalAmount <= 50: amountCategory = 'lessFifty'
  elif totalAmount >= 50.01 and totalAmount <= 100: amountCategory = 'fiftyHundred'
  elif totalAmount >= 100.01 and totalAmount <= 150: amountCategory ='hundredFifty'
  else: amountCategory = 'free'

  shippingCosts = {
      'USA': {'lessFifty': 6, 'fiftyHundred': 8, 'hundredFifty': 10, 'free': 0},
      'Canada': {'lessFifty': 8, 'fiftyHundred': 10, 'hundredFifty': 12, 'free': 0}
  }

  shippingCost = shippingCosts[country][amountCategory]

  print('Your shipping cost is: ${:.2f}'.format(shippingCost))
  print('Your grand total is: ${:.2f}'.format(totalAmount + shippingCost))

  endProgram = input ('Do you want to restart the program?')
  if endProgram in ['no', 'No', 'NO', 'false', 'False', 'FALSE']:
    break

I also used a list instead of a tuple to check for endProgram since it's more idiomatic. I don't think there's much more to do at this point to reduce size. You could find a smarter way to compute the shipping cost (see Kolmogorov complexity) but that would make it harder to change the rules for no good reason. The block which compute amountCategory isn't too nice but since ifs are not expressions in Python you can't write amountCategory = if ...: 'lessFifty' else: 'free'. You could use a function if you wanted.

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

Sure you can condense some things, and improve the input loop. But I find your original code easy to read, whereas this is probably harder to read :

banner                  = 'Now we\'ll calculate your total charge, including shipping.'
cost_prompt             = 'Enter your total purchase amount : '
country_prompt          = 'Where are you shipping to ?\nEnter "USA" for the USA or "Canada" for Canada : '
shippingis              = 'Your shipping is '
grandtotalis            = 'Your grand total is: '
exit_prompt             = 'Type q to quit, or enter to continue: '
shiplaw                 = {
                            'USA' : {
                                0 : 6, 1 : 8, 2 : 10, 3 : 0
                            },
                            'CANADA' : {
                                0 : 8, 1 : 10, 2 : 12, 3 : 0
                            }
                        }

def get_int(prompt):
    try:        return int(float(raw_input(prompt).replace(',','').replace('$','')))
    except:     return None

def dollars_n_cents(*vals):
    return '$' + str(int(sum(vals))) + '.00'

def response(total,shipping):
    shippingstr = 'free!' if shipping == 0 else dollars_n_cents(shipping)
    return (shippingis + shippingstr, grandtotalis + dollars_n_cents(total,shipping))


def run():
    while True:
        print banner
        total_amount = get_int(cost_prompt)
        while total_amount is None: total_amount = get_int(cost_prompt)
        country_str = raw_input(country_prompt)
        while country_str.upper() not in shiplaw: country_str = raw_input(country_prompt)
        result = response(total_amount,shiplaw[country_str.upper()][min(3,total_amount/50)])
        print '\n'.join(result)
        try : 
            if raw_input(exit_prompt)[0] == 'q': break
        except : pass

import sys
if __file__ == sys.argv[0]:
    run()
\$\endgroup\$
0
\$\begingroup\$

One thing you could do is to calculate the shippingCost first, and then print with this logic

if shippingCost = 0
    print: free shipping
else 
    print: your shipping is shippingCost
end
print: grandtotal = totalAmount+shippingCost

It should not be too hard to translate this into proper python.

\$\endgroup\$

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.