3
\$\begingroup\$

The code ive written takes the user entered numbers and sorts them into a list and finds the two biggest numbers, im looking to make the code less verbose. any help/advice is greatly appreciated.

def printTwoLargest():
    numList = []
    num = 'holder'
    x=0
    y=0
    while num != '':
        num = input('Please enter a number: ')
        if num != '':
            numList.append(float(Decimal(num)))
        else:
            break
    numList.sort(reverse=True)
    for i in range(len(numList)):
        if numList[i]>x:
            x = numList[i]
        elif numList[i]>y:
            y = numList[i]
        else:
            pass
    if x>0 and y>0:
        print('The largest number is:',x)
        print('The second largest number is:',y)
    elif x>0:
        print('The largest number is:',x)
    else:
        print('No positive numbers were entered.')
\$\endgroup\$
0

4 Answers 4

4
\$\begingroup\$

You should change your function to take a list and return the two largest of it. And so you should move the creation of numList out into it's own function. You should also move the printing out of the function too, and instead return a list of the two largest. This should spilt your code up to achieve:

def getNumbers():
    numList = []
    num = 'holder'
    while num != '':
        num = input('Please enter a number: ')
        if num != '':
            numList.append(float(Decimal(num)))
        else:
            break
    return numList

def twoLargest(numbers):
    x=0
    y=0
    numbers.sort(reverse=True)
    for i in range(len(numbers)):
        if numbers[i] > x:
            x = numbers[i]
        elif numbers[i] > y:
            y = numbers[i]

    if y > 0:
        return x, y
    if x > 0:
        return x,
    return (,)

if __name__ == '__main__':
    numbers = getNumbers()
    largest = twoLargest(numbers)
    print(largest)

However you can improve getNumbers, rather than checking to break from the loop twice, you can instead check once. This removes the need for num = 'holder', and can reduce the amount of lines needed.

def getNumbers():
    numList = []
    while True:
        num = input('Please enter a number: ')
        if num == '':
            break
        numList.append(float(num))
    return numList

However it can further be simplified by making it a generator function. To do this you can just yield the values, rather than numList.append them.

def getNumbers():
    while True:
        num = input('Please enter a number: ')
        if num == '':
            break
        yield float(num)

Moving onto twoLargest, it doesn't work with a generator, as you don't cast numList to a list. You also use sort the list in descending order, so rather than using a loop to find the largest two, you should instead use slicing. This can get you:

def twoLargest(numbers):
    return sorted(numbers, reverse=True)[:2]

Going forward, you can make an 'n' largest function. But rather than using sorted you can use; heapq.nlargest, and max. Making the interface as standard as possible you can get:

from heapq import nlargest

def nLargest(numbers, n=1, key=None, sort=False):
    kwargs = {'key': key}
    if key is None:
        del kwargs['key']
    if n == 1:
        try:
            return [max(numbers, **kwargs)]
        except ValueError: # if passed an empty iterable
            return []
    if sort:
        return sorted(numbers, reverse=True, **kwargs)[:n]
    else:
        return nlargest(n, numbers, **kwargs)

The reason for the three methods is described under nlargest:

The latter two functions perform best for smaller values of n. For larger values, it is more efficient to use the sorted() function. Also, when n==1, it is more efficient to use the built-in min() and max() functions. If repeated usage of these functions is required, consider turning the iterable into an actual heap.

\$\endgroup\$
2
  • \$\begingroup\$ As I said in another comment, in your first implementation if the list is 10, 1, 2, 3, 10, 4, the output will be 10, 10 instead of 10, 4. \$\endgroup\$
    – ChatterOne
    Commented Jan 3, 2017 at 11:01
  • 1
    \$\begingroup\$ @ChatterOne The one where all I done was split up OPs code? Also I disagree that it should return 10, 4, if that is needed use nLargest(set(...)). \$\endgroup\$
    – Peilonrayz
    Commented Jan 3, 2017 at 11:12
3
\$\begingroup\$
  1. Function name: It should be print_two_largest. Check PEP8

  2. Instead of num = 10000 you can use num = float('inf') or in Python 3.5:

    import math
    num = math.inf
    

    Reference

  3. Var names, instead of x or num try with names that means something. Like, min_number, current_number, counter, first_largest, etc.. Just x, a, my_var are not self-explanatory.

  4. I would remove unnecessary comments. What unnecessary comments are? Code tells you how, Comments tell you why

  5. if x > 0 and y > 0: instead of if x>0 and y>0: Check PEP8

  6. Efficiency: What is the complexity of your code? (Big-O notation; You can be able to do this in \$O(n)\$)

  7. What happens if the user inserts zero (only one negative number) or just one positive number? A more elegant solution will use a empty list and handle this case in the code.

\$\endgroup\$
0
\$\begingroup\$

i changed my code and used the pop() function which i liked better.

def printTwoLargest():
numList = [0,0] #place holder # in list 
num = 1000000 #place holder #
x = 0
y = 0
while num > 0:
    num = input('Please enter a number: ')
    num = eval(num)
    numList.append(num)
numList.sort()
x = numList.pop()
y = numList.pop()
if x>0 and y>0:
    print('The largest is',x)
    print('The second largest is',y)
elif x>0:
    print('The largest is',x)
else:
    print('No positive numbers were entered')
\$\endgroup\$
1
  • 1
    \$\begingroup\$ You should fix your indentation. Other than that, there's the problem of what happens if you have an input like 10, 5, 6, 7, 10. You should either account for that or use a set. \$\endgroup\$
    – ChatterOne
    Commented Jan 2, 2017 at 21:48
0
\$\begingroup\$

You should add if __name__ == '__main__': that works as main() function because you created a user define function printTwoLargest(). For float values you should import from decimal import Decimal.Then your code works properly. We may try your code this way.Thank You.

from decimal import Decimal


def printTwoLargest():
    numList = []
    num = 'holder'
    x=0
    y=0
    while num != '':
        num = input('Please enter a number: ')
        if num != '':
            numList.append(float(Decimal(num)))
        else:
            break
    numList.sort(reverse=True)
    for i in range(len(numList)):
        if numList[i]>x:
            x = numList[i]
        elif numList[i]>y:
            y = numList[i]
        else:
            pass
    if x>0 and y>0:
        print('The largest number is:',x)
        print('The second largest number is:',y)
    elif x>0:
        print('The largest number is:',x)
    else:
        print('No positive numbers were entered.')

if __name__ == '__main__':
    printTwoLargest()
\$\endgroup\$
2
  • 1
    \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. \$\endgroup\$
    – Peilonrayz
    Commented Jan 5, 2017 at 9:53
  • \$\begingroup\$ Also, have a look at Loop Like A Native to learn how to loop properly in Python. \$\endgroup\$
    – Graipher
    Commented Jan 5, 2017 at 9:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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