Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question
add comment

migrated from programmers.stackexchange.com Jan 31 '13 at 13:16

This question came from our site for professional programmers interested in conceptual questions about software development.

7 Answers

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)
share|improve this answer
    
Could you explain more into your script? I don't understand how to make it work into a working application. –  SimplyJordanNET Jan 31 '13 at 12:36
    
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. –  sotapme Feb 1 '13 at 10:42
1  
@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. –  John Optional Smith Feb 1 '13 at 19:04
add comment
  • 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
share|improve this answer
    
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. –  Quentin Pradet Jan 31 '13 at 14:20
add comment

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
share|improve this answer
add comment
  • 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
share|improve this answer
    
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. –  Thijs van Dien Jan 31 '13 at 8:57
    
line 9, in <module> country = str(input('Type "', canada, '" for Canada and "', usa, '" for USA: ')) TypeError: input expected at most 1 arguments, got 5 –  SimplyJordanNET Jan 31 '13 at 9:58
add comment

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.

share|improve this answer
add comment

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()
share|improve this answer
add comment

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.

share|improve this answer
add comment

protected by Malachi Feb 24 at 15:44

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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