A four-item text-based menu in Python

Question

I picked up python again, and I am proud with what I have without using if/elif statements throughout the whole thing. I have modified this couple of times, since I kept asking how could I make it better or more organized. My biggest complaint is using the if/elif at the bottom, for some reason when I use.

func = optionMenu.get(test, "INVALID ENTRY")

it would would return blank instead of showing "invalid entry" if I selected option that wasn't on the menu. So, I had to use if/else to band-aid it for now.

#!/usr/bin/env python

import os



def mainMenu():

    print "=" * 25
    print "Please Select An Option"
    print "=" * 25
    print ""
    print "1. option one\n"
    print "2. option two\n"
    print "3. option three\n"
    print "4. filler\n"
    menuOption = input("Select Option: ")

    optionSelect(menuOption)

class selections(object):
    def one(self):
        print "option one"

    def two(self):
        print "option two"

    def three(self):
        print "option three"

    def four(self):
        print "option four"

def optionSelect(test):
    menuSelection = selections()
    optionMenu = { 
    1 : menuSelection.one, 
    2 : menuSelection.two, 
    3 : menuSelection.three, 
    4 : menuSelection.four,
    }

    func = optionMenu.get(test)
    if test in optionMenu:
        func()
    else:
        print "ERROR INVALID ENTRY ERROR"
mainMenu()

Answer 1

I think, there is room for some improvement:

1) You are mixing OOP with procedural code

If you choose one paradigm, you should stick with it. Besides: why are you abstracting selections into a class and not the whole menu?

2) DRY - Don't repeat yourself

print "=" * 25
print "Please Select An Option"
print "=" * 25

Why are you repeating the same task? You could refactor the "=" * 25 into a constant: separator = "=" * 25.

Everytime you need a separator, you could use separator, which is more meaningful.

3) You should separate concerns

print "4. filler\n"
menuOption = input("Select Option: ")

Here you are mixing two concerns printing the options and prompting for input. This pollutes your mainMenu. One function should only do one thing (SRP).

4) Your code lacks an overall structure

If you take every point together, this should now be clear.

---

I came up with the following:

#!/usr/bin/env python2

import os
from collections import namedtuple

class Menu():

    Option = namedtuple('Option', 'label')
    _separator = "=" * 25
    _options = {1: Option("option one"), 2: Option("option two"),
                3: Option("option three"), 4: Option("option four")}

    def print_header(self):
        print "{0}\n Please Select An Option\n{0}\n".format(self._separator)

    def print_mainMenu(self):
        self.print_header()
        for option in sorted(self._options.keys()):
            print "{0} {1}".format(option, self._options[option].label)

    def prompt(self):
        return input("Select Option: ")

    def handle_input(self, chosen_option):
        try:
            print self._options[chosen_option].label
        except KeyError:
            print "Wrong Option"


def main():
    menu = Menu()
    menu.print_mainMenu()
    menu.handle_input(menu.prompt())

if __name__ == '__main__':
    main()

For this example, the use of namedtuple is not necessary, but on the other hand gives you something like a lightweight object which could not only contain lables. On top options[chosen_option].label is very readable.

---

Answer 2

Since you weren't very clear on which aspects of your code you want to have reviewed, here are several:

1. PEP 8: Standard guidelines. I can see several deviation from standard. It is always a good practice to follow standard guidelines, even in very short and simple programs.

2. Documentation. No documentation provided at either class level, module level, function level or inline.

3. Design. Right now, functions option_select and main_menu and class selections all assume that there are four options overall. What happens if you change the number of options in one of them, but forgot to update the rest? Think of the way so that you do not have to be manually editing everything once a single change occurs.

4. Shebangs. If you don't have good reason to put it, don't put it. More info: Shebangs

5. Unused imports. Why have you imported os?

6. You said you wanted to avoid last if/else statement. There are two ways I can think of:

   1. Have another method error in your class Selection which prints your error message, then:

      func = option_menu.get(test, menu_selection.error)
      func()
      

   2. Instead of printing statements in class functions, return the statements, then:

      print option_menu.get(test, "ERROR MESSAGE")
      

Answer 3

A few years later, but I just found this so I hope my answer will help others.

From my understanding of SRP, which can be explained simply by saying, "A class should have only one reason to change." For this reason I feel that the example Thomas Junk provided, while good, isn't exactly following the SRP as it states the options internally and also prints the information directly from the class methods. Here is what I think would be more appropriate in 2018:

Option = namedtuple('Option', ['label', 'callback'])

class Menu:

    SEPARATOR = '-'

    _title = ''
    _options = []

    def __init__(self, title, options):
        self._title = title

        for option in options:
            self._options.append(Option(option[0], option[1]))

    def header(self, text):
        line = self.SEPARATOR * (len(text) + 2)
        return f"{line}\n {text}\n{line}\n"

    def display(self):
        string = self.header(self._title)

        for i, option in enumerate(self._options):
            string += f"{i + 1} {option.label}\n"

        return string

    def callback(self, i):
        if i <= len(self._options):
            return self._options[i - 1].callback

Which could then be implemented with the following:

game_is_running = True

main_menu = Menu(
    "Main Menu - Please Select an Option", [
    ('New Game', new_game),
    ('Load Game', load_game),
    ('Options', display_options),
    ('Exit', exit_game)])

print(main_menu.display())

while game_is_running:
    option = int(input('>> '))
    main_menu.callback(option)()

The added benefits of this are that you can create a menu without having to change the class directly, unless you want to change how the menu itself is displayed.

Answer 4

You can get rid of that if test by defining a function that produces (or raises) the error message:

In [19]: def foo():
   ....:     print "ERROR"

In [20]: optionMenu.get('test',foo)()
ERROR

In [21]: optionMenu.get(1,foo)()
option one

That function could be a selections method

class Selections...   # Capitolize classes
    ....
    def error():
       print 'error'

 optionMenu.get(test, menuSelection.error)()

In other words, if you want the .get to return a callable, regardless of what key you give it, then the default should itself be a callable.

There formatting and documentation issues, but none of that falls under the heading of 'optimize'.

Instead of the repeated print you could format a input text with triple quotes:

prompt = """
=========================
input 
options:
   one
   two 
   ...
=========================
"""
....
print prompt

I'd be tempted to make the optionMenu dictionary part of the Selections class, may be as a class variable. But that depends a lot on whether Selections is used just for this input, or whether it has other purposes.

As a further exercise you might try replacing the interactive input with a commandline input. In other words, use something like argparse to parse sys.argv, and then act on one of the Selections.