3
\$\begingroup\$

The function of this program is repeatedly prompting a user for integer numbers until the user enters 'done'. Once 'done' is entered, print out the largest and smallest of the numbers. Suppose all input is proper, so we do not need to check whether the input value is illegal or not.

largest = None
smallest = None

while True :
    num = raw_input("Enter a number: ")
    if num == "done" :
        break
    try :
        num = int(num)
    except :
        print "Invalid input"
        continue
    if largest is None :
        largest = num
    elif largest < num :
        largest = num
    if smallest is None :
        smallest = num
    elif num < smallest :
        smallest = num

print "Maximum is", largest
print "Minimum is", smallest
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review. There were a couple of issues with your original question. First, the code has to be working correctly to be reviewed. (If you believe that your code is correct and that the Coursera autograder is wrong, then include justification, such as the tests that you tried.) \$\endgroup\$ Commented Oct 7, 2016 at 12:46
  • 1
    \$\begingroup\$ Second, there should be one question per question. The title should reflect the task being performed. I've addressed both issues by removing the second program. You can ask that as a separate question. \$\endgroup\$ Commented Oct 7, 2016 at 12:50

4 Answers 4

4
\$\begingroup\$

It's a shame python doesn't have a do..while loop, which would have been a better fit for the occasion.

You do plenty of branching. For every valid input, you check if the extreme values so far are still None. If you think about it, this will only be the case the first time a valid value is entered. It's not very plausible to keep on performing that check and it needlessly bloats the code.

To find maximum and minimum efficiently, use max() and min() and initialise your variables with appropriate values, that make the initial case blend in with the rest of the operation. +/-Infinity are good candidates for such values. Everything is bigger than -infinity, which makes it ideal as a starting value for the largest number. Here's what your code would look like with those changes applied:

largest = -float('inf')
smallest = float('inf')

while True :
    num = raw_input("Enter a number: ")
    if num == "done" :
        break
    try :
        num = int(num)
    except :
        print "Invalid input"
        continue

    largest = max(num, largest)
    smallest = min(num, smallest)

print "Maximum is", largest
print "Minimum is", smallest
\$\endgroup\$
2
\$\begingroup\$

Good work on not just storing all the entered numbers in a list, and calling max() and min() on the final product. That would run into performance problems (and maybe run out of memory) if somebody entered a lot of numbers; your code doesn’t suffer from this bug.

Now, a few small suggestions:

  • You could combine the code for setting smallest and largest into single conditions:

    if (largest is None) or (num > largest):
        largest = num
    if (smallest is None) or (num < smallest):
        smallest = num
    
  • In general, you should always catch as specific an exception as possible – only the errors you expect to see. Don’t write a bare except:. In this case, you’re catching an exception when the user enters a non-integer, so catch ValueError.

  • Read PEP 8; no spaces before semicolons.

  • I would be more tolerant of malformed user input – for example, if I enter done , or Done, or Done, I think the intention is pretty clear. So I’d be checking:

    if num.strip().lower() == 'done':
    
\$\endgroup\$
1
\$\begingroup\$

You could call max on to variables directly and put the if statements in line and avoid the try catch by checking if the user input is digits.

from string import digits


def main():
    user_input = ""
    smallest = largest = None
    while user_input != "done":
        user_input = input("Enter a number -> ")
        if all(l in digits for l in user_input):
            user_input = int(user_input)
            smallest = min(smallest, user_input) if smallest else user_input
            largest = max(largest, user_input) if largest else user_input
    print("Largest: {}, Smallest: {}".format(largest, smallest))


if __name__ == '__main__':
    main()

My personal belief is that if you don't need the try catch, don't use it. However the above code is analogues to yours.

\$\endgroup\$
1
\$\begingroup\$

With the utmost respect to @alexwlchan i disagree: considerations as to implementations must consider the purpose of the program and how you expect to use it: if you are only entering 10 numbers in, then it may well be easier to stuff all the numbers in an array and call max/min on the array. Memory is not an issue. Fundamentally there is a great danger in over designing and in such cases, quick and dirty works best. personally i hate using while loops - please note that i've written it in ruby, but you should be able to quite easily understand what is going on.

# prompts user to enter numbers until 'done' is typed, max and min numbers are printed
numbers = []

while 
    puts "Enter a number (or type 'done' to finish)"
    input = gets.chomp.to_f

# adds input to numbers array (or exits from loop if required)

if input == 'done'
    break
else
    numbers << input
end

end

puts "Max number is: #{numbers.max }"
puts "Min number is: #{numbers.min}"
\$\endgroup\$

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