17
votes

This is the piece of code I have:

choice = ""

while choice != "1" and choice != "2" and choice != "3": 
    choice = raw_input("pick 1, 2 or 3")

    if choice == "1":
        print "1 it is!"

    elif choice == "2":
        print "2 it is!"

    elif choice == "3":
        print "3 it is!"

    else:
        print "You should choose 1, 2 or 3"

While it works, I feel that it's really clumsy, specifically the while clause. What if I have more acceptable choices? Is there a better way to make the clause?

7
Are you always going to be printing choice, "it is" (?) or will these truly be separate cases? - Andy Hayden
@hayden The actual code is very different, I just simplified it to make the question more clear. - M830078h
I realise it was simplified (good work), but I was more asking: (if 1 do f(), if 2 do g(), ... rather than if 1 or 2, do f(). Since you accepted the answer, these can't be truly separate cases :). - Andy Hayden

7 Answers

47
votes

The while bit could be refactored a little to make it a little bit cleaner by checking if the element is within a list of choices like so

while choice not in [1, 2, 3]:

This is checking if the value of choice is not an element in that list

6
votes

You can push the logic into the loop, and replace

while choice != "1" and choice != "2" and choice != "3": 

with

while True:

and then the initial line choice = "" is unnecessary. Then, in each branch, once you're done what you want to do you can break.

4
votes

I think something like that would be better

possilities = {"1":"1 it is!", "2":"2 it is!", "3":"3 it is!"} 
choice = ""

while True:
    choice = raw_input("pick 1, 2 or 3")
    if choice in possilities:
        print possilities[choice]
        break
    else:
        print "You should use 1, 2 or 3"
1
votes

You can use a dictionary to map 1 to the code you want to execute when 1 is the value, and so on... That way you get rid of the ifs and your code can support other values in the future by simply updating the dictionary. As for the condition in the while, you just check if the key is in the dictionary.

1
votes

I'd suggest having a function which just loops until a valid option is chosen, then returns the chosen value.

This means the rest of your code is not intended inside the while, keeping everything nice and flat ("Flat is better than nested")

def get_choice(options):
    """Given a list of options, makes the user select one.

    The options must be strings, or they will never match (because raw_input returns a string)

    >>> yn_choices = ['y', 'n']
    >>> a = get_choice(options = yn_choices)
    """
    prompt_string = "Pick " + ", ".join(options)
    while True:
        choice = raw_input(prompt_string)
        if choice in options:
            return choice
        else:
            print "Invalid choice"

# Prompt user for selection
choice = get_choice(["1", "2", "3"])

# Do stuff with choice...
if choice == "1":
    print "1 it is!"

elif choice == "2":
    print "2 it is!"

elif choice == "3":
    print "3 it is!"

else:
    print "You should choose 1, 2 or 3"
0
votes

I think you can use a set which contains all of your possible choices and use "in" expression to judgefor the while part.

As for the if-else part, print (choice, " it is!") will be ok.

-1
votes

while str(choice) not in "123" .....