1
\$\begingroup\$

This is my attempt on the Python 3 Sal's Shipping challenge on Codecademy (Challenge Link (instructions in the link)).

I'd really appreciate anyone to take the time to review my code. I'm a very new to programming and I really wanna improve my coding, so all feedback is apreciated.

def groundCost(weight):
  if weight > 10:
    cost = weight * 4.75
  elif weight > 6:
    cost = weight * 4
  elif weight > 2:
    cost = weight * 3
  else:
    cost = weight * 1.5
  cost += 20
  return cost

def droneCost(weight):
  if weight > 10:
    cost = weight * 14.25
  elif weight > 6:
    cost = weight * 12
  elif weight > 2:
    cost = weight * 9
  else:
    cost = weight * 4.5
  return cost

def costCalc(weight):
  g = groundCost(weight)
  p = 125
  d = droneCost(weight)
  gS = "Ground Shipping"
  pS = "Premium Ground Shipping"
  dS = "Drone Shipping"
  if d == g or d == p or g == p:
    if d == g and d == p:
      print("All shipping methods cost $125 according to your items weight")
      return 
    if d == g:
      same1 = dS
      same2 = gS
      cost = d
    if d == p:
      same1 = dS
      same2 = pS
      cost = d
    if g == p:
      same1 = gS
      same2 = pS
      cost = g
    print("Youre cheapest shipping method is "+same1+" and "+same2+" as they both cost $"+str(cost))
    return
  elif (g < d) and g < p:
    cheapMeth = gS
    cost = g
  elif (p < d) and p < g:
    cheapMeth = pS
    cost = p
  elif (d < g) and d < p:
    cheapMeth = dS
    cost = d
  print("Youre cheapest shipping method is "+cheapMeth+" costing $"+str(cost))
  return
weight = int(input())
costCalc(weight)
\$\endgroup\$
  • 1
    \$\begingroup\$ The challenge you link to is in their Pro program, so not everyone can see it. This and the fact that you should always provide at least the essential parts of the challenge description means that you have work to do. \$\endgroup\$ – AlexV Jul 3 at 21:00
  • 1
    \$\begingroup\$ Please can you include the core of the challenge description. I'm a bit confused that you output "All shipping methods cost $125 according to your items weight" but I can't find any value where this can be true. \$\endgroup\$ – Peilonrayz Jul 4 at 2:05
  • \$\begingroup\$ I thought that there was a possibility of that being true so i just added that part just in case, but turns out that it wasn't possible according to you hehe. \$\endgroup\$ – Ahmad Izzudin Alifyandra Jul 4 at 6:55
2
\$\begingroup\$

Idiomatic changes & small changes:

  1. Indent with 4 spaces, not 2.
  2. Variable and function names should be in lower_snake_case.
  3. Unneeded parentheses is unidiomatic.
  4. Use spaces around concatenation, so "+abc becomes " + abc.
  5. Use a if __name__ == '__main__': guard to prevent your code from running when it’s not the initialised file.
  6. Have two spaces around top level function definitions.
  7. “You’re” has an apostrophe in it, but you should be using “your” instead. Because you aren’t a cheapest shipping method.

Further changes:

  1. Use if, elif and else rather than just if.
  2. You can make your code dry by merging ground_cost and drone_cost together.

  3. You can simplify your code by using sorted.

    You sort g, d and p. From this you can then iterate through the list and see how many are the same.

    This makes cost_calc contain only three simple if statements.

    You would have to add something like a enum.Enum to know what type each value is, when sorting.

import enum


def size_cost(value, sizes):
    for size, cost in sizes[:-1]:
        if value > size:
            return cost
    return sizes[-1][1]


ground_costs = [
    (10, 4.75),
    (6, 4),
    (2, 3),
    (None, 1.5),
]
drone_cost = [
    (10, 14.25),
    (6, 12),
    (2, 9),
    (None, 4.5),
]


class Shipping(enum.Enum):
    GROUND = 'Ground'
    PREMIUM = 'Premium Ground'
    DRONE = 'Drone'


def cost_calc(weight):
    costs = [
        (Shipping.GROUND, size_cost(weight, ground_costs) * weight + 20),
        (Shipping.PREMIUM, 125),
        (Shipping.DRONE, size_cost(weight, drone_cost) * weight)
    ]
    costs.sort(key=lambda i: i[1])
    price = costs[0][1]
    costs = [s for s, p in costs if p == price]
    if len(costs) == 3:
        print("All shipping methods cost $125 according to your items weight")
    elif len(costs) == 2:
        print(
            "Your cheapest shipping method is "
            + costs[0].value
            + " shipping and "
            + costs[1].value
            + " shipping as they both cost $"
            + str(price)
        )
    else:
        print(
            "Your cheapest shipping method is "
            + costs[0].value
            + " shipping costing $"
            + str(price)
        )


if __name__ == '__main__':
    cost_calc(int(input()))

Output is also what you’d expect:

>>> cost_calc(1)
Your cheapest shipping method is Drone shipping costing $4.5
>>> cost_calc(5)
Your cheapest shipping method is Ground shipping costing $35
>>> cost_calc(1000)
Your cheapest shipping method is Premium Ground shipping costing $125
>>> cost_calc(10 / 3)
Your cheapest shipping method is Ground shipping and Drone shipping as they both cost $30.0
>>> cost_calc(105 / 4.75)
Your cheapest shipping method is Ground shipping and Premium Ground shipping as they both cost $125.0

NOTE: Complete changes.

\$\endgroup\$
  • \$\begingroup\$ thanksss a bunch, lots of new things im learning from this post. \$\endgroup\$ – Ahmad Izzudin Alifyandra Jul 4 at 6:50
2
\$\begingroup\$

Ok, first of all you should've take care of variable names. Naming them like p, g, e, etc is really a bad habbit.

Once you change them, it's much easier to read.

def cost_calc(weight):
  ground_cost = calculate_ground_cost(weight)
  drone_cost = calculate_drone_cost(weight)
  premium_cost = 125
  ground_shipping_name = "Ground Shipping"
  premium_shipping_name = "Premium Ground Shipping"
  drone_shipping_name = "Drone Shipping"

Second python naming convention tells you to use underscores while naming variables or functions.

Third, your functionc that calculates costs can be minimized in many ways, when one of them is:

import bisect 

def calculate_ground_cost(weight):
  limits = [2, 6, 10]
  multipliers = [1.5, 3, 4, 4.75]
  idx = bisect.bisect_left(limits, weight)
  return (weight * multipliers[idx]) + 20

def calculate_drone_cost(weight):
  limits = [2, 6, 10]
  multipliers = [4.5, 9, 12, 14.25]
  idx = bisect.bisect_left(limits, weight)
  return (weight * multipliers[idx])

Then you should take care of IFs statements that are really messy.

if drone_cost == ground_cost or drone_cost == premium_cost or ground_cost == premium_cost:

Is equvalent to

if drone_cost == ground_cost == premium_cost:

And the next rule good to follow is always return ASAP.

if drone_cost == ground_cost == premium_cost:
    return "All shipping method cost %s according to your items weight" % premium_cost

Then all of this mess:

if  d == g and d == p:
      print("All shipping methods cost $125 according to your items weight")
      return 
    if d == g:
      same1 = dS
      same2 = gS
      cost = d
    if d == p:
      same1 = dS
      same2 = pS
      cost = d
    if g == p:
      same1 = gS
      same2 = pS
      cost = g
    print("Youre cheapest shipping method is "+same1+" and "+same2+" as they both cost $"+str(cost))
    return

Can be changed to

if ground_cost == drone_cost:
    return "Youre cheapest shipping method is %s and %s as they both cost$%s" %(ground_shipping_name, premium_shipping_name, ground_cost)

Because this is the last "IF" that is matching the statement.

And at the end, where u try to determine what is the lowest cost:

  elif (g < d) and g < p:
    cheapMeth = gS
    cost = g
  elif (p < d) and p < g:
    cheapMeth = pS
    cost = p
  elif (d < g) and d < p:
    cheapMeth = dS
    cost = d
  print("Youre cheapest shipping method is "+cheapMeth+" costing $"+str(cost))
  return

You can change it to

costs = sorted([
   (ground_cost, ground_shipping_name), 
   (drone_cost, drone_shipping_name), 
   (premium_cost, premium_shipping_name)])

min_cost, method = costs[0]
return "Your cheapest shipping method is %s costing $%s" % (method, min_cost)

So at the end, your code look like:

import bisect 

def calculate_ground_cost(weight):
  limits = [2, 6, 10]
  multipliers = [1.5, 3, 4, 4.75]
  idx = bisect.bisect_left(limits, weight)
  return (weight * multipliers[idx]) + 20

def calculate_drone_cost(weight):
  limits = [2, 6, 10]
  multipliers = [4.5, 9, 12, 14.25]
  idx = bisect.bisect_left(limits, weight)
  return (weight * multipliers[idx])

def cost_calc(weight):
  ground_cost = calculate_ground_cost(weight)
  drone_cost = calculate_drone_cost(weight)
  premium_cost = 125
  ground_shipping_name = "Ground Shipping"
  premium_shipping_name = "Premium Ground Shipping"
  drone_shipping_name = "Drone Shipping"
  if drone_cost == ground_cost == premium_cost:
    return "All shipping methodrone_shipping_name cost $125 according to your items weight"

  if ground_cost == drone_cost:
    return "Youre cheapest shipping method is %s and %s as they both cost$%s" %(ground_shipping_name, premium_shipping_name, ground_cost)

  costs = sorted([
    (ground_cost, ground_shipping_name), 
    (drone_cost, drone_shipping_name), 
    (premium_cost, premium_shipping_name)])

  min_cost, method = costs[0]

  return "Your cheapest shipping method is %s costing $%s" % (method, min_cost)


weight = int(input("Enter weight: "))
print(cost_calc(weight))
\$\endgroup\$
  • \$\begingroup\$ ooh thanks, i really need to study this bisect class \$\endgroup\$ – Ahmad Izzudin Alifyandra Jul 4 at 6:52

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.