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.
6 Answers 6
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:
- prompt user for all input numbers
- filter to just the odd numbers
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
-
1\$\begingroup\$ Setting a
maximum
to 0 doesn't make much sense either, does it? \$\endgroup\$2023年05月02日 19:00:55 +00:00Commented 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\$Shreeyash Motiwale– Shreeyash Motiwale2023年05月02日 21:25:30 +00:00Commented 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– Barmar2023年05月03日 14:43:54 +00:00Commented 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– tjalling2023年05月05日 13:12:13 +00:00Commented 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.– SD.2023年05月05日 18:48:54 +00:00Commented May 5, 2023 at 18:48
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))
-
1\$\begingroup\$ I would generally avoid calling a function that prints stuff (
"Enter a number:"
) as an argument ofprint
. It makes the flow a bit weird. \$\endgroup\$JiK– JiK2023年05月03日 13:55:21 +00:00Commented May 3, 2023 at 13:55 -
1\$\begingroup\$ Like that, @Jik? \$\endgroup\$sbottingota– sbottingota2023年05月03日 15:42:44 +00:00Commented 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– Clashsoft2023年05月03日 19:59:18 +00:00Commented 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\$Ξένη Γήινος– Ξένη Γήινος2023年05月04日 07:28:19 +00:00Commented May 4, 2023 at 7:28
-
\$\begingroup\$ @ΞένηΓήινος is this what you mean? \$\endgroup\$sbottingota– sbottingota2023年05月04日 07:53:52 +00:00Commented May 4, 2023 at 7:53
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.
-
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 asdouble
. 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 againstfloat('-inf')
, FTW! \$\endgroup\$J_H– J_H2023年05月03日 02:37:51 +00:00Commented May 3, 2023 at 2:37 -
1\$\begingroup\$ @J_H Personally, I think I’d use the
Optional[int]
. \$\endgroup\$Davislor– Davislor2023年05月03日 03:02:50 +00:00Commented 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\$Darrel Hoffman– Darrel Hoffman2023年05月05日 20:45:13 +00:00Commented 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– Davislor2023年05月06日 03:49:56 +00:00Commented May 6, 2023 at 3:49
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.
-
3\$\begingroup\$ I am surprised no one has pointed it out yet, but
max
has a keyword argumentdefault
, which will be returned if theIterable
is empty, similar tonext
. So it should be better written as:max(odd_values, default='no odd values were inputted')
. \$\endgroup\$Ξένη Γήινος– Ξένη Γήινος2023年05月05日 03:01:23 +00:00Commented May 5, 2023 at 3:01
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')
)
-
\$\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'))
throwsOverflowError: 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\$Peter Cordes– Peter Cordes2023年05月05日 07:47:28 +00:00Commented May 5, 2023 at 7:47
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
-
\$\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 aprint
within a loop. \$\endgroup\$2023年05月05日 16:08:12 +00:00Commented May 5, 2023 at 16:08 -
\$\begingroup\$ @SᴀᴍOnᴇᴌᴀ I forgot to add the break statement. I've added it now. \$\endgroup\$The Empty String Photographer– The Empty String Photographer2023年05月05日 17:43:19 +00:00Commented May 5, 2023 at 17:43
-
\$\begingroup\$ Thank you so much for your advice . \$\endgroup\$Shreeyash Motiwale– Shreeyash Motiwale2023年05月07日 22:42:29 +00:00Commented May 7, 2023 at 22:42
x
instead of a number? \$\endgroup\$int
function silently truncates the fractional part. So, if the user inputs several odd numbers lower than e.g. 11 and then inputs at last11.23
, the return value would be 11, which would be wrong. \$\endgroup\$