1
\$\begingroup\$

Ask user to input 10 integers and then print the largest odd number that was entered. If no odd number was entered, print a message to that effect.

This is my follow-up code on feedback from Find the largest odd number (follow-up code)

This time I'm trying to achieve the same result with nested functions. I need your comments please. Also, please note that I'm working through a book and it's meant to be used with Python 2.x

def find_largest_odd():
    numbers_entered = []

    def get_ten_ints():
        while len(numbers_entered) < 10:
            try:
                number = int(raw_input('Enter an integer: '))
                numbers_entered.append(number)
            except ValueError:
                print 'That was not an integer!'

    get_ten_ints()

    try:
        largest_odd_number = max(item for item in numbers_entered if item % 2)
        return 'The largest odd number entered was {}.'.format(largest_odd_number)
    except ValueError:
        return 'No odd number was entered.'
\$\endgroup\$
5
\$\begingroup\$

Since your first attempt, you are indeed showing progress, and it's kind of fun to watch. When entering into the realm of using functions the ball game changes a little, and you need to start thinking of segregation of duties and possible whether you can reuse one or more of the functions.

Some more or less general cases, addressed toward your code:

  • Make functions general – Allow for the input function to enter an arbitrary number of integers, i.e. rename/repurpose the get_ten_ints() function
  • Be wary of global variables – Do not use global variables if they can be avoided. This also applies to using functions and variables defined within the function (i.e. the numbered_entries variable)
  • Try avoiding magic numbers – In your code the 10 stands out as kind of a magic number, which do affect the running of your code, but where does it come from? If you want to have such numbers, they are better declared as constants in top of the file (or function)
  • Don't use nested functions, unless there is a real good reason – To me, I don't see why the get_ten_ints() is a nested function. I would propagate it to the top level of your code
  • Start documenting your code – A common practice is to use docstrings to document your function, so that it is clearer when you return to your code what it was supposed to do
  • Separate presentation from business logic – Decide if a function is supposed to present stuff or return stuff. Your original code had find_largest_odd() both find this number, and present it. In such a small functions that could be OK, but it's better to make it a clean function returning the number, and then letting the caller decide what it wants to do with the number, i.e. print it or use it for other calculations
  • Introducing main() function – The larger the code base the more interesting is it to introduce a pattern which reveals a common entry point to your code. The __name__ == '__main__' test ensures that if you run python on your file you get the execution defined, but still allowing for the file to be imported as a module giving access to your functions
  • Normalize your print statements – I'm not sure according to which standard, but it's quite normal (both in python 2.x and 3.y) to use constructs like print("my text: {}".format(some_variable))

Applying all of these principles you could end up with something like:

def get_ints(number_of_ints):
    """Read from input exactly number_of_ints numbers, and verify that they
       actually are ints. Return the list of these numbers"""
    numbers_entered = []
    while len(numbers_entered) < number_of_ints:
        try:
            number = int(raw_input("Enter an integer: "))
            numbers_entered.append(number)
        except ValueError:
            print("That was not an integer!")

    return numbers_entered

def find_largest_odd(numbers):
    """From a given list, find the largest odd number. Return None if no odd
       number is found."""
    try:
        largest_odd_number = max(item for item in numbers if item % 2)
        return largest_odd_number
    except ValueError:
        return None

def main():
    """Read 10 integers into a list, and find the largest odd number"""
    my_numbers = get_ints(10)
    the_number = find_largest_odd(my_numbers)
    if the_number != None:
        print("Largest odd number in list: {}".format(the_number))
    else: 
        print("No odd number in list")

if __name__ == '__main__':
    main()        
\$\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.