3
\$\begingroup\$

I've just started learning python, mostly by some YouTube videos and other online tutorials.

Now I want to create a dictionary of callables (actions) asking for user input (a hotkey), but the user should also be able to pass more arguments to the actions.

My problem is, that my current solution doesn't seem very elegant and feels kind of wrong to me.

Is this really the only way to achieve what I want, or is there a better/correct implementation?

This is my example code so far:

from collections import OrderedDict

# functions that prints all the possible hotkys and a description

def add_action(actions: dict, hotkey: str, action: object, name: str):
    actions[hotkey.lower()] = action
    actions[hotkey.upper()] = action
    print('- {}: {}'.format(hotkey, name))

# all the functions that we want to execute

def do_a(name: str):
    print('Hello ' + name + ' this is A')


def do_b():
    print('This is B')


def do_c():
    print('This is C')

# function to create a dictionary with all the callables and the corresponding hotkyes

def get_available_actions():

    actions = OrderedDict()

    add_action(actions, hotkey='a', action=do_a, name='Action A will also print your name (use like a john')
    add_action(actions, hotkey='b', action=do_b, name='Action B')
    add_action(actions, hotkey='c', action=do_c, name='Action C')

    return actions

# the main function that is executed to ask the user what to do

def choose_action():
    action = None

    while not action:
        available_actions = get_available_actions()

        # this is the current solution to get arguments
        # first get the whole input
        command = input('\nWhat do you want to do? ')

        action_input = None
        arg = None

        # check if the user typed in more than one string spereated by a whitespace and split it
        if ' ' in command:
            action_input, arg = command.split(' ')
        else:
            action_input = command

        # now get the action based on the hotkey, which should have been the first argument
        # using get here to avoid, that the input is a "hotkey" that we don't have in our dictionary
        action = available_actions.get(action_input)

        if action:
            # and if we have an argument, pass it to the action
            if arg:
                action(arg.strip())
            else:
                action()
        else:
            print('Not found, choose again')


choose_action()

```
\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

So it is great that you are starting learning Python! It is a fun and easy language to get started using. I will echo m-alorda and recommend you use some library specifically designed to do this as there are many pitfalls.

However, if you want to know how to improve your coding keep reading.


General feedback

Code Tells You How, Comments Tell You Why?

You have tried to document your code through comments, which is great! However, comments using # are mostly intended for other programmers, and is usually reserved for explaining terse parts of the code. A common way to document your code -- which is intended for the user -- is with PEP 257 -- Docstring Conventions.

As stated in the title comments (#) should be used sparsely. First you should first strive to make your code as simple as possible to understand without relying on comments as a crutch. Only at the point where the code cannot be made easier to understand should you begin to add comments.

Your code is already quite clear so I would remove all your comments, and replace them with docstrings.


upper lower

This

actions[hotkey.lower()] = action
actions[hotkey.upper()] = action

Can be replaced by using lower on the input.

f-strings

f-strings are the new way of formating strings in Python, and you should usually use them. For instance

print('- {}: {}'.format(hotkey, name))

becomes

print(f'- {hotkey}: {name}')

Which is easier to read.

business logic and user interface

Business logic or domain logic is that part of the program which encodes the real-world business rules that determine how data can be created, stored, and changed. It is important to separate how you handle your data (business logic) from how you present it to the user (user interface).

So this

def do_a(name: str):
    print('Hello ' + name + ' this is A')

Should really be this

def do_a(name: str):
    return 'Hello ' + name + ' this is A'

Note that this allows us to change how we present the results from do_a, maybe we want to decorate, add or remove something before we present it to the user.


actions = OrderedDict()

I would rather just use a list instead of an OrderedDict, but we will get back to that.

add_action(actions, hotkey='a', action=do_a, name='Action A will also print your name (use like a john')
add_action(actions, hotkey='b', action=do_b, name='Action B')
add_action(actions, hotkey='c', action=do_c, name='Action C')

Here you are mixing two concepts, do you see which? nameis not really just a name is it? You use name as a name for B and C, but for A you also use it as a description. It would be better to seperate these two.

Secondly, note that your names are just the hotkey capitalized. So this could instead be

`f'Action {hotkey.upper()}'`

Again utilizing those nifty f-strings


if __name__ == "__main__":

Put the parts of your code that are the ones calling for execution behind a if __name__ == "__main__": guard. This way you can import this python module from other places if you ever want to, and the guard prevents the main code from accidentally running on every import.

So this

# the main function that is executed to ask the user what to do

def choose_action():
    action = None
.
.
.

becomes

def main():
"""Summary of the function goes here"""
.
.
.

`if __name__ == "__main__":
    main()

nitpicking

Use a proper linter / editor. A proper linter would probably warn you that action_input = None is never called. Do you see why?

    action_input = None
    if ' ' in command:
        action_input, arg = command.split(' ')
    else:
        action_input = command

You also had some minor spacing issues between your functions which is why I suspect you could use a better editor =)

user interface

I really like your user interface, it is mostly clear what you want your users to do. I would recommend some clearer naming

'\nWhat do you want to do? '

Is at best vague ^^


Suggestion

  • Implemented all the suggestions above.

  • Use a class to keep track of which functions we want to be callable (allowing every function is bad from a security standpoint).

  • We store the allowed actions as a namedtuple.

  • When adding functions we make sure they are callable.

  • Added options for more than one argument. This is done by *args

  • The printing is left to the __str__ part of the class.

  • Rewrote parts of the code to suit the new walrus operator :=

  • Rewrote parts of the code to have fewer exit points. Do note that I am not trying to strictly follow the single entry / single exit mantra, as it can be harmful in some circumstances. Sometimes, however, it can make the intent of the code clearer.

    before

    if hotkey.lower() in ACTIONS.hotkeys:
        print(ACTIONS.call_function_by_hotkey(hotkey, args))
    else: 
        print("Not found, choose again")
    

    after

    print(
        ACTIONS.call_function_by_hotkey(hotkey, args)
        if hotkey.lower() in ACTIONS.hotkeys
        else "Not found, choose again"
    )
    

    In the second the most important part comes first: the print. Then an conditional is applied. Both styles are fine, just be comfortable with both.

code

from collections import namedtuple


def do_a(name, bobby=""):
    string = f"Hello {name} this is A"
    return string if not bobby else f"{string}\nHello {bobby} this is {name}"


def do_b():
    return "This is B"


def do_c():
    return "This is C"


LINEWIDTH = 79
LINEBREAK = "\n" + "=" * LINEWIDTH + "\n"


class Actions:

    Action = namedtuple("Action", ["hotkey", "name", "function", "description"])

    def __init__(self):
        self.hotkeys = dict()
        self.functions = []

    def add(self, hotkey, function, description, name=None):
        if not self.is_function(function):
            raise NameError("Function " + function + "() is not defined")
        name = "Action " + hotkey.upper() if name is None else name
        self.hotkeys[hotkey] = self.Action(
            hotkey, name, function, description.capitalize()
        )
        self.functions.append(function)

    def is_function(self, function):
        try:
            if eval("callable(" + function + ")"):
                return True
        except NameError:
            return False
        except SyntaxError:
            return False

    def function_2_string(self, function, args, strings=True):
        return function + (
            "()"
            if len(args) == 0
            else "("
            + ", ".join(args if not strings else ["'" + str(arg) + "'" for arg in args])
            + ")"
        )

    def call_function_by_hotkey(self, hotkey, args):
        if function := self.hotkeys[hotkey].function if hotkey in self.hotkeys else "":
            return eval(self.function_2_string(function, args))
        raise NameError("Hotkey " + hotkey + " is not defined")

    def __str__(self):
        string = LINEBREAK + "ACTIONS".center(LINEWIDTH) + LINEBREAK
        action_strings = []
        for hotkey, name, _, description in self.hotkeys.values():
            action_strings.append(f"- {hotkey}: {name}")
            if description:
                action_strings.append("  -- " + description)
        return string + "\n".join(action_strings) + LINEBREAK


EXIT_ = set("", "exit", "return", "end", "break") 
ACTIONS = Actions()
ACTIONS.add(
    hotkey="a", function="do_a", description="args: (str) name \n optional: (str) bobby"
)
ACTIONS.add(hotkey="b", function="do_b", description="")
ACTIONS.add(hotkey="c", function="do_c", description="")


def main():
    title_txt = str(ACTIONS)
    title_txt += "\nPick an hotkey from the list above to perform that action\n"
    while (command := input(title_txt + "> ")) not in EXIT_:

        if " " in command:
            hotkey, *args = command.split(" ")
        else:
            hotkey, args = command, []

        print(
            ACTIONS.call_function_by_hotkey(hotkey, args)
            if hotkey in ACTIONS.hotkeys
            else "Not found, choose again"
        )


if __name__ == "__main__":
    main()
\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much for the immense feedback. I've written some of the comments just for the post here, but I'll review the real code more in regards to your suggestions. Helps a lot! \$\endgroup\$
    – ObAcht
    Commented Jun 22, 2021 at 5:17
2
\$\begingroup\$

Exception

Your code makes no attempt to determine if the called function takes arguments or not. If you provide an argument to do_b or do_c, the code will crash with:

TypeError: do_b() takes 0 positional argument by more were given

You should surround the call in a try ... except TypeError: ... block, and report to the user the problem, instead of crashing.

Exception

If the user enters more than one argument, the code crashes at:

action_input, arg = command.split(' ')

due to assigning a list of length 3 or more to a tuple of length 2. Worse, if they only give one argument, but accidentally used two or more spaces between the command and the argument, the problem will still exist. Finally, if they separate the command and the argument with a tab instead of space, the code won’t even recognize that an argument is present.

command.split() is a special variant of the split method. It splits the string on any white space character (spaces, tabs, etc). Moreover, it treats consecutive white space characters as only one separator, so accidentally using two spaces between the command and the argument is not a problem.

Much, much worse, your code is very inflexible. If you did want to allow 2 or more arguments, you would have to count spaces to try and guess at the correct number of arguments, and assign different variable names to each, like arg1, arg2, arg3, and so on. The code would get unmanageable very quickly. Instead, you want to use an argument list.

The splat operator (*) is used to expand a list of items into multiple arguments, and collect multiple arguments into a single list. We can use this to dramatically simplify the code.

    command = input('\nWhat do you want to do? ')

    action_input, *args = command.split()

    action = available_actions.get(action_input)

    if action:
        try:
            action(*args)
        except TypeError as type_error:
            print("Incorrect arguments:", type_error)
    else:
        print('Not found, choose again')

Notice there is no checking if any arguments are given, and no different paths for 0 arguments or 1 argument. If “a john” is given as input, it is split into ["a", "john"]. The assignment takes the first item of the list and assigns it to action_input, and the remaining items in the list are collected into the args variable as the list ["john"], where as if “b” is given as input, args becomes the empty list []. When action(*args) is called, if args has 0 items, it calls action with no arguments, if args has 1 item, it calls action with 1 argument, if args has 2 items, it calls action with 2 arguments, and so on. You are free to define functions with any number of arguments (including default arguments), and action(*args) will populate the function’s parameter list with the items in args.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback. Better understanding of Exception handling and sanitizing user input is also on my "to do/learning goals" list. Much apprrciated. Currently I've modified the code already to prevent these exceptions, I just didn't change the example here. But your suggestion is cleaner and I'll learn more about this. \$\endgroup\$
    – ObAcht
    Commented Jun 22, 2021 at 5:19
1
\$\begingroup\$

If you are not trying to reinvent the wheel, what you want is to use some library that allows you to easily create a CLI. For example, click.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback, I guess with Python I'll haveto learn a lot about the usage of libraries :) \$\endgroup\$
    – ObAcht
    Commented Jun 22, 2021 at 5:18

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.