I've been taking the Computer Science course on Codecademy. The course includes this project where we have to create a python script to find the cheapest shipping method based on the weight of an item and I'm just wondering if this is the best way to code the solution?

def ground_shipping_cost(weight):
 flat_cost = 20
 premium_cost = 125
 if weight <= 2:
 flat_cost += weight * 1.50
 elif weight > 2 and weight <= 6:
 flat_cost += weight * 3.00
 elif weight > 6 and weight <= 10:
 flat_cost += weight * 4.00
 elif weight > 10:
 flat_cost += weight * 4.75
 return flat_cost, premium_cost


def drone_shipping_cost(weight):
 cost = 0
 if weight <= 2:
 cost = weight * 4.50
 elif weight > 2 and weight <= 6:
 cost = weight * 9.00
 elif weight > 6 and weight <= 10:
 cost = weight * 12.00
 elif weight > 10:
 cost = weight * 14.25
 return cost


def cheapest_shipping(weight):
 ground_cost, premium_cost = ground_shipping_cost(weight)
 drone_cost = drone_shipping_cost(weight)
 if drone_cost < ground_cost and drone_cost < premium_cost:
 return "You should use drone shipping as it will only cost " + str(drone_cost)
 if ground_cost < drone_cost and ground_cost < premium_cost:
 return "You should use standard ground shipping as it will only cost " + str(ground_cost)
 if premium_cost < ground_cost and premium_cost < drone_cost:
 return "You should use premium shipping as it will only cost " + str(premium_cost)


print(cheapest_shipping(4.8)) # You should use standard ground shipping as it will only cost 34.4 
print(cheapest_shipping(41.5)) # You should use premium shipping as it will only cost 125
print(cheapest_shipping(1.5)) # You should use drone shipping as it will only cost 6.75
print(cheapest_shipping(4.0)) # You should use standard ground shipping as it will only cost 32.0

The issue I see is maybe there's too many if, elif statements and maybe there is a way to simplify the code

Welcome to CodeReview! Having others review your code is one of the very best ways to find bugs and improve your coding. And we're going to improve your coding, no matter how much it hurts! :-)

I'm going to repeat many of the points made in Maarten's review, although with slightly different results.

From your post, I know you're still learning. From your code, I believe you have learned: if/elif/else statements, Python tuple types, and functions. So I'm going to focus on these in any improvements.

else means 'Not true'

The most glaring problem, and one you intuited a little yourself, is that you are doing "else" wrong. Consider:

if weight <= 2:
 flat_cost += weight * 1.50
elif weight > 2 and weight <= 6:
 flat_cost += weight * 3.00
elif weight > 6 and weight <= 10:

In this sequence, you first check if weight <= 2. Now pretend that if statement fails. What do you know? You know that if any one of the else statements executes, then weight must be > 2 because otherwise the if statement would have executed!

So never "test" something you know to be true. Or false. If you know a thing, you don't need to test it. (You might assert it for sanity checking, but that's different.)

Note: With compound statements, like if A and B you might have to (re) test one of the statements when the compound fails:

if A and B:
elif A:

But that's technically different because they are different conditions.

So let's rewrite your conditions:

def drone_shipping_cost(weight):
 cost = 0
 if weight <= 2:
 cost = weight * 4.50
 elif weight > 2 and weight <= 6:
 cost = weight * 9.00
 elif weight > 6 and weight <= 10:
 cost = weight * 12.00
 elif weight > 10:
 cost = weight * 14.25
 return cost

Becomes:

def drone_shipping_cost(weight):
 cost = 0
 if weight <= 2:
 cost = weight * 4.50
 elif weight <= 6:
 cost = weight * 9.00
 elif weight <= 10:
 cost = weight * 12.00
 else:
 cost = weight * 14.25
 return cost

Note two things: first, the weight > 10 case becomes a blanket else statement, since you are covering all the possible numbers; and second, there's no reason to set cost = 0 initially, since you cover all possible numbers:

def drone_shipping_cost(weight):
 if weight <= 2:
 cost = weight * 4.50
 elif weight <= 6:
 cost = weight * 9.00
 elif weight <= 10:
 cost = weight * 12.00
 else:
 cost = weight * 14.25
 return cost

Keep separate things separate

You could rewrite your ground_shipping_cost function in a similar way, but let's take a harder look at that:

def ground_shipping_cost(weight):
 flat_cost = 20
 premium_cost = 125
 if weight <= 2:
 flat_cost += weight * 1.50
 elif weight > 2 and weight <= 6:
 flat_cost += weight * 3.00
 elif weight > 6 and weight <= 10:
 flat_cost += weight * 4.00
 elif weight > 10:
 flat_cost += weight * 4.75
 return flat_cost, premium_cost

You're doing a couple of things wrong here. First, you're "accumulating" when you should be "adding". And second, you're returning a tuple just to get the premium cost. In reality, the premium shipping cost is another form of shipping.

Let's get the low-hanging-fruit out of the way:

def premium_shipping_cost(weight):
 ''' Compute cost of premium shipping for a package. '''
 return 125

That was easy, wasn't it!

Now, let's remove the premium_cost from the ground shipping, and fix the if/else statements:

def ground_shipping_cost(weight):
 flat_cost = 20
 if weight <= 2:
 flat_cost += weight * 1.50
 elif weight <= 6:
 flat_cost += weight * 3.00
 elif weight <= 10:
 flat_cost += weight * 4.00
 else:
 flat_cost += weight * 4.75
 return flat_cost

This looks better, but you're still "accumulating" instead of "adding." In this case, that's the wrong thing to do because you're only going to add one thing. Phrasing the computations as accumulating costs gives the wrong impression to the reader. Let's make it clear that there's a flat charge and a by-weight charge:

def ground_shipping_cost(weight):
 ''' Compute cost of ground shipping for a package. '''
 flat_cost = 20
 if weight <= 2:
 weight_charge = weight * 1.50
 elif weight <= 6:
 weight_charge = weight * 3.00
 elif weight <= 10:
 weight_charge = weight * 4.00
 else:
 weight_charge = weight * 4.75
 return flat_cost + weight_charge

This version makes it clear that there's a flat cost and a weight charge. Future-Stephen will thank you.

You make the call!

Now here's where I'll diverge from Maarten Fabre's review. The DRY principle tells us that those two chains of if/elif/else statements should be moved into a separate function.

First I have to ask, what's the objective? If you are in part of a class where they are focused on writing functions, then that is absolutely right and you should do it.

But if you are in a part of the class where they are starting to focus on classes and objects, and encapsulating behavior, then maybe it would be the wrong thing to do. Why? Because maybe the weights and cost multipliers were the same only by coincidence, and maybe the next thing they ask will be for you to separate them!

So you have to use your own judgement. You could write a function that returns the cost multiplier. You could write a function that returns a cost "category" and use that to look up the multiplier. Or you could leave the two cost functions with duplicated code, so that you can change the cost layers or cost multipliers independently.

One built-in function

Python has a built-in function called min. By default, min compares objects in a sequence, or objects passed as positional parameters. It compares them using the default Python comparison, and for tuples that comparison compares the elements of the tuple in ascending order. This is discussed nicely in this SO answer.

What this means for you is that you can use min on a sequence of tuple values, in different ways:

• You could compute name, cost tuples for the shipping types, and find the lowest cost.


• You could store name, cost-function tuples and compute the lowest cost using a special key function.

Let's try the most direct approach, since it's easier to understand. And let's put the cost before the name, so that we get the right results (comparison is in tuple order!):

def cheapest_shipping(weight):
 ''' Determine the cheapest shipping method for a package. '''
 drone_cost = drone_shipping_cost(weight)
 ground_cost = ground_shipping_cost(weight)
 premium_cost = premium_shipping_cost(weight)

 cheapest_tuple = min((drone_cost, 'drone shipping'),
 (ground_cost, 'ground shipping'),
 (premium_cost, 'premium shipping'))
 return cheapest_tuple

At this point, you can do what is called "tuple unpacking" (remember the word "unpacking" -- you'll want to search for it later). That lets you return multiple values into multiple separate variables:

cost, name = cheapest_shipping(weight)
print(f"You should use name, it costs only cost!")

premium_cost

There is no reason to include the return of this premium_cost in ground_shipping_cost. Use a different method here

DRY

You are correct. There are too many if statements. If you want to introduce an new threshold for the cost, this will include a adding an extra clause, and changing the limits for the preceding and following thresholds. This is just waiting for errors to pop up. Better would be to keep a dict of the thresholds, and iterate over them to get this price factor:

def ground_shipping_cost(weight):
 thresholds = 2: 1.5, 6: 3.0, 10: 4.0, float("inf"): 4.75
 flat_cost = 20
 for threshold, factor in sorted(thresholds.items()):
 if weight <= threshold:
 break
 return flat_cost + weight * factor

thresholds is a dict, with the cost per weight unit as value, and the threshold as key.

The drone_shipping_cost can be tackled comparably.

Now you have 2 methods that, starting from a list of thresholds, tries to determine the cost factor. We can easily refactor this out:

def get_factor(thresholds, value):
 for threshold, factor in sorted(thresholds.items()):
 if value <= threshold:
 return factor

def ground_shipping_cost(weight):
 thresholds = 2: 1.5, 6: 3.0, 10: 4.0, float("inf"): 4.75
 flat_cost = 20
 return flat_cost + get_factor(thresholds, weight)


def drone_shipping_cost(weight):
 thresholds = 2: 4.5, 6: 9.0, 10: 12.0, float("inf"): 14.75
 return weight * get_factor(thresholds, weight)

Cheapest costs

Your cheapest_shipping method calculates the costs for the different shipping methods, finds the cheapest one and formats this into a string. This string formatting is also very repetitive, and should be done somewhere else. cheapest_shipping will be the most clear if it only returned which method is the cheapest, and the corresponding cost. This also allow you to test this method with unit tests further on.

Since methods are real objects in Python, and you can pass references to them on and store these in dicts, calculating the costs for the different methods can be simplified a lot:

def cheapest_shipping(weight):
 methods = 
 "drone": drone_shipping_cost,
 "standard ground": ground_shipping_cost,
 "premium ground": lambda weight: 125,
 
 results = method: calculation(weight) for method, calculation in methods.items()

To look for the cheapest option among those, you can use the built-in min:

cheapest_method = min(results, key=lambda method: results[method])

return cheapest_method, results[cheapest_method]

Note: the lambda weight: 125 is the equivalent of

def premium_shipping(weight):
 return 125

and "premium ground": premium_shipping, in the methods dict

And this can be called and formatted using str.format or f-strings in Python 3.6+

method, cost = cheapest_shipping(4)
f"You should use method shipping as it will only cost cost"

'You should use standard ground shipping as it will only cost 23.0'

Further refactoring

You can even refactor this one step further, and have 1 generalized cost calculation method that take a flat_cost and thresholds as arguments

def get_shipping_cost(weight, thresholds=None, flat_cost=0):
 if thresholds is None:
 return flat_cost
 return flat_cost + weight * get_factor(thresholds, weight)

then you can define the different shipping methods like this:

shipping_methods = 
 "drone": "thresholds": 2: 4.5, 6: 9.0, 10: 12.0, float("inf"): 14.75,
 "standard ground": 
 "flat_cost": 20,
 "thresholds": 2: 1.5, 6: 3.0, 10: 4.0, float("inf"): 4.75,
 ,
 "premium ground": "flat_cost": 125,


def cheapest_shipping2(methods, weight):
 results = 
 method: get_shipping_cost(weight, **parameters)
 for method, parameters in methods.items()
 
 cheapest_method = min(results, key=lambda method: results[method])
 return cheapest_method, results[cheapest_method]

method, cost = cheapest_shipping2(shipping_methods, 4)

different min

Instead of using min with the cost as key, you can reverse the results dictionary:

def cheapest_shipping2(methods, weight):
 results = 
 get_shipping_cost(weight, **parameters): method
 for method, parameters in methods.items()
 

 cost, method = min(results.items())
 return method, cost

In case of an ex aequo, this can have different results than the other method

even further refactoring.

Now all shipping methods are calculated with get_shipping_cost. If you want to use different functions for the different shipping methods, you can do something like this:

def cheapest_shipping3(methods, weight, default_cost_method=get_shipping_cost):
 results = 
 parameters.pop("method", default_cost_method)(
 weight, **parameters
 ): method
 for method, parameters in methods.items()
 
 cost, method = min(results.items())
 return method, cost

shipping_methods2 = 
 "drone": "thresholds": 2: 4.5, 6: 9.0, 10: 12.0, float("inf"): 14.75,
 "standard ground": 
 "flat_cost": 20,
 "thresholds": 2: 1.5, 6: 3.0, 10: 4.0, float("inf"): 4.75,
 ,
 "premium ground": "method": lambda weight: 125,


method, cost = cheapest_shipping3(shipping_methods2, 4)

please note that the parameters.pop mutates the original shipping_methods2 after the execution:

 "drone": "thresholds": 2: 4.5, 6: 9.0, 10: 12.0, inf: 14.75,
 "standard ground": 
 "flat_cost": 20,
 "thresholds": 2: 1.5, 6: 3.0, 10: 4.0, inf: 4.75,
 ,
 "premium ground": ,

To prevent this, we need to make a copy of the methods:

def cheapest_shipping3(methods, weight, default_cost_method=get_shipping_cost):
 methods_copy = 
 method: parameters.copy()
 for method, parameters
 in methods.items()
 
 results = 
 parameters.pop("method", default_cost_method)(
 weight, **parameters
 ): method
 for method, parameters in methods_copy.items()
 
 cost, method = min(results.items())
 return method, cost