Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have written a piece of python code in response to the following question and I need to know if it can be "tidied up" in any way. I am a beginner programmer and am starting a bachelor of computer science.

Question: Write a piece of code 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.

My solution:

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

list1 = [a, b, c, d, e, f, g, h, i, j]
list2 = [] # used to sort the ODD values into 
list3 = (a+b+c+d+e+f+g+h+i+j) # used this bc all 10 values could have used     value'3'
                          # and had the total value become an EVEN value

 if (list3 % 2 == 0): # does list 3 mod 2 have no remainder
    if (a % 2 == 0): # and if so then by checking if 'a' has an EVEN value     it rules out
                 # the possibility of all values having an ODD value entered
        print('All declared variables have even values')
    else:
        for odd in list1: # my FOR loop to loop through and pick out the     ODD values
            if (odd % 2 == 1):# if each value tested has a  remainder of one to mod 2
                list2.append(odd) # then append that value into list 2
        odd = str(max(list2)) # created the variable 'odd' for the highest ODD value   from list 2 so i can concatenate it with a string.
        print ('The largest ODD value is ' + odd) 
share|improve this question
    
Thanks everyone for your input, i really appreciate that ! i will take a look at the extra code and try to understand how, where and why it fits in. ! –  Ryan Schreiber Feb 16 '13 at 21:02
13  
If you have 10 lines that are pretty much the same chances are good that you are doing it wrong and want a loop instead. –  ThiefMaster Feb 16 '13 at 21:09
    
great site great people ! :) whoever cant become a skilled programmer these days with all this help deserves to sit and wonder why they not moving forward.. thank you ! –  Ryan Schreiber Feb 16 '13 at 21:28
4  
You seem to have a logic problem is your solution. Entering 2,1,1,2,2,2,2,2,2,2 returns All declared variables have even values. –  Caramdir Feb 17 '13 at 2:00
1  
@palacsint at least we can up vote the comments 0:) –  Albert Renshaw Feb 19 '13 at 4:16

9 Answers 9

Here are a couple of places you might make your code more concise:

First, lines 2-11 take a lot of space, and you repeat these values again below when you assign list1. You might instead consider trying to combine these lines into one step. A list comprehension might allow you to perform these two actions in one step:

>>> def my_solution():
    numbers = [input('Enter a value: ') for i in range(10)]

A second list comprehension might further narrow down your results by removing even values:

    odds = [y for y in numbers if y % 2 != 0]

You could then see if your list contains any values. If it does not, no odd values were in your list. Otherwise, you could find the max of the values that remain in your list, which should all be odd:

    if odds:
        return max(odds)
    else:
        return 'All declared variables have even values.'

In total, then, you might use the following as a starting point for refining your code:

>>> def my_solution():
        numbers = [input('Enter a value: ') for i in range(10)]
        odds = [y for y in numbers if y % 2 != 0]
        if odds:
            return max(odds)
        else:
            return 'All declared variables have even values.'


>>> my_solution()
Enter a value: 10
Enter a value: 101
Enter a value: 48
Enter a value: 589
Enter a value: 96
Enter a value: 74
Enter a value: 945
Enter a value: 6
Enter a value: 3
Enter a value: 96
945
share|improve this answer
2  
Good answer, but if len(list2) != []: is not really pythonic. You could simply do: if len(list2): –  Zenon Feb 16 '13 at 21:35
1  
Welcome to Code Review and thanks for the excellent answer. –  codesparkle Feb 16 '13 at 22:18
    
Thanks to you both for sharing your knowledge and improvements. I updated the answer. –  Justin Barber Feb 16 '13 at 22:34
7  
@Zenon couldn't you just do if list2:? –  Snakes and Coffee Feb 16 '13 at 23:37
1  
@SnakesandCoffee yes you could and shouldd, because it's much better. –  nightcracker Feb 17 '13 at 8:08

A tip: check immediately after asking each single question. No list to store values is needed, because you can forget each even number immediately, and follow the biggest odd number only.

And: there is listobject.append() BTW, see http://docs.python.org/3/library/stdtypes.html#sequence-types-list-tuple-range

share|improve this answer
1  
A wonderful tip: forget what you don't need to know for the problem at hand. Just keep the largest odd so far or an indicator that you don't have one yet (like start the largest at zero and test for zero at the end). –  Ross Millikan Feb 20 '13 at 5:31

No-one has given the code I would write, which doesn't create a list but instead tracks the answer in a loop. The logic is a little more complex, but it avoids storing values (here it doesn't really matter, since it's only 10 values, but a similar problem with input from another program, might have more data than fits in memory).

maxOdd = None
for _ in range(10):
    value = int(input('Enter a value: '))
    if (value % 2 and (maxOdd is None or value > maxOdd)):
        maxOdd = value
if maxOdd:
    print('The largest odd value entered was', maxOdd)
else:
    print('No odd values were entered.')

Note that I am treating non-zero values as True, so value % 2 is a test for oddness (because it gives a non-zero remainder, and Python treats non-zero values as true). Similarly, if maxOdd tests that maxOdd is not None (nor zero, but it cannot be zero, because that is even).

Another way, closer to the other answers, in that it treats the numbers as a sequence, but still avoiding storing everything in memory, is to use a generator:

from itertools import islice

def numbers():
    while True:
        yield input('Enter a value: ')

def odd(n):
    return n % 2

try:
    print('The largest odd value entered was',
          max(filter(odd, map(int, islice(numbers(), 10)))))
except ValueError:
    print('No odd values were entered.')

This is more "advanced", but numbers() creates a generator, and the chain of functions max, filter, map, islice effectively call that for each value (simplifying slightly). So the end result is a kind of pipeline that, again, avoids keeping everything in memory.

(The ValueError happens when max doesn't receive any values.)

Another way of writing that would be (note the lack of square brackets):

try:
    print('The largest odd value entered was',
          max(filter(odd,
                     map(int, (input('Enter a value: ')
                               for _ in range(10))))))
except ValueError:
    print('No odd values were entered.')

where (input ...) is a generator expression.

share|improve this answer
    
thank you, ur right, it is a little advanced for me right now, although i like the idea of not storing the values. any suggestions on a good reading material related to computer logic? –  Ryan Schreiber Feb 17 '13 at 6:50

some advice:

  • you don't need 10 named variables for the input, use an array instead, and a loop to do it
  • use list comprehension to filter out all odd numbers
  • take a look at the string formatting docs of python
share|improve this answer
    
thanks Fivesheep –  Ryan Schreiber Feb 16 '13 at 19:48
2  
Fivesheep, thanks for answering questions on Code Review. I hope you enjoy it. :) Your answer is already very useful as Ryan noted it, but I think it could use a bit more details: eg. you what string formatting functions do you recommend? Is it possible to show how the list comprehension would look like? –  Quentin Pradet Feb 16 '13 at 20:12
1  
Can you explain to me how to define the variables without having to type them all, is it just a=0, b=0 etc. do i use a FOR loop, to loop through the questions, Ive tried using the variables as ive mentioned and used a FOR loop, and it looped through the questions, but my output was totally wrong :8 –  Ryan Schreiber Feb 16 '13 at 20:19
    
@Ryan You do not need to define the variables per se. You merely need to create a list with 10 inputs. In other words, you would want something like this: [input('Enter a value: ') for i in range(0,10)]. This list comprehension would be converted to a list of 10 values without having to define 10 separate variables. –  Justin Barber Feb 16 '13 at 21:02
    
@user1775603, yes, thanks, i am starting to become aware of that now. it is WAAYY easier to do that. its very cool. –  Ryan Schreiber Feb 17 '13 at 11:12

I would do it something like this:

def is_odd(x):
    return x & 1 and True or False


olist = []

for i in xrange(10):
    while True:
        try:
            n = int(raw_input('Enter a value: '))
            break
        except ValueError:
            print('It must be an integer.')
            continue

    if is_odd(n):
        olist.append(n)

try:
    print 'The largest ODD value is %d' % max(olist)
except ValueError:
    print 'No ODD values were entered'

The is_odd function is checking the last bit of the integer to see if it is set (1) or not (0). If it's set then it must be odd. Eg: 010 (2 is not odd) 011 (3 is odd).

Also, I would filter odds on the fly so I don't need to parse the list multiple times and I would use xrange (a generator) to keep memory "safe".

For this case those "optimizations" wouldn't be necessary as the list is only size 10 and there isn't much computation but if you had to take a lot of integers you would need to consider it.

share|improve this answer
    
With python3, I prefer range since it is optimized to do lazy evaluation. xrange.. doesn't feel good while reading. :P –  Mahesh Feb 17 '13 at 9:11

Since you didn't specify whether you're using Python 2 or Python 3, I'm not sure if the below answer is relevant. In Python 2, input is risky. In Python 3, input is fine.

Although some answers have replaced input(...) by int(raw_input(...)), I don't think anybody has explained yet why this is recommended.

input considered harmful in Python 2.x

In Python 2.x, input(x) is equivalent to eval(raw_input(x)). See, for example, the documentation for input in Python 2.7, and the documentation for raw_input. This means that a user can execute an arbitrary Python expression, such as:

>>> input("Number? ")
Number? os.unlink("/path/to/file") 

Therefore, in Python 2, always use raw_input rather than input.

Some explanation: raw_input returns a string, for example, '10'. This string you need to convert to a number. The function int can do this for you (see documentation in Python 2.7). So wherever you use input to get a whole number, you would rather want int(raw_input(...)). This will take the input as a string, and immediately convert it to an integer. This will raise an exception if the input cannot be converted to an integer:

>>> int(raw_input("Number? "))
Number? os.unlink("/path/to/file")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: invalid literal for int() with base 10: 'os.unlink("/path/to/file")'

In Python 3, input is equivalent to Python 2 raw_input, see for example, the documentation for input in Python 3.3.

share|improve this answer
    
I see. i was trying to create this program and it wouldnt run when i used raw_input, im using python2.7, but it took input and worked. why would that be? and thanks for the insight –  Ryan Schreiber Feb 17 '13 at 10:56
    
@RyanSchreiber I added some explanation –  gerrit Feb 17 '13 at 11:00
    
fantastic, okay, i understand now why it never worked! another note on that, why would i as a programmer, want to have the text inputted as a string and converted to an integer ? is this a secure way of programming and if so what is the reason behind that? and btw, yes, i will read the documentation, would just like another outside view on it :) –  Ryan Schreiber Feb 17 '13 at 11:07
    
surely you mean raw_input rather than input –  Janus Troelsen Feb 17 '13 at 13:55
    
@Ysangkok Oops, yes I do of course, now fixed. –  gerrit Feb 17 '13 at 14:39

This is the finger exercise from Ch.2 of Introduction to Computation and Programming Using Python by John V. Guttag. It's the recommended text for the MITx: 6.00x Introduction to Computer Science and Programming. The book is written to be used with Python 2.7.

At this point the book has only introduced variable assignment, conditional branching programs and while loops as well as the print function. I know as I'm working through it. To that end this is the code I wrote:

counter = 10
while counter > 0:
    x = int(raw_input('Please type in integer: '))
    if counter == 10 or (x%2 and x > ans):
        ans = x
    if ans%2 == 0: # Even Number Safeguard
        ans = x 
    counter = counter -1
if ans%2 != 0:
    print 'The largest odd number entered was', str(ans)
else:
    print 'No odd number was entered'`

Seems to work. It can probably be shortened but the idea was to use only the limited concepts that have been covered in the book.

EDITED to include commments to shorten code. Still meets the criteria of using the limited concepts introduced in the book.

share|improve this answer
    
I was referring to the exact question from the text. I've change it now. –  archery1234 Nov 9 '13 at 2:46
    
Oh, okay. It seemed like a new question at first. –  Jamal Nov 9 '13 at 2:48
1  
ans = ans does nothing, and you could simplify all the if...else...elif conditions to a single one: if counter == 10 or (x % 2 and x > ans): ans = x –  Stuart Nov 10 '13 at 14:04
    
Yep, it can definitely be shortened. I'll get my head round the computational way of thinking, eventually. –  archery1234 Nov 13 '13 at 4:29
    
I suggest: ans = None at the top, if x % 2 and (ans is None or x > ans) inside the loop, and if ans is not None after the loop. Not repeating 10 and % 2 makes the code more maintainable. (That ends up being @andrewcooke's answer, except without using range().) –  200_success Nov 14 '13 at 18:26

I think in this case, performance is of no concern and so the priority is making workings of the program as immediately obvious as possible.

user_input = [input('Enter a value: ') for x in range(10)]
numbers = map(int, user_input)
odd_numbers = [n for n in numbers if n % 2 != 0]
if odd_numbers:
    print(max(odd_numbers))
else:
    print("All the numbers are even")

Here each line is a new logical and concise step.

  1. Get numbers from user as strings using input in Python 3 or raw_input in Python 2.
  2. Convert the strings to ints.
  3. Extract all the odd numbers from the list of numbers.
  4. Print the biggest odd number or a message.
share|improve this answer

Why not multiply each value by its least-significant bit (aka that number in modulo 2), so that even numbers are zero and odd numbers stay what they are, then use max() to return the largest number (which has to be odd because all the evens were multiplied by zero)

**Note, assuming no negative-numbers, if there are than just use max but strip all "0" from your array.

share|improve this answer
    
Modulo 2 maps everything to 0 or 1, which is not useful. –  200_success Nov 9 '13 at 5:54
    
@200_success If you read the full answer you would see that I said MULTIPLY the number by itself in modulo 2... which would make all even numbers 0 and all odd numbers would stay what they were (since they were multiplied by 1), THEN use max() and return the largest number (which would, by definition, be the largest ODD number). –  Albert Renshaw Nov 9 '13 at 6:40
    
@200_success If you could undo your down-vote it would be greatly appreciated. –  Albert Renshaw Nov 9 '13 at 6:43
    
I've changed the wording for clarity and reversed my vote. –  200_success Nov 9 '13 at 6:54
    
@200_success Thankyou very much :) It means alot to me haha! –  Albert Renshaw Nov 10 '13 at 5:54

protected by 200_success Nov 9 '13 at 5:55

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.