2
\$\begingroup\$

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()
\$\endgroup\$
  • 1
    \$\begingroup\$ forgive my ignorance, but, why is having no if/else in your code a desirable goal? \$\endgroup\$ – PabTorre Sep 12 '15 at 14:49
3
\$\begingroup\$

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.


\$\endgroup\$
  • 1
    \$\begingroup\$ Is there a particular reason why you changed '=' * 25 to '=' * 26? \$\endgroup\$ – mkrieger1 Sep 13 '15 at 9:59
  • \$\begingroup\$ Not really ... Typo ;) \$\endgroup\$ – Thomas Junk Sep 13 '15 at 10:07
  • \$\begingroup\$ This is very informative. I read SRP you linked. That explains quite a bit, whenever Iooked at source code for java/C# i wondered why they made separate class for barely any code in it (it might not have been SRP thing, though) \$\endgroup\$ – andyADD Sep 13 '15 at 15:27
  • \$\begingroup\$ You could break it down to the following: small units which do one thing are easy to maintain and nice building blocks for complex systems. This is not limited to classes or OOP. \$\endgroup\$ – Thomas Junk Sep 13 '15 at 16:14
2
\$\begingroup\$

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")
      
\$\endgroup\$
  • \$\begingroup\$ Thank you for the constructive criticism! You're right I need to remove import os, it was there from previous version of the script. I don't follow about the shebang? Is there something wrong with it? Should I specify which version of python in shebang (ex. python2.7). The prints in each method is temp (which I should've mentioned) \$\endgroup\$ – andyADD Sep 12 '15 at 11:23
  • \$\begingroup\$ No, shebangs are simply not required. Preferably, I keep it in only the scripts which I need to invoke from command line. It is matter of taste though, but you must know when that they are required only in very few programs. I suspected that print statement might be just a placeholder, that why I suggested the first method of defining a method for holding error. \$\endgroup\$ – kushj Sep 12 '15 at 11:44
2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

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.