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\$
  • 3
    \$\begingroup\$ @sf31 Have you looking into the existing parsing facilities of argparse? Idk if it does this, but have you checked? \$\endgroup\$ – Alexander - Reinstate Monica Dec 23 '19 at 9:37
  • \$\begingroup\$ @CodeCaster Please provide answers in answers, not in comments. \$\endgroup\$ – Peilonrayz Dec 23 '19 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 '19 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 '19 at 12:29
  • \$\begingroup\$ @Alexander-ReinstateMonica I just skimmed it, seems helpful, I'll work on it. Thanks! \$\endgroup\$ – sf31 Dec 23 '19 at 20:29
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\$
  • 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\$ – c4llmeco4ch Dec 22 '19 at 21:13
  • 3
    \$\begingroup\$ this would fail if there are more than 10 elements \$\endgroup\$ – hjpotter92 Dec 23 '19 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 '19 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\$ – c4llmeco4ch Dec 23 '19 at 20:46
  • 1
    \$\begingroup\$ @sf31 instead of fruit_ls being a list, create a set \$\endgroup\$ – hjpotter92 Dec 24 '19 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\$
  • \$\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 '19 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\$ – c4llmeco4ch Dec 23 '19 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\$ – Maarten Fabré Dec 23 '19 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\$
  • \$\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 '19 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 '19 at 23:52

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.