0
\$\begingroup\$

This is a question about some code I've written that works, but I'm not sure it's good code.

I would like to get some comments on this or if possible hints to build a better solution.

Background:

This is a small program to calculate some data, connect these to a participant and store it in a JSON file.

Here the already working solution:

menu_entries = (
                    ('Order Pizza', order_pizza),
                    ('Order Burger', order_burger),
                    ('Order Coke', order_coke, '0.5')
               )


def menu(menu_title, menu_entries):

    while True:
        mpoint = 0

        print(menu_title)

        # Build the menu entries plus one to close
        for entry in menu_entries:
            mpoint += 1
            print(str(mpoint) + '.', entry[0])

        print(str(mpoint + 1) + '.', 'Ende')

        # Get value and validate
        try:
            task = int(input('> '))
        except ValueError:
            task = None

        if task is None or task > mpoint + 1:
            task = None
            continue

        if task == mpoint + 1:
            print('Done')
            break

        # Build a function call
        get_task = menu_entries[task - 1][1]
        try:
            get_task(menu_entries[task - 1][2])
        except IndexError:
            get_task()


def order_pizza(size=None):
    if size is None:
        size = ''
    print('Bring me a Pizza!')


def order_burger(size=None):
    if size is None:
        size = ''
    print('One {} Burger please.'.format(size))


def order_coke(size=None):
    if size is None:
        size = ''
    print('A {} Coke please.'.format(size))


menu('Main menu', menu_entries)

As you can see the menu function gets a main title and a tuple of menu entries.

In these menu entries I could define the menu point title, a function name and one parameter.

All these other functions are only examples and are of no interest (they are ugly, I know).

Please have a look, criticize and if possible show me a better solution.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

You have an actual error in your code:

$ python test.py 
Traceback (most recent call last):
  File "test.py", line 2, in <module>
    ('Order Pizza', order_pizza),
NameError: name 'order_pizza' is not defined

You need to define menu_entries after creating the things it refers to.

Some notes:

  1. All Python code could benefit from being run through at least one linter such as flake8 or pycodestyle. Nothing springs out at me except possibly the indentation of menu_entries, but it's worth a check.
  2. Generally, names don't need to repeat anything which is understood from context. So def menu(menu_title, menu_entries): could be easier written as def menu(title, entries):
  3. You always add one to mpoint - a cleaner solution would be to mpoint = 1 and continue from there. If mpoint was used for indexing into an array that would be different, but it's really only for presentation.
  4. When reading the code top to bottom I should be able to understand what a name means the first time I encounter it. mpoint should be renamed to what it is (maybe menu_index?).
  5. For consistency (and to write more idiomatic code) the strings in all your print statements should be formatted.
  6. This is a prime example for code which could be made object oriented - obvious classes include Menu, MenuEntry and Order.
  7. Setting task = None and then checking if it's None to check whether you should continue is redundant - you can just continue immediately.
  8. size is unused in order_pizza, so it should be removed.
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks for the notes! Most of them are clear and I think would be no problem to eliminate. Point 6 and 8 are the same, right? One thing about point 7: How to do this with a class? I'm not really aware how classes work and have not found a good tutorial (don't want to build cars...). As already mentioned the helper functions are ugly. Only the menu function is relevant. \$\endgroup\$ Oct 24, 2018 at 9:09
  • \$\begingroup\$ None of the SO/SE sites will help you to learn foundational skills like OO (by design), and I'm afraid I can't give any good pointers to learn it. Sorry. \$\endgroup\$
    – l0b0
    Oct 24, 2018 at 19:38

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.