12
\$\begingroup\$

I implemented the finger exercise from chapter 2.4 of the book called Intro to Computation Programming using Python by John Guttag.

The task is:

Write a program that asks the user to input 10 integers, and then prints the largest odd number that was entered. If no odd number was entered, it should print a message to that effect.

Is the code without any holes? How can I avoid repeating the same line 10 times?

a = int(input('Enter the number: '))
b = int(input('Enter the number: '))
c = int(input('Enter the number: '))
d = int(input('Enter the number: '))
e = int(input('Enter the number: '))
f = int(input('Enter the number: '))
g = int(input('Enter the number: '))
h = int(input('Enter the number: '))
i = int(input('Enter the number: '))
j = int(input('Enter the number: '))

A = [a,b,c,d,e,f,g,h,i,j]
A.sort()

while True:
    if A[len(A)-2]<A[len(A)-1]:
        if A[len(A)-1]%2==1:
            break
        A[len(A)-2]=A[(len(A)-2)-1]
        A[len(A)-1]=A[(len(A)-1)-1]
        
print(A[len(A)-1],'is the largest odd')
\$\endgroup\$
3
  • 2
    \$\begingroup\$ Hi I have edited your post to reflect the rules of this site. I have put the goal of your code to the title of the question and I moved the reference to the source book into the body of your question. Please check it and if I did some mistake or misinterpretation please correct that. \$\endgroup\$
    – slepic
    Nov 18, 2022 at 6:27
  • 2
    \$\begingroup\$ Hi, there is no misinterpretation. Thank you for editing it out. \$\endgroup\$
    – G.Yasuo
    Nov 18, 2022 at 6:29
  • 1
    \$\begingroup\$ Check this out: codereview.stackexchange.com/a/22793/98809 List comprehensions and filters solving the same problem. \$\endgroup\$
    – minseong
    Nov 18, 2022 at 15:19

6 Answers 6

19
\$\begingroup\$

If you find yourself typing the same line over and over, that's a good indicator that a loop might be a good idea.

A = []
while len(A) < 10:
    A.append(int(input("Enter a number: ")))

I'm going to stop here to ask an important question: why are we storing all of the entered numbers? The problem asks only for the largest odd number, so we can ignore all other numbers. Storing every number that was entered only complicates the code.

biggest_odd = None  # The value None represents no odd number has been entered yet.
for _ in range(10):  # The name _ indicates we are not going to use this variable.
    n = int(input("Enter a number: "))
    if n % 2 == 1 and (biggest_odd is None or n > biggest_odd):
        biggest_odd = n
print(biggest_odd, "is the biggest odd.")

One final comment on your original code. I don't really understand what your while True: loop is doing, but if the last number entered is even then it loops forever. It only looks at or modifies the last three numbers entered.

\$\endgroup\$
5
  • \$\begingroup\$ Thank you for your feedback, appreciate it very much. Regarding the question: I am not sure what you mean by storing, so pardon me if I answer your question wrongly. If you were asking why I made a list of all the numbers after having the user input the numbers individually. I didn't know how to do otherwise, so was stuck with that. Regarding the while loop, As while True makes the loop go until the condition is met, I wanted to make sure that the loop runs until the condition is met. \$\endgroup\$
    – G.Yasuo
    Nov 18, 2022 at 14:05
  • 5
    \$\begingroup\$ I don't agree with "storing every number entered only complicates the code". I think having one loop to input all numbers, and a second step to get the answer (e.g. max([x for x in numbers if x % 2])) is a totally fine solution, and is conceptually clearer than keeping track of largest odd number so far at every iteration. \$\endgroup\$
    – minseong
    Nov 18, 2022 at 15:08
  • 2
    \$\begingroup\$ You can get the best of both worlds by using generators: numbers = (int(input("Enter a number: ")) for _ in range(10)); return max(x for x in numbers if x % 2)) \$\endgroup\$ Nov 18, 2022 at 15:20
  • \$\begingroup\$ @G.Yasuo I asked about storing the numbers to get you to think of other ways of handling incoming data. Sometimes, input data is irrelevant to solving a problem, like even numbers for this problem. As for the while True loop, look again at the code you wrote and ask what happens if the last number the user enters is even. Go through the loop line by line. Can the break statement ever be reached in this case? \$\endgroup\$
    – Mark H
    Nov 19, 2022 at 3:23
  • \$\begingroup\$ @theonlygusti I agree with you, but I wanted to focus on using loops to replace repeated lines and not introduce too many new concepts. \$\endgroup\$
    – Mark H
    Nov 19, 2022 at 3:27
18
\$\begingroup\$

Re-read the problem statement - you missed this bit:

If no odd number was entered, it should print a message to that effect.


I'm not sure of your knowledge level, but I'll introduce the idea of a filter, which is the easiest way to select just the odd numbers from the array. Let's write a named function to detect odd numbers:

def is_odd(n):
    return n % 2 == 1

Then we can filter our numbers (let's give the list a more meaningful name than A, and use lower-case as that's Python standard):

    odd_numbers = filter(is_odd, numbers)

Once we have the odd numbers, we can use the built-in max function, though we need to provide a default value for when no numbers are odd:

    return max(odd_numbers, default=None)

We can put these together in a function:

def largest_odd(numbers):
    '''
    Return the largest odd number in the array,
    or None if there are no odd numbers

    Examples:
    >>> largest_odd([])
    
    >>> largest_odd([0,2])
    
    >>> largest_odd([0,2,1])
    1
    >>> largest_odd([0,-1,-5])
    -1
    >>> largest_odd([-5,-1,0])
    -1
    '''
    odd_numbers = filter(is_odd, numbers)
    return max(odd_numbers, default=None)

Notice that I've provided a description of what the function does, including examples. The description is called a docstring, and will become more important when you write more complex code. The examples are written in a particular form so they can be automatically tested using this bit of magic (don't worry too much about how it works just now):

if __name__ == '__main__':
    import doctest
    exit(doctest.testmod()[0] > 0)

To use largest_odd in a program, I suggest this:

    numbers = [int(input('Enter the number: ')) for _ in range(10)]
    n = largest_odd(numbers)
    if n is not None:
        print(n)
    else:
        print("No odd numbers present")

That first line here is a list comprehension that calls input 10 times and creates an array.

Instead of print(n), we could produce more informative output. We'll use a Python-3 f-string for this:

        print(f'The largest odd number was {n}')

The f-string is a bit of magic that evaluates and replaces everything enclosed in braces, and it's very handing for formatting user output like this.

We can make it more robust by creating a function that repeatedly asks for input until an integer is provided:

def input_integer(prompt):
    while (True):
        try:
            return int(input(prompt))
        except ValueError:
            print('An integer value is required')

If we put it all together, the code looks like this:

#!/usr/bin/python3

def is_odd(n):
    return n % 2 == 1

def largest_odd(numbers):
    '''
    Return the largest odd number in the array,
    or None if there are no odd numbers

    Examples:
    >>> largest_odd([])
    
    >>> largest_odd([0,2])
    
    >>> largest_odd([0,2,1])
    1
    >>> largest_odd([0,-1,-5])
    -1
    >>> largest_odd([-5,-1,0])
    -1
    '''
    odd_numbers = filter(is_odd, numbers)
    return max(odd_numbers, default=None)

def input_integer(prompt):
    while (True):
        try:
            return int(input(prompt))
        except ValueError:
            print('An integer value is required')

if __name__ == '__main__':
    numbers = [input_integer('Enter the number: ') for _ in range(10)]
    n = largest_odd(numbers)
    if n is not None:
        print(f'The largest odd number was {n}')
    else:
        print('No odd numbers present')

if __name__ == '__main__':
    import doctest
    exit(doctest.testmod()[0] > 0)

Important note

Be sure you're using Python 3, not Python 2:

  • Python 2 is in maintenance-only state now, and it's better to be learning the current language.
  • In Python 2, input() is dangerous because it interprets what is typed as a Python expression. If you cannot use Python 3, you should use raw_input() instead.
  • In Python 2, the default argument to max() is a positional argument, so you'd write max(odd_numbers, None) instead.
\$\endgroup\$
2
  • 3
    \$\begingroup\$ When reading your solution, the if n: test jumped out at me. It took me a while to convince myself it was correct: if n is not None then it's necessarily odd, and therefore it can't be zero so it's truthy. If that test had been written if n is not None: instead, that would probably have reflected the intent more closely. \$\endgroup\$ Nov 18, 2022 at 20:46
  • \$\begingroup\$ Agreed @Daniel, and edited. Thank you! \$\endgroup\$ Nov 19, 2022 at 8:53
5
\$\begingroup\$

You have not implemented the last part of the specification.

If no odd number was entered, it should print a message to that effect.

\$\endgroup\$
2
  • \$\begingroup\$ I was halfway through writing my answer, beginning with this observation, when you wrote this. I feel I'm in good company! \$\endgroup\$ Nov 18, 2022 at 13:45
  • \$\begingroup\$ Thank you. I mistakenly missed that. \$\endgroup\$
    – G.Yasuo
    Nov 18, 2022 at 14:06
5
\$\begingroup\$

code duplication

Your first ten lines exhibit code duplication, which is undesirable and a good clue that you can rewrite it with a loop instead.

You immediately turn all inputs into a list right afterwards: [a, b, c, ...], so why not just build the list directly in the first place? This also means you don't have to store so many variables.

A = []
for _ in range(10):
    A.append(int(input('Enter the number: ')))
A.sort()

input validation

You can also code some form of input validation to make sure your program won't crash if the user enters something that int(...) can't convert. A loop like this is often a common pattern for this purpose:

A = []
for _ in range(10):
    entry = input('Enter the number: ')
    while True:
        try:
            A.append(int(entry))
            break
        except ValueError:
            print(f"Please only enter an integer, like '114' or '7' but not '{entry}'.")
            entry = input('Try again: ')
A.sort()

I used an f-string to show the user the invalid input they entered that caused a problem.

I've used a try: ... except: ... block instead of if entry.isnumeric(), because it is generally considered better practice in Python to "Ask forgiveness, not permission". However, I think the ask-permission approach actually produces cleaner code in this case:

A = []
for _ in range(10):
    entry = input('Enter the number: ')
    while not entry.isnumeric():
        print(f"Please only enter an integer, like '114' or '7' but not '{entry}'.")
        entry = input('Try again: ')
    A.append(int(entry))
A.sort()

negative indices

After sorting your list, you loop through it backwards to get the first odd number you encounter. Good idea.

Did you know indexing like A[len(A)-2]<A[len(A)-1] is exactly equivalent to A[-2]<A[-1] ? Python programmers usually use the second form because it has less noise, and everyone can easily understand the second form (perhaps even more easily than the first, because it is more common).

solution design

Just a note, that if you wanted to you could have sorted from largest-to-smallest in the first place:

A.sort(reverse=True)

then the contents of your loop would have been easier to understand, without all the counting-backwards A[len(a)-x] indexing.

I can see what you are trying to do with the smallest-to-largest sorted list A:

  1. check if the last item in the list is greater than the second-last item in the list
  2. if it is, check if it is odd, and if it is odd, output it
  3. always replace the last item with the second-last item
  4. and replace the second-last item with the third-last item

This won't work for lists that have more than 3 items, because the third-last item never changes.

It would work if the method continued:

  1. replace the third-last item with the fourth-last item
  2. replace the fourth-last item with the fifth-last item
    ...
    1. replace the ninth-last item with the tenth-last item

    Which could be implemented with a loop, but a cleaner version altogether may be simply to remove the last item each iteration:

    while True:
        if A[-2] < A[-1]:
            if A[-1] % 2 == 1:
                break
            A.pop()
    

    Python called this method pop because a common use case for it is to use lists like stacks. A.pop() actually also returns the last item (the item it removes) too, so you can write:

    while True:
        last_item = A.pop()
        if A[-1] < last_item:
            if last_item % 2 == 1:
                break
    
    print(last_item, 'is the largest odd')
    

    because A.pop() removes the item, the second-last item ends up at A[-1] instead of A[-2] now. However, because the list is sorted, A[-1] will always be <= last_item (which is surely what you meant in the first place instead of <), so you can get rid of that if-statement.

    while True:
        last_item = A.pop()
        if last_item % 2 == 1:
            break
    

    This will crash when the list loses all its items, because you can't pop from an empty list. Instead of looping forever, loop until the list is empty:

    while A:
        last_item = A.pop()
        if last_item % 2 == 1:
            break
    

    This is great idiomatic code.

    what if there are no odd numbers?

    Your task says

    If no odd number was entered, it should print a message to that effect.

    which means that if the user enters ten even numbers, you should display a message like "There are no odd numbers."

    Currently your code will print one of the even numbers (actually the smallest one) and falsely call it an odd number, because you haven't attempted to solve this part of the problem. Give it a go!

    variable names

    Variable names should always be meaningful, so A could be renamed numbers or something else you think explains what it stores.

    spacing around operators

    You'll notice in the code I've been writing, I've fixed your spacing for you. Operators should have space around them, so A[len(A)-1]%2==1 would be better written A[len(A) - 1] % 2 == 1 for example.

    \$\endgroup\$
    1
    • \$\begingroup\$ Thank you for your feedback. This is very clear to follow and very insightful especially with comments regarding what is a generally accepted way of writing codes. I will keep these in mind for the next set of problems I will try. \$\endgroup\$
      – G.Yasuo
      Nov 18, 2022 at 15:24
    1
    \$\begingroup\$

    The main thing I'd do: Separate the I/O from the algorithmic part.

    Write (1) an input function which deals with all the typical pesky problems of guiding, vetting and controlling user input; and (2) another function which gives you the maximum of a sequence of numbers. Whether your input routine returns only the odd numbers or whether you filter them out afterwards depends on whether you need the even part for something else later.

    This approach improves a number of things:

    • Re-usability: The max function you wrote is not limited any longer to handling manual input or even input at all but instead any sequence from anywhere; and since max is such a great thing to have, somebody has followed my advice already ;-). Which reveals that re-usability through modularization is a two-way street: Write re-usable code, and re-use code. The latter is important because old code is typically efficient and correct, while new code is typically neither.

    • Readability: Because the main control flow now only consists of function calls it is really easy to understand what the code should do. In pseudo code:

        odd_arr = input_odds()
        max_elem = max(odd_arr)
      

      There are two downsides of this. You have to look elsewhere in order to see what's really done; and you cannot implement the most efficient "one-pass" solution suggested by Marc H. But still, the benefits of modularization often outweigh that harm. The most efficient solution, after all, would be in assembler, at the cost of readability.

    • Maintainability: If something is wrong with the input routine you can fix it and be reasonably sure that the algorithm is not affected because it is separate.

    • Last not least: Testability. This becomes more important in large projects and in safety critical software with extensive test requirements. In these environments you really want small, independently testable code units. Separating the algorithm from the "data acquisition", to use a fancy word (which could be a hardware port, a database or, like here, manual input) makes the "test bed" for the algorithm trivial: We don't need a mock database connection, hardware emulation or emulated user interaction to test max().

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

    Mark H's code can be simplified by initializing biggest_odd to negative infinity rather than None, coercing parity to boolean, and using max rather than testing whether the newest is larger. Also, it might be good to have some error handling for if the user enters something that can't be turned into an integer. In such a case, we can just set n to zero, since all even numbers are treated the same. Or you could print an error message.

    biggest_odd = float('-inf')
    for _ in range(10): 
        try:
            n = int(input("Enter a number: "))
        except ValueError:
            n = 0
        if n % 2:
            biggest_odd = max(biggest_odd, n)
    if biggest_odd > float('-inf'):
        print(biggest_odd)
    else:
        print('No odd numbers were entered.')
    

    If you want to get fancy:

    class odds_first:
        def __init__(self, value):
            try:
                self.value = int(value)
            except ValueError:
                self.value = 0
        def __gt__(self, other):
            if self.value%2 and not other.value%2:
                return True
            if other.value%2 and not self.value%2:
                return False
            return (self.value>other.value)
        def __repr__(self):
            if self.value%2:
               return str(self.value)
            return 'No odd values entered.'
       
    print(max(odds_first(input("Enter a number: ")) for _ in range(10))
    

    This creates a class in which all odd numbers are considered "larger" than even numbers. Also, when members of this class are printed, they are printed as 'No odd values entered.' if they are even. So all you have to do is take the max of all the inputs and print that out.

    \$\endgroup\$

    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.