4
\$\begingroup\$

I wrote this code to solve a problem from a John Vuttag book:

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

Can my code be optimized or made more concise? Any tips or errors found?

{
    a = int (raw_input("enter num: "))
    b = int (raw_input("enter num: "))
    c = int (raw_input("enter num: "))
    d = int (raw_input("enter num: "))
    e = int (raw_input("enter num: "))
    f = int (raw_input("enter num: "))
    g = int (raw_input("enter num: "))
    h = int (raw_input("enter num: "))
    i = int (raw_input("enter num: "))
    j = int (raw_input("enter num: "))


    num_List = {a,b,c,d,e,f,g,h,i,j }
    mylist=[]

    ## Use for loop to search for odd numbers

    for i in num_List:

                 if i&1 :
                     mylist.insert(0,i)
                     pass


                 elif i&1 is not True: 
                     continue
    if not mylist:
        print 'no odd integers were entered'
    else:
        print max (mylist)

}
\$\endgroup\$
  • \$\begingroup\$ Here's my response to a suspiciously similar question: codereview.stackexchange.com/questions/26187/… \$\endgroup\$ – Johntron Jun 4 '13 at 19:04
  • \$\begingroup\$ this question is a "finger exercise" in John Vuttag book, introduction to programming. Sorry if it was posted before \$\endgroup\$ – user25830 Jun 4 '13 at 19:12
  • \$\begingroup\$ Oh no problem, I was just trying to help :) \$\endgroup\$ – Johntron Jun 4 '13 at 20:09
4
\$\begingroup\$

Your main loop can just be:

for i in num_list:
    if i & 1:
        mylist.append(i)

There is no need for the else at all since you don't do anything if i is not odd.

Also, there is no need at all for two lists. Just one is enough:

NUM_ENTRIES = 10

for dummy in range(NUM_ENTRIES):
    i = int(raw_input("enter num: "))
    if i & 1:
        mylist.append(i)

Then the rest of the program is as you wrote it.

\$\endgroup\$
  • \$\begingroup\$ Thanks! it looks cleaner. I am working on using one raw_input instead of 10 \$\endgroup\$ – user25830 Jun 4 '13 at 18:56
  • \$\begingroup\$ No problem! I am no Python specialist though \$\endgroup\$ – fge Jun 4 '13 at 19:04
  • \$\begingroup\$ @fge one small thing, wouldn't a for j in range(10) be better than incrementing in a while loop? \$\endgroup\$ – tijko Jun 4 '13 at 20:03
  • \$\begingroup\$ tijko: yup... I didn't know of range :p \$\endgroup\$ – fge Jun 4 '13 at 21:08
5
\$\begingroup\$

This is a solution that doesn't use lists, and only goes through the input once.

maxOdd = None

for _ in range(10):
    num = int (raw_input("enter num: "))
    if num & 1:
        if maxOdd is None or maxOdd < num:
            maxOdd = num

if maxOdd:
    print "The max odd number is", maxOdd        
else:
    print "There were no odd numbers entered"
\$\endgroup\$
4
\$\begingroup\$

One other minor change you could make, would be to just loop over a range how many times you want to prompt the user for data:

mylist = list()
for _ in range(10):
    while True:
        try:
            i = int(raw_input("enter num: "))
            if i & 1:
                mylist.append(i)
            break
        except ValueError:
            print "Enter only numbers!"

No need to create an extra variable and increment it here.

\$\endgroup\$
  • 1
    \$\begingroup\$ Since you don't care about the value of j, it's customary to write for _ in range(10) instead. \$\endgroup\$ – 200_success Jan 18 '14 at 0:56
3
\$\begingroup\$

My 2c: though I don't know Python syntax that well, here's an optimization idea that would be more important given a (much) bigger dataset.

You don't need any lists at all. On the outside of the loop, declare a "maximum odd" variable, initially equal to -1. On the inside of the loop, whenever a number is input, if it's odd and greater than maximumOdd, then set maximumOdd equal to that number. This requires nearly no memory, whereas building up a list and then operating on it scales linearly in memory (not good).

\$\endgroup\$
  • \$\begingroup\$ I was trying to get the loop concept down, this is taking it to the next level, Thanks. Can you please tell me why you set the variable to -1 \$\endgroup\$ – user25830 Jun 6 '13 at 13:45
  • \$\begingroup\$ The variable is set to -1, or some other value making it obvious that it's "unset". That way, if the loop runs through without finding any odd numbers, you can tell afterwards. \$\endgroup\$ – Reinderien Jun 6 '13 at 18:58
1
\$\begingroup\$

I'd write:

numbers = [int(raw_input("enter num: ")) for _ in range(10)]
odd_numbers = [n for n in numbers if n % 2 == 1]
message = (str(max(odd_numbers)) if odd_numbers else "no odd integers were entered")
print(message)
\$\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.