13
\$\begingroup\$

I'm currently working on reincarnation of the Cactus project, Cactus Reborn, and I've gotten the basic implementation so far. At this point, you can create simple, playable games.

There are currently three usable classes/functions:

  • Location - This class describes data about a location in a GameFlowchart class, like a title, description, location references, etcetera.
  • GameFlowchart - This class describes the "map", or "flowchart" that the player traverses while playing the game.
  • play_game - This function runs the user-created game.

I'd like to know the following things:

  • Is it appropriate to require the user to specify argument names with *?
  • Where am I making things too complex? What can be simplified?
  • Am I over-documenting?
  • Are there any design issues that I should be concerned about here?
  • Is there a simpler way to have the user describe data for games?
  • Anything else?

location.py

"""
location.py

The classes and methods in this file are used to describe
a location, contained in a GameFlowchart class.
"""
import re
import sys
import time


class Location:
    """Represents a location in a GameFlowchart map.

    A location represents a "location" on the game's
    map, and contains data like a title, locations
    to other positions, etc.

    Keyword arguments:
    title       -- The title of the position.
    description -- The description of the position.
    locations  -- A dictionary of possible inputs, and reference keys.
    """
    def __init__(self, *, title, description_enter, description_exit, on_exit_function, locations):
        self.title = title
        self.description_enter = description_enter
        self.description_exit = description_exit
        self.on_exit_function = on_exit_function
        self.locations = locations

    def on_enter(self):
        """This function is run when the user enters.

        When the user "enters" a positions, this function will
        display the title, and position description.
        """
        print("-" * len(self.title))
        print(self.title)
        print(self.description_enter)

        if len(self.locations) != 0:
            print("Locations: ", end="")
            for key, value in self.locations.items():
                print(key + ", ", end="")

    def on_exit(self):
        """This function is run when the user exits.

        When the user "exits" as position, this function will
        display the exit message, and a newline.
        """
        print(self.description_exit)
        print("-" * len(self.description_exit) + "\n")

        if self.on_exit_function is not None:
            time.sleep(5)
            self.on_exit_function()

    def get_user_input(self, prompt, error_message, case_sensitive, global_commands):
        """Get user input, and check to make sure it's valid.

        This function takes user input, sanitizes it, and
        then checks to make sure that it's valid by checking
        it against the locations. If it is valid, then the
        referenced map key is returned. It it's invalid, then
        the function returns None.

        Keyword arguments:
        prompt        -- The prompt to use with user input.
        error_message -- The error message to display when the input is invalid.
        """
        if self.on_exit_function != sys.exit:
            user_input = re.sub(r"([^\s\w]|_)", "", input("\n" + prompt).strip())
            user_input = user_input.lower() if not case_sensitive else user_input
            if user_input in self.locations:
                return self.locations[user_input]

            elif user_input in global_commands:
                global_commands[user_input]()
                return

            print(error_message)
            return

game_flowchart.py

"""
game_flowchart.py

The classes and methods in this file are used to create a
flowchart. A flowchart is essentialy a "map" of how your
game is played.
"""
import re


class GameFlowchart:
    """Represents a flowchart, or "map".

    This class is a representation of a "map" in a
    Cactus game. Essentially, a Flowchart is a
    dictionary, where the keys are locations to
    each position.

    Keyword arguments:
    locations -- A dictionary of socks.
    """
    def __init__(self, *, locations):
        self.locations = locations

    def iterate_locations(self):
        for key, value in self.locations:
            yield (key, value)

    def find_by_reference(self, reference):
        """Find, and obtain the value of a position, by reference.

        This function checks to see if a reference value is in
        the dictionary of locations. If it is, then return the
        value, and if it isn't, then return None.

        Keyword arguments:
        reference -- The reference value.
        """
        if reference in self.locations:
            return self.locations[reference]
        return

    def find_start(self):
        """Returns the "starting position" in the locations.

        This function will find the starting position in
        the locations. It does not matter if the starting
        position is capitalized, contains odd characters, or
        has spaces. As long as it contains the characters
        "start", it (should) work. This will return None
        if the item is not found.
        """
        for key, item in self.locations.items():
            if re.sub(r"([^\s\w]|_)", "", key) == "start":
                return self.locations[key]
        return

main_game.py

"""
main_game.py

The classes and methods in this file are used to create,
and specify and additional data about your game, and then
wrap it up in a nice container, ready to play.
"""
import sys


def play_game(*, name, description, prompt, flowchart, case_sensitive, error_message, global_commands):
    """Play a user-created game.

    Keyword arguments:
    name            -- The name of the game.
    description     -- The description of the game.
    prompt          -- The game's prompt.
    flowchart       -- The flowchart that the player traverses.
    case_sensitive  -- Whether or not user input is lowered.
    error_message   -- The message to display when the user enters invalid input.
    global_commands -- Global commands that can be executed anywhere.
    """
    current_location = flowchart.find_start()

    if current_location is not None:
        while True:
            current_location.on_enter()
            new_key = current_location.get_user_input(prompt, error_message, case_sensitive, global_commands)
            current_location.on_exit()

            if new_key is not None:
                if new_key in flowchart.locations:
                    current_location = flowchart.locations[new_key]
                else:
                    raise KeyError("Invalid location key \"{0}\"".format(new_key))

The below is a very simple example game, but I think it shows what you can do with Cactus.

import sys
import cactus


FLOWCHART = cactus.game_flowchart.GameFlowchart(
    locations={
        "start": cactus.location.Location(
            title="The Shire",
            description_enter="As you enter the Shire, you are surrounded by the endless rolling hills.",
            description_exit="As you leave the Shire, you look back and wish that you could stay longer.",
            on_exit_function=None,
            locations={
                "mordor": "mordor",
                "laketown": "laketown"
            }
        ),
        "mordor": cactus.location.Location(
            title="Mordor",
            description_enter="As you enter Mordor, the Dark Lord Sauron spots you and kills you.",
            description_exit="As you pass out of Arda, you reflect on your bad decision.",
            on_exit_function=sys.exit,
            locations={}
        ),
        "laketown": cactus.location.Location(
            title="Laketown",
            description_enter="As soon as you enter Laketown, you realize that there is no Laketown.",
            description_exit="You leave Laketown, disappointed.",
            on_exit_function=sys.exit,
            locations={}
        )
    }
)

cactus.main_game.play_game(
    name="LOTR Quest",
    description="Some dumb LOTR quest.",
    prompt="> ",
    flowchart=FLOWCHART,
    case_sensitive=False,
    error_message="Enter the correct input!",
    global_commands={
        "exit": sys.exit
    }
)

If you're interested about Cactus, you can visit the new repository, the /r/CactusEngine subreddit, or the official chat room.

\$\endgroup\$
13
\$\begingroup\$

Is it appropriate to require the user to specify argument names with *?

Yes! I think this is excellent practice, and in my opinion a far underused feature of Python 3. It greatly enhances the readability of your function when called.

Reading just the invocation of play_game() in your simple example, I can immediately work out what all of the arguments mean. That would be impossible if these arguments were specified positionally. I don’t think it imposes much burden on the caller, but it’s easier to read.


Where am I making things too complex? What can be simplified?

A few suggestions for things I’d tidy up:

  • It’s great that you enforce keyword-only arguments in the play_game() function; I would also add default arguments so that I can omit keywords that I’m not using. That can cut down on visual noise when the function is called.

  • The presence of on_enter() and on_exit() methods in the Location class make me twitchy. I feel like that’s just asking to be tidied up into a context manager, so that in main_game.py I can type:

    with current_location as loc:
        loc.get_user_input(prompt, error_message, case_sensitive, global_commands)
    

    That’s a bit more Pythonic, and eliminates the risk that I’ll forget the on_exit() method.

  • In the on_enter() method of Location, you can tidy up the for loop:

    print(", ".join(self.locations.keys()))
    

    That’s more Pythonic, shorter, and gets rid of the trailing comma.


Am I over-documenting?

I don’t think you’re over-documenting, but some of the docstrings are quite wordy. People can have an aversion to reading lots of text; you can probably convey the same information with less words. There are also places where the docstring is getting mixed up with implementation details.

For example:

Find, and obtain the value of a position, by reference.

This function checks to see if a reference value is in the dictionary of locations. If it is, then return the value, and if it isn't, then return None.

This docstring essentially describes how the function works. Really, all the information it should contain is something like:

Get the value of a position from a reference. Returns None if the position is not found.

Separately, I’d consider wording this to sound less like ‘get a value by reference’, just because that choice of words has me thinking of C.


Are there any design issues that I should be concerned about here?

Nothing jumps out at me, but it’s a Sunday evening and I’m tired.

At best, that’s an endorsement that there aren’t any glaring holes, not that the design is flawless.


Is there a simpler way to have the user describe data for games?

As stated above, don’t force me to specify every argument for every location – have some sensible defaults.

The structure of the function calls looks quite like JSON – perhaps you could allow the user to provide a JSON file (with a specified structure) that initialises the game.


Anything else?

Here are other miscellaneous suggestions:

  • In game_flowchart.py, the docstring describes the locations argument as “a dictionary of socks”. Perhaps I’m missing something, but I have no idea what a sock is in this context. Some more explanation would be useful.

  • In a couple of places, you check for a value in a dictionary before retrieving it. Rather than doing

    if key_foo in dict_bar:
        return dict_bar[key_foo]
    return
    

    look at using the .get() method on the dictionary:

    return dict_bar.get(key_foo)
    

    The get() method looks up a key and returns a default if the key is missing; if no default is supplied, then it falls back to None.

  • In the GameFlowchart class, I think there’s a bug in your iterate_locations() method. I think you want to iterate over self.locations.items(), not self.locations. If I try the current implementation, I get a TypeError:

    >>> from cactus import game_flowchart as gf
    >>> cities = gf.GameFlowchart(locations={1: "london", 2: "basel", 3: "milan"})
    >>> for city in cities.iterate_locations():
    ...     print(city)
    ...
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/cactusgame/cactus/game_flowchart.py", line 26, in iterate_locations
        for key, value in self.locations:
    TypeError: 'int' object is not iterable
    
  • There are a few slightly snarly regexes. I could unpick them, but it would be easier if there was a comment explaining what they’re supposed to do.

  • Here’s some slightly weird output:

    ---------
    The Shire
    As you enter the Shire, you are surrounded by the endless rolling hills.
    Locations: mordor, laketown,
    > narnia
    Enter the correct input!
    As you leave the Shire, you look back and wish that you could stay longer.
    --------------------------------------------------------------------------
    
    ---------
    The Shire
    As you enter the Shire, you are surrounded by the endless rolling hills.
    Locations: mordor, laketown,
    > 
    

    Since I entered incorrect input, I haven’t left the Shire yet. So why am I seeing the message about looking back on it?

  • In the play_game() function, I’d turn the initial check into a guard clause and save yourself a level of indentation. That is:

    def play_game(*args, **kwargs):
        current_location = get_current_location()
        if current_location is None:
            return
        # rest of function here
    

    I think that makes the function easier to read.

  • Part of Python’s philosophy is EAFP, or Easier to Ask Forgiveness than Permission.

    There are several points in your code where you check for a key in a dictionary, and only go ahead and retrieve it after you know that it’s there. Under this philosophy, it would be better to just lookup the key, and catch the KeyError if it’s missing.

    For example, in the play_game() function, consider:

    try:
        current_location = flowchart.locations[new_key]
    except KeyError:
        raise KeyError('Invalid location key "{}"'.format(new_key))
    
  • I would modify the output to make the location stand out a little more, e.g.

    ----------
    # Laketown
    
    As soon as you enter Laketown, you realize that there is no Laketown.
    You leave Laketown, disappointed.
    ---------------------------------
    

    Also note that the closing line is only as wide as the final statement, not the overall message. That looks a bit odd – perhaps just extend to 79 characters and be done with it?

    If you want to get fancy and read the width of the user’s terminal and size your line appropriately, Ir recommend Thomas Ballinger’s PyCon talk Terminal Whispering, which has a bunch of fancy tricks for this sort of thing.

  • I am wary of letting a user specify functions that can be triggered by text input (this is the arguments parameter). Perhaps I’m paranoid, but that feels like a bit of a security risk.

    There are surely only a limited number of commands that could be used here – exit, restart, undo – so I would just hardcode those possibilities, and get rid of this option.

    I would also give them a special prefix, e.g. !exit, so that these commands can’t be confused with regular options.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Whoops, the "sock" part was left over from me auto-typing while chatting in The 2nd Monitor. \$\endgroup\$ – Ethan Bierlein Sep 20 '15 at 22:15

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.