3
\$\begingroup\$

I'm writing a tool which users can select multiple choices at once. Whilst I can't provide the actual content, I've replicated it with fruit. I'm replicating some functionality from Nikto where it allows different flags like "-mutation abcd".

for example here we have three fruits:

1- apple
2- orange
3- kiwi

I wanna let users to type "12" or "appleorange" or "21" or "orangeapple" for selecting apple and orange, or "123" or "appleorangekiwi" or vice versa to select apple, orange and kiwi and etc.

Assume that there are more than 3 choices, so it would be pain in the neck if I want to write elif statement for all multiple selections. How can I make my code simpler and shorter?

Here is my code:

fruits_ls = []


def add_fruits():
    while True:
        print("choose your favorite fruits:\n"
            "1- apple\n"
            "2- orange\n"
            "3- kiwi\n"
            "4- exit\n")

        my_fruits = input()
        if my_fruits == str(1) or my_fruits == "apple":
            fruits_ls.append("Apple")
        elif my_fruits == str(2) or my_fruits == "orange":
            fruits_ls.append("Orange")
        elif my_fruits == str(3) or my_fruits == "kiwi":
            fruits_ls.append("Kiwi")
        elif my_fruits == str(12) or my_fruits == "appleorange" or my_fruits == str(21) or my_fruits == "orangeapple":
            fruits_ls.append("Apple")
            fruits_ls.append("Orange")

        elif my_fruits == str(13) or my_fruits == "applekiwi" or my_fruits == str(31) or my_fruits == "kiwiapple":
            fruits_ls.append("Apple")
            fruits_ls.append("Kiwi")

        elif my_fruits == str(23) or my_fruits == "orangekiwi" or my_fruits == str(32) or my_fruits == "kiwiorange":
            fruits_ls.append("Orange")
            fruits_ls.append("Kiwi")

        elif my_fruits == str(4) or my_fruits == "exit":
            break


add_fruits()

print(fruits_ls)
\$\endgroup\$
5
  • 3
    \$\begingroup\$ @sf31 Have you looking into the existing parsing facilities of argparse? Idk if it does this, but have you checked? \$\endgroup\$
    – Alexander
    Dec 23, 2019 at 9:37
  • \$\begingroup\$ @CodeCaster Please provide answers in answers, not in comments. \$\endgroup\$
    – Peilonrayz
    Dec 23, 2019 at 10:45
  • \$\begingroup\$ @Peilonrayz that was not an answer, not even a partial one. It was a warning towards the OP, so they could mention that in their question. \$\endgroup\$
    – CodeCaster
    Dec 23, 2019 at 12:09
  • \$\begingroup\$ @CodeCaster It was more than enough to be an answer on Code Review. Note that all critiques of code are answer material, and so warnings on edge cases or unforeseen input are fair game in answers. \$\endgroup\$
    – Peilonrayz
    Dec 23, 2019 at 12:29
  • \$\begingroup\$ @Alexander-ReinstateMonica I just skimmed it, seems helpful, I'll work on it. Thanks! \$\endgroup\$
    – sf31
    Dec 23, 2019 at 20:29

3 Answers 3

11
\$\begingroup\$

Creating a mapping from input to output can be very useful here. A dictionary is the ideal data structure for this

VALID_INPUTS = {
    '1': 'Apple',
    'apple': 'Apple',
    '2': 'Orange',
    'orange': 'Orange',
    '3': 'Kiwi',
    'kiwi': 'Kiwi',
}
EXIT_INPUTS = ['4', 'exit']

Then you can loop over this mapping and append the matching inputs

if my_fruits in EXIT_INPUTS:
    break
for valid_input, fruit in VALID_INPUTS.items():
    if valid_input in my_fruits:
         fruits_ls.append(fruit)
\$\endgroup\$
11
  • 1
    \$\begingroup\$ A note for op if he opts to use this implementation: you'll need to reconfigure how you exit the while loop. A simple input asking if the user wants to enter more options that trips a boolean controlling the while loop would work \$\endgroup\$ Dec 22, 2019 at 21:13
  • 3
    \$\begingroup\$ this would fail if there are more than 10 elements \$\endgroup\$
    – hjpotter92
    Dec 23, 2019 at 4:51
  • 3
    \$\begingroup\$ @Graham you're right. for eg. consider 10=pineapple, 11=guava; how would input 114 or 1104 be parsed? \$\endgroup\$
    – hjpotter92
    Dec 23, 2019 at 9:20
  • 1
    \$\begingroup\$ Simply add ``` and '<fruitnamehere>' not in fruits_ls ``` to the end of each if. This confirms the item is not already in the cart before adding it. You will need to add parens around the or section of the if \$\endgroup\$ Dec 23, 2019 at 20:46
  • 1
    \$\begingroup\$ @sf31 instead of fruit_ls being a list, create a set \$\endgroup\$
    – hjpotter92
    Dec 24, 2019 at 3:29
3
\$\begingroup\$

The first thing I would like to do is applaud you for not immediately turning to back-to-back if statements. It's a trap I commonly see when tackling these kinds of problems. However, in this case, I'd ask a question that is counter to my previous point: are your options mutually exclusive? In other words, is it a problem if a person has multiple options in a loop accepted? Given you are accepting inputs like 31 or 12, my expectation is no. As such, instead of:

if my_fruits == str(1) or my_fruits == "apple":
    fruits_ls.append("Apple")
elif my_fruits == str(2) or my_fruits == "orange":
    fruits_ls.append("Orange")
elif my_fruits == str(3) or my_fruits == "kiwi":
    fruits_ls.append("Kiwi")
elif my_fruits == str(12) or my_fruits == "appleorange" or my_fruits == str(21) or my_fruits == "orangeapple":
    fruits_ls.append("Apple")
    fruits_ls.append("Orange")

elif my_fruits == str(13) or my_fruits == "applekiwi" or my_fruits == str(31) or my_fruits == "kiwiapple":
    fruits_ls.append("Apple")
    fruits_ls.append("Kiwi")

elif my_fruits == str(23) or my_fruits == "orangekiwi" or my_fruits == str(32) or my_fruits == "kiwiorange":
    fruits_ls.append("Orange")
    fruits_ls.append("Kiwi")

elif my_fruits == str(4) or my_fruits == "exit":
    break

A possibility would be checking if an instance of an option is inside the input:

if '1' in my_fruits or 'apple' in my_fruits:
    fruits_ls.append('Apple')
if '2' in my_fruits or 'orange' in my_fruits:
    fruits_ls.append('Orange')
if '3' in my_fruits or 'kiwi' in my_fruits:
    fruits_ls.append('Kiwi')
if '4' == my_fruits:
    break

There are some small problems with the above solution that may or may not be in scope. For example, "121" will only give "AppleOrange" when "AppleOrangeApple" may be desired. I can provide further insight if this is the case.

Hopefully this helps. Assuming the need for elifs is always a safe bet at first, but when refactoring code, elifs can be ditched if the answers can coexist without hindering the code's performance. Let me know if I can help further.

\$\endgroup\$
3
  • \$\begingroup\$ I guess this is the correct answer, I'll check the code tonight and let you know. Fortunately there is no option like "121". So you mean that I should change elifs and rewrite the code like the "ifs" you provided, right? because I have some other functions which works almost like this one. \$\endgroup\$
    – sf31
    Dec 23, 2019 at 5:54
  • 1
    \$\begingroup\$ Right. The idea is elifs imply one or the other can happen, but not both. Like in a sports game, both teams cannot win at the same time. However, this problem does not have that concern. A user can have both an apple and a kiwi at the same time. Thus, we can use ifs rather than being concerned with elifs. \$\endgroup\$ Dec 23, 2019 at 8:00
  • 1
    \$\begingroup\$ Even if they are not mutually exclusive, you can adopt a data structure and a loop to make it more clear and expendable, as in codereview.stackexchange.com/a/234504/123200 \$\endgroup\$ Dec 23, 2019 at 8:17
3
\$\begingroup\$

This could be made into a generic function that accepts a list of items. It displays a menu based on the list and lets the user select one or more items from the menu. Rather than keep a global list, the function returns the list of select items.

def select_items(prompt, items):
    selected_items = set()

    while True:
        print(f"{prompt}:")
        for n, item in enumerate(items, 1):
            print(f"    {n:2} - {item}")
        exit_no = n+1
        print(f"    {exit_no:2} - exit")

        raw_selection = input("> ")
        selection = raw_selection.lower()

        for n, item in enumerate(items, 1):
            if item in selection or str(n) in selection:
                selected_items.add(item)

        if 'exit' in selection or str(exit_no) in selection:
            return selected_items


select_items("Choose your favorite fruits", "apple orange kiwi banana".split())
\$\endgroup\$
2
  • \$\begingroup\$ It works but the issue is still there, if users type each item again and again, the item will be shown in the output twice. like: Choose your favorite fruits: 1 Choose your favorite fruits: 1 Result: Apple, Apple \$\endgroup\$
    – sf31
    Dec 23, 2019 at 20:50
  • 1
    \$\begingroup\$ @sf31 - Then change selected_items to a set() instead of a list (see revised answer). Or check to see if the item is already in selected_items before appending it. \$\endgroup\$
    – RootTwo
    Dec 23, 2019 at 23:52

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.