16
\$\begingroup\$

I am mechanical engineer and have started learning Python from a book called 'Introduction to Computation and Programming Using Python' by John V. Guttag. This is one of the examples. I know that there are several similar questions posted here, but I thought my program is too simplistic. Here is the code:

#Program to enter 10 numbers and finding the greatest odd number 

x = 0
counter = 1
while counter < 11:
    num = int(input('enter number'))
    if num%2 != 0 and num > x:
        x = num
    counter = counter + 1    
print(x)

It produces the desired output. However, as I am a complete novice at this, I don't know if this will give rise to any further problems. Please share your feedback.

\$\endgroup\$
5
  • \$\begingroup\$ Welcome to Code review! Is this code straight from the book or have you modified it? \$\endgroup\$ May 2, 2023 at 19:57
  • \$\begingroup\$ Actually ,it is a question given as ' Finger exercise ' and doesn't have any answer .The above was written by me . \$\endgroup\$ May 2, 2023 at 21:06
  • 2
    \$\begingroup\$ Which number should be output if the user does not enter an odd number at all? 0? What, if the user enters negative odd numbers? Should the answer still be 0? Please give some additional info, your question is not completely defined. \$\endgroup\$ May 3, 2023 at 7:40
  • 2
    \$\begingroup\$ I know you are just starting out (congrats by the way!) and don't want to throw too many new topics at you - but it may be worth exploring error handling, for example what happens if the user enters x instead of a number? \$\endgroup\$
    – Luke
    May 3, 2023 at 11:03
  • \$\begingroup\$ I just experimented a little bit and it seems that float inputs also make problems as the int function silently truncates the fractional part. So, if the user inputs several odd numbers lower than e.g. 11 and then inputs at last 11.23, the return value would be 11, which would be wrong. \$\endgroup\$ May 3, 2023 at 15:25

6 Answers 6

20
\$\begingroup\$

Well, like you said, it works. We could still pick at some nits, though.

x = 0

This is OK, but it perhaps isn't the best identifier choice. Consider renaming to maximum or max_val. Also, see below, as negative numbers are valid inputs.

while counter < 11:

This is correct. But maybe not transparently obvious to someone reading this for the first time, given the starting value of 1, which in CS circles is a bit less common than a starting value of 0.

Consider re-phrasing this as for counter in range(10), which a human reader will immediately recognize as "oh, we're gonna do this TEN times" (not eleven). If the value of counter was of interest within the loop, then range(1, 11) would be equivalent to what the OP does.

    if num%2 != 0 and num > x:

Utilities like black can make your code slightly more legible.

    counter = counter + 1    

Usually we prefer a more concise notation when incrementing something:

    counter += 1    

It's a DRY thing. For short identifiers it hardly matters. For multi-word identifiers, being able to mention it just once is actually helpful. So the += operator has the effect of encouraging the use of longer more descriptive identifiers than would otherwise be convenient.

print(x)

This is correct, but less helpful than it could be. Feel free to label the output so anyone can correctly interpret it, without needing to read the source code: print("The maximum odd value entered was:", x)


The comment gives a Specification for this use case: "enter 10 numbers ...".

One could quibble about "numbers", as entering 3.14 is going to blow up with a ValueError. We will go with "integers", fine.

But then it turns out that the user should only offer non-negative integers. Consider finding the maximum of -3 and -9. If we initialize the "biggest number seen so far" to zero, then how will -3 ever be reported as the winner?

Either initialize to "negative infinity" (which is kind of hard with python since integers can have waaaay more than 64 bits so we would use a float('-inf') FP comparison), or constrain the range of valid inputs, or initialize to a sentinel value like None. First time through the loop, notice the sentinel and copy the user's input (like -9). Rest of the loop works the same as the OP described it.


Python has some more advanced concepts that you will want to explore.

functions

Wrapping this code within a def would be a logical next step.

generators

The yield keyword is a bit advanced for where you are at the moment. But a natural way to code up this problem would be to def odd_number_filter(numbers): which discards all the even inputs.

lists

We don't need to allocate storage for every input number, since we can operate on them one-at-a-time just fine. But it might be convenient to break this task into three sequential steps:

  1. prompt user for all input numbers
  2. filter to just the odd numbers
  3. return max(odd_numbers)

That last step would take advantage of one of python's builtin functions: max. For example:

>>> max([5, 27, 11])
27
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Setting a maximum to 0 doesn't make much sense either, does it? \$\endgroup\$
    – Mast
    May 2, 2023 at 19:00
  • \$\begingroup\$ Thank you so much for your feedback .I will definitely try incorporate your suggestions in my future codes . \$\endgroup\$ May 2, 2023 at 21:25
  • 3
    \$\begingroup\$ Instead of a sentinel that you check for every time through the loop, I like to recommend asking for the first number before the loop. Although this is a little less DRY since you have to duplicate the prompt. \$\endgroup\$
    – Barmar
    May 3, 2023 at 14:43
  • \$\begingroup\$ I'm rusty on python naming conventions, but I think function names should be verbs? odd_number_filter reads like a noun to me. \$\endgroup\$
    – tjalling
    May 5, 2023 at 13:12
  • \$\begingroup\$ What is scary is asking the same question to ChatGPT gave 5/6 of your suggestions + 2 additional ones on error handling. \$\endgroup\$
    – SD.
    May 5, 2023 at 18:48
6
\$\begingroup\$

Your code is good, but could use a few improvements:


The while loop

You could just do this with a for loop:

for i in range(10):
    ...

Your way of finding the largest (odd) number

This is impractical, and it wouldn't work for negative numbers (if all numbers are negative, then it would just output 0).
I would instead do:

nums = []
...
nums = [x for x in nums if x % 2 == 1]
max_num = max(nums)

print("Highest odd number:", str(max_num))

And now, you could even use list comprehension to find the ten numbers:

nums = [int(input("Enter a number: ")) for i in range(10)]

though this might hinder your readability.


Functions

If you plan on running this multiple times, you might want to put your code inside a function:

def greatest_odd_num():
    ...

Full Code:

def greatest_odd_num():
    nums = []

    for i in range(10):
        nums.append(int(input("Enter a number: ")))

    nums = [x for x in nums if x % 2 == 1]
    return max(nums)

greatest_num = greatest_odd_number()
print("Greatest Odd Number:", str(greatest_num))
\$\endgroup\$
8
  • 1
    \$\begingroup\$ I would generally avoid calling a function that prints stuff ("Enter a number:") as an argument of print. It makes the flow a bit weird. \$\endgroup\$
    – JiK
    May 3, 2023 at 13:55
  • 1
    \$\begingroup\$ Like that, @Jik? \$\endgroup\$ May 3, 2023 at 15:42
  • \$\begingroup\$ I would also split the logic from the input, so the input nums is generated outside the function and passed as a parameter. \$\endgroup\$
    – Clashsoft
    May 3, 2023 at 19:59
  • \$\begingroup\$ List comprehension shouldn't be used in this scenario, as in this case it waits for user input which is inherently slow and will block the execution, list comprehension is used to speed up for loops that run without human input, in this case there won't be any performance improvement due to the human factor. \$\endgroup\$ May 4, 2023 at 7:28
  • \$\begingroup\$ @ΞένηΓήινος is this what you mean? \$\endgroup\$ May 4, 2023 at 7:53
5
\$\begingroup\$

What About Negative Numbers?

If the user enters ten negative numbers (which are valid integers), none of them will be greater than 0, so the program will not accept an odd negative number as a valid input. Thus, giving the values -10 to -1 will not return -1 as it should.

In Python 3, there is no maximum or minimum size for int, and therefore no initial int value guaranteed to be less than any valid input. You can use an Optional[int] initialized to None, compare a float to an initial value of float('-inf'), or use another variable to remember whether you’ve seen any odd integers yet.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ I agree. This seems like a simple exercise. But it taught me something. (Ok, re-taught me, I had noticed this long ago.) In most languages a comparison of my_int > neg_inf_float will do type conversion so both numbers are of the same type, such as double. But through the magic of python, it turns out that a negative integer of super-large magnitude will always do the right thing when we compare against float('-inf'), FTW! \$\endgroup\$
    – J_H
    May 3, 2023 at 2:37
  • 1
    \$\begingroup\$ @J_H Personally, I think I’d use the Optional[int]. \$\endgroup\$
    – Davislor
    May 3, 2023 at 3:02
  • \$\begingroup\$ Another edge-case: What if the user enters only even numbers? Then the program will spit out 0, which isn't a valid answer. (I think the only valid answer in this case is to give an error message that no odd numbers were supplied.) \$\endgroup\$ May 5, 2023 at 20:45
  • \$\begingroup\$ @DarrelHoffman Also true! In whatever representation, the solution is the same: check whether the final result is the default empty case. \$\endgroup\$
    – Davislor
    May 6, 2023 at 3:49
5
\$\begingroup\$

As with most things, there is more than one way to do it and what way you choose depends mostly on non-functional requirements - if I had to write a script that did the task in question only once and knew that only valid integers would be entered, then it wouldn't be worth changing anything in your code.

Much as @J_H suggested, I would break the process down into the parts of reading data, filtering the data and calculating the result.

I would use generators to do this, as they allow the use of library functions and comprehensions. I appreciate you probably haven't come across these yet and are still at the writing mostly procedural scripts with loops in, but a lot of Python's success is that it has libraries such as pandas and numpy which allow operations on large amounts of data without explicitly looping over each datum.

So, you want a function which prompts the user to enter an integer, and handles errors; @Ξένη Γήινος already has the bulk of that in the body of his loop:

def read_integer():
    while True:
        try:
            return int(input('enter an integer: '))
        except ValueError:
            print('please input an integer consisting only of decimal digits')

As it's a function we don't have to deal with nested flow control (having a break which breaks out the inner while but not the for) but can just return as soon as we have a valid value. Creating functions generally is to be preferred over nested loops.

Try to keep your functions at about five lines of code. There's a good chance that if it is longer than that any you're not doing something like solving Navier-Stokes then your function is doing more than one thing and could be more than one function. This helps a lot to break things into small parts that can be tested - you will be spending more time trying to work out why your program is not working than you will writing it, so being able to narrow the error down as must as practical is a win.

The most consise way of getting more than one call of a function is a comprehension. Python has both list and generator comprehensions, the difference being that the elements in a list are evaluated when the list is encountered, and the elements in a generator are evaluated when the generator is used. They look much the same on the inside, but generator comprehensions have round brackets, lists have square.

As there are only ten numbers, it doesn't really matter whether we allocate memory for it or not, but generally I prefer to use generators until the last moment as in real applications you don't have such small limits.

values = (read_integer() for _ in range(10))

(It's common to use underscore for variables whose value you don't care about but are syntactically required)

values now holds a generator which, when iterated over, will call read_integer() ten times. When Python executes this line, it creates the generator but does not yet evaluate its members, so the amount of memory used is not dependent on the number of elements.

The next two steps are very small, a single line each. You could also use a generator for the odd values, then calculate max:

odd_values = (value for value in values if value % 2 == 1)
print(max(odd_values))

However, this would mean you get an exception if there are no odd values. You could use try/except to catch that, but for only ten values it is simpler to use a list comprehension and test the 'truthyness' of the list. An empty list is treated as false, so using the list comprehension instead for the final step allows handling that event:

odd_values = [value for value in values if value % 2 == 1]
print(max(odd_values) if odd_values else 'no odd values were input')

I wouldn't usually create a function to filter the odd values, as that is a very simple arithmetic test, but I would if it were a more complicated test.

\$\endgroup\$
2
  • \$\begingroup\$ +1 for handling the situation where no odd number was inputted. \$\endgroup\$ May 4, 2023 at 21:50
  • 3
    \$\begingroup\$ I am surprised no one has pointed it out yet, but max has a keyword argument default, which will be returned if the Iterable is empty, similar to next. So it should be better written as: max(odd_values, default='no odd values were inputted'). \$\endgroup\$ May 5, 2023 at 3:01
4
\$\begingroup\$

Your code works, but as others pointed out, it needs many improvements.

First the user may input a negative number, and your code will return a zero when all user inputs are negative. It definitely needs to be fixed.

Now I am against using max in this case, sure it works, but it will loop through the inputted values a second time, to find the largest value. Sure it takes a tiny bit of time to run, but it will do unnecessary computations when you can do it in one go.

The easiest fix is to set your initial value so low it is guaranteed any number inputted will be greater than that.

And there is such a number smaller than any other number: negative infinity, and because Python uses Double-Precision Floating Point, the smallest float number is -1.7976931348623157E+308. Any number smaller than that will overflow to negative infinity. So -1e309 evaluates to -inf. Sure float('-inf') works, but it is less concise. And -int('9'*309) > -1e309 is True.

Using list comprehension is counterproductive here, it hinders readability and makes no significant improvement here. Here you have blocking calls that asks for human input, and input will be the biggest performance blocker here no matter how performant what your other code might be. And it can never get considerably faster.

You are using a while loop here, don't, while loops repeat the code inside the loop continually until the condition is not met. Here you have a set number of iterations, initializing a counter of iterations, manually increment the counter and checking if the number of iterations has reached a limit is very inefficient and unPythonic. Here you can just use a simple for loop.

Now you are using int(input('enter number')), don't do it. Assume the user will input any character combinations, and only a small fraction of the combinations can be successfully be converted to int. What happens when the user inputs invalid strings?

In [19]: int('Hello')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
Cell In[19], line 1
----> 1 int('Hello')

ValueError: invalid literal for int() with base 10: 'Hello'

It will raise a ValueError exception and stop the execution. (But actually you can convert 'Hello' to int: int('Hello', 36) == 29234652).

So it is better to ask user input continually until the user inputs valid input.

Suggested code:

largest = -1e309
for _ in range(10):
    while True:
        try:
            num = int(input('enter number: '))
            break
        except ValueError:
            print('inputted string was not a number, please input a number')
    if num % 2 and num > largest:
        largest = num
print('the largest odd number inputted is: ', largest)
In [24]: largest = -1e309
    ...: for _ in range(10):
    ...:     while True:
    ...:         try:
    ...:             num = int(input('enter number: '))
    ...:             break
    ...:         except ValueError:
    ...:             print('inputted string was not a number, please input a number')
    ...:     if num % 2 and num > largest:
    ...:         largest = num
    ...: print('the largest odd number inputted is: ', largest)
enter number: fail
inputted string was not a number, please input a number
enter number: -999
enter number: 666
enter number: 242516
enter number: 333
enter number: 4294967295
enter number: 365
enter number: 42
enter number: 1618
enter number: 299792458
enter number: 0
the largest odd number inputted is: 4294967295

Now this is a more concise version, it isn't more performant because you have a input function. And much less readable, but other answers are making the code more concise, so I would follow suit.

def read_integer():
    while True:
        try:
            return int(input('enter an integer: '))
        except ValueError:
            print('please input an integer consisting only of decimal digits')

print(max((i 
    for _ in range(10) 
    if ( i:= read_integer()) % 2),  
    default='no odd values were inputted')
)
\$\endgroup\$
1
  • \$\begingroup\$ -1.7976931348623157e308 is -DBL_MAX, not -INFINITY. Infinity compares as less-than any finite number, and it can't convert to an integer value. int(float('-inf')) throws OverflowError: cannot convert float infinity to integer. (FP literal expressions like 1e309 round up, overflowing to Infinity, so your code suggestion works, but the explanation starts with a mis-statement about infinity having a numeric value. The number you're talking about is the larger finite double.) \$\endgroup\$ May 5, 2023 at 7:47
0
\$\begingroup\$

You can simplify this code quite a bit using lists. First, you need to collect the 10 inputs into a list:

list = [] # Initializes a list
for x in range(10): # Repeats 10 times
 list.append(int(input("enter number: "))) # Appends the integer version of the input number to the list

Then, you can use the max() and pop() functions in Python to do thing to the list:

for y in range(10): # Repeats 10 times
 x = list.pop(list.index(max(list))) # Sets x equal to the biggest number in the list, which also gets removed
 if x % 2 == 1: # Checks if x is odd
  print(x) # Prints x if x is odd
  break # Stops the loop

Here is the full code:

list = []
for x in range(10):
 list.append(int(input("enter number: ")))
for y in range(10):
 x = list.pop(list.index(max(list)))
 if x % 2 == 1:
  print(x)
  break
\$\endgroup\$
3
  • \$\begingroup\$ The code output is not the same as the original code. The OPs code only has a single print while the full code has a print within a loop. \$\endgroup\$ May 5, 2023 at 16:08
  • \$\begingroup\$ @SᴀᴍOnᴇᴌᴀ I forgot to add the break statement. I've added it now. \$\endgroup\$ May 5, 2023 at 17:43
  • \$\begingroup\$ Thank you so much for your advice . \$\endgroup\$ May 7, 2023 at 22:42

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.