6
\$\begingroup\$

The program here is simply supposed to tell you the shipping charge based off of the weight of the package. I am just wondering if there is any further way to concise it? At the moment, I do not think it can go any further than that. The only place where I'm unsure whether I could improve it, could be Booleans. Possibly use a while statement?

def main():

    weight = eval(input("Please enter the weight of the package: "))

    rate = 0

    if weight <= 2:
        rate += 1.25
    elif weight > 2 and weight <= 5:
        rate += 2.35
    elif weight > 5 and weight <= 10:
        rate += 4.05
    elif weight > 10:
        rate += 5.55

    charge = rate * weight

    print("The shipping charge is: $%s" % charge)

print(main())
\$\endgroup\$
  • 14
    \$\begingroup\$ weight = eval(input("Please enter the weight of the package: ")) it's extremely important to not do this. You do not want to evaluate untrusted code, it creates a massive security hole in your code. \$\endgroup\$ – shuttle87 Oct 29 '15 at 15:31
  • \$\begingroup\$ @shuttle87 This would count as an answer in my opinion, even if small it is still really important and relevant to know! Next time consider making an answer instead of a comment. (relevant discussion meta.codereview.stackexchange.com/questions/2106/…) \$\endgroup\$ – Marc-Andre Oct 30 '15 at 13:25
3
\$\begingroup\$

Here are some more thoughts, although some of it has been mentioned before:

  • Calculating shipping charge sounds like a function – This sounds like something you could do multiple times, and as such should be made into a function
  • Trust in the if / elif ... – It is unneccessary to do both the if weight <= 2 and the elif weight > 2.
  • ... or alternatively use a dictionary/list and a loop – When you build repeating structures as the multiple if & elif you should consider if you can make this into a loop structure, which in general looks nicer than the repeated structure.
  • Seperate business logic from presentation logic – In your case remove the print() arount the calling to main, and simplify main() to handle input/ouput, and let the function do the logic
  • Add the tagging to allow use as a module – If you add the if __name__ == '__main__': block/tagging, you could reuse your module from another place when you want to calculate shipping charges
  • Use int(input(..)) for integer input, float(input(...)) or just input(...) for string – These allows for better handling of your input. Also note that if you are using Python 2, you should use raw_input() instead of input().
  • Change to the newer string.format – In Python 3 it is preferred to stop using the % operator, and rather use the string formatting. I.e. doing stuff like: "The shipping charge is: ${:.2f}".format(charge) which format the shipping charge as a float with 2 decimals.
  • Document your code using docstrings/comments – Always nice to document constants or functions in your code!
  • Include error handling – No point in calculating shipping charges of negative weiths for example. So a little input validation is always good

Code refactoring

# WEIGHT_RATES is a list of tuples = (min_weight, rate),
# As it is reverse sorted, look from start to find the first
# entry where your weight is higher than min_weight, and use
# corresponding rate
WEIGHT_RATES = [
      ( 10, 5.55),
      (  5, 4.05),
      (  2, 2.35),
      (  0, 1.25)
    ]

def shipping_charge(weight):
    """Return shipping charge based upon the weight."""

    if weight < 0:
       raise ValueError("Can't calculate shipping charge of negative weights")

    for min_weight, rate in WEIGHT_RATES:
        if weight > min_weight:
            return weight * rate


def main():
    """Read a single weight, and calculate shipping charge"""

    weight = int(input("Please enter the weight of the package: "))
    print('The shipping charge is: ${:,.2f}'.format(shipping_charge(weight)))


if __name__ == '__main__':
    main()

Added: shipping_charge_no_list

As commented upon, your teacher is not fond of lists, so here is a version of the function without the list. Note that, in my opinion, it is a little better to change the order around, mainly because I'll expect the first case to be most prominent and should break out of the if-block firstly. But that is a matter of taste, and your mileage may vary.

def shipping_charge_no_list(weight):
    """Return shipping charge based upon the weight."""

    if weight < 0:
       raise ValueError("Can't calculate shipping charge of negative weights")

    if weight > 10:
        rate = 5.55
    elif weight > 5:
        rate = 4.05
    elif weight > 2:
        rate = 2.35:
    else
        rate = 1.25

    return weight * rate
\$\endgroup\$
  • \$\begingroup\$ thank you for such exhaustive explanation! It is much appreciated. This is for my Python Lab at school and my professor unfortunately wants things the specific way. I am already aware of loops and lists thanks to Codecademy (which is what I would use in the first place), but I would get marked down for that. \$\endgroup\$ – Paul Oct 29 '15 at 21:54
  • \$\begingroup\$ Also, I am confused about the list. Why are you using parentheses? To me it looks kind of like a dictionary, where there's a key and a value. If I were to do it similar to yours, I would do two lists and zip them together. I didn't know that by using parentheses, you create tuples within a single list. \$\endgroup\$ – Paul Oct 29 '15 at 22:02
  • \$\begingroup\$ @Paul, I use the list to avoid initialising an OrderedDict. An ordinary dict will not maintain the order of the weights. And using zip() on two list, when you can write that the exact same thing using parentheses seems like extraneous work... Don't you agree? \$\endgroup\$ – holroy Oct 29 '15 at 22:11
  • \$\begingroup\$ I agree; I was just very surprised that I've never stumbled upon using dictionaries that way before. This makes the entire script look so much cleaner and easier to understand. \$\endgroup\$ – Paul Oct 29 '15 at 22:15
  • 1
    \$\begingroup\$ @Paul, RATES is not a dictionary, it is a list of tuples. But it is a quite useful construct, and can easily be extend to multiple values of the tuple. It does however have downsides like easily find any given entry... If that is needed, one should seriously consider using an ordered dict. \$\endgroup\$ – holroy Oct 29 '15 at 22:24
11
\$\begingroup\$

You are checking much more than you need to. Consider the first two cases:

if weight <= 2:
    # A
elif weight > 2 and weight <= 5:
    # B

If weight <= 2, we fall into case A. If we're not in case A, only then do we continue onto the next condition checks (because we're using elif and not if). As such, if we're even in the path that checks the condition which leads to B, we already know that weight > 2. Thus, that check is redundant and can be reduced to simply weight <= 5.

The whole body can become:

if weight <= 2:
    rate += 1.25
elif weight <= 5:
    rate += 2.35
elif weight <= 10:
    rate += 4.05
else:
    rate += 5.55

Which isn't just less code, it's easier to reason about since all your bounds are visually in the same column.

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

As others have commented on the logic, I'll comment on the rest :)

weight = eval(input("Please enter the weight of the package: "))

Why have you used eval()? It is used for running raw code from the input which you don't really need.

Instead, do:

int(input("Please enter the weight of the package: "))

Here, you convert the input to an integer, at the same time. If you want to use floats, simply do:

float(input("Please enter the weight of the package: "))

As far as I can see, there's no need to add to the rate because you're only calling the function once. So rate will always be 0 at the beginning.

So instead, remove the rate = 0 and change the rate += 1.25, etc... to rate = 1.25, etc...


There's also no need to print() main() because you have a print statement already in it.

Instead, you could return the string that you are currently printing, or just remove the surrounding print() from print(main())


With those changes in mind:

def main():

    weight = int(input("Please enter the weight of the package: "))

    if weight <= 2:
        rate = 1.25
    elif weight > 2 and weight <= 5:
        rate = 2.35
    elif weight > 5 and weight <= 10:
        rate = 4.05
    elif weight > 10:
        rate = 5.55

    charge = rate * weight

   return "The shipping charge is: $%s" % charge

print(main())
\$\endgroup\$
  • \$\begingroup\$ It's just how I've been taught and have always been confused. I assumed that as long as I'm using integers, that eval( ) was necessary. \$\endgroup\$ – Paul Oct 29 '15 at 15:19
  • 1
    \$\begingroup\$ @Paul no, if you're inputting integers, convert the input() to an int via int() :) And for strings, just do a normal input() :) \$\endgroup\$ – ᔕᖺᘎᕊ Oct 29 '15 at 15:22
  • \$\begingroup\$ Also, I just noticed that if I do use int(), I cannot use a float because it spits out a Value Error. Thank you, though! :) \$\endgroup\$ – Paul Oct 29 '15 at 15:22
  • 3
    \$\begingroup\$ @Paul If you want to use floats and not ingegers, convert to a float instead with float(). \$\endgroup\$ – ᔕᖺᘎᕊ Oct 29 '15 at 15:23
4
\$\begingroup\$

Since you have covered all possible cases, the last

elif weight > 10:
    rate += 5.55

can just be:

else:
    rate += 5.55

Also, in python, you can do:

elif 2 < weight <= 5:

Finally, your main doesn't return anything, so instead of

print(main())

you can just do:

main()
\$\endgroup\$
  • \$\begingroup\$ Thank you! @fredtantini I didn't think it was possible dropping a variable between the two signs. I'm glad you mentioned. \$\endgroup\$ – Paul Oct 29 '15 at 15:15
  • \$\begingroup\$ @Paul This is called interval comparion and is pretty much unique to Python. Most (sane) languages would interpret this as (2 < weight) <= 5, which is nonsensical. \$\endgroup\$ – isanae Oct 30 '15 at 1:10
3
\$\begingroup\$

By extracting the computation of the rate to a different function, you can simplify the code by returning early when you know the rate:

def compute_rate(weight):
    if weight <= 2:
        return 1.25
    if weight <= 5:
        return 2.35
    if weight <= 10:
        return 4.05
    return 5.55

Used like this in main:

charge = compute_rate(weight) * weight
\$\endgroup\$
  • 3
    \$\begingroup\$ The original if .. elif chain short circuits evaluation in the same way. Please focus on the value of extracting the discrete logic to a separate method. \$\endgroup\$ – psaxton Oct 29 '15 at 15:38
  • \$\begingroup\$ I think what @psaxton is saying is he's downvoting you. \$\endgroup\$ – user1717828 Oct 29 '15 at 22:01
  • \$\begingroup\$ @user1717828 he didn't downvote - there are no downvotes on this answer. \$\endgroup\$ – ᔕᖺᘎᕊ Oct 29 '15 at 23:50
2
\$\begingroup\$

I like to separate logic from data, to make both more readable. Here, the key rate change points are in a separate dictionary. We can find the correct rate by checking from the end of the list. Shipping is calculated in the function, or we could return just the rate.

RATES = {
    0: 1.25,
    2: 2.35,
    5: 4.05,
    10: 5.55,
}

def calc_shipping(weight):
    # Make reversed key weight list
    key_weights = sorted(RATES.keys(), reverse=True)

    for more_than in key_weights:
        if weight > more_than:
            return RATES[more_than] * weight
    else:
        return 0.0

To test for the correctness of the solution compared to the original, I used this py.test parameterized function:

@pytest.mark.parametrize("weight, expected_shipping", [
    (0, 0),
    (1.5, 1.875),
    (2, 2.5),
    (2.0001, 4.700235),
    (4, 9.4),
    (5, 11.75),
    (5.0001, 20.250405),
    (8, 32.4),
    (10, 40.5),
    (15, 83.25),
])
def test_calc_shipping(weight, expected_shipping):
    # Check for approximate equality
    assert abs(calc_shipping(weight) - expected_shipping) < 0.0000001
\$\endgroup\$

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.