8
\$\begingroup\$

My rational for this program, was to learn more of Python's standard library, I didn't actually know cmd was even in it until I started this program. It's also the first community challenge, and I have some catching up to do.

This is an overly engineered version of the famous Rock Paper Scissors game. By default it's the exact same, but allows for people to define their own weapons, such as Rock Paper Scissors Lizard Spock, or the less liked Rock Paper Scissors Chainsaw. To do this I used configparser for an INI like input format. The sections are the winners, where the keys that follow are the losers. I also allow keys to be passed values which are the winning action, defaulting to 'beats'. Here is an example of a configuration file used for Rock Paper Scissors Lizard Spock (saved as rpsls, and used in the example):

[rock]
scissors: crushes
lizard: crushes

[paper]
rock: covers
spock: disproves

[scissors]
paper: cuts
lizard: decapitates

[lizard]
paper: eats
spock: poisons

[spock]
rock: vaporizes
scissors: smashes

To simplify the usage of this program I made some simple directed Graph and Node classes that inherit from dict. Which I think follow SOLID.

I also used cmd to create a simple command line interface. It didn't need to be anything fancy, and so it seemed like the best, and simplest, solution.

This also only works in Python 3.6, as I used f-strings.

import cmd
import configparser
import itertools
import random

class RPSException(Exception):
    pass


class DirectedGraph(dict):
    class Node(dict):
        __slots__ = ['name', '_graph']
        def __init__(self, graph, name):
            self.name = name
            self._graph = graph

        def add_edge(self, node, edge):
            self[node.name] = edge

    def adjacent(self, x, y):
        return x in self[y] or y in self[x]

    def add_node(self, *args, **kwargs):
        node = self.Node(self, *args, **kwargs)
        self[node.name] = node

    def add_edge(self, node_x, node_y, edge):
        self[node_x].add_edge(node_y, edge)

    def missing_edges(self):
        for keys in itertools.combinations(self.keys(), 2):
            if not self.adjacent(*keys):
                yield keys



class RPSGraph(DirectedGraph):
    class Node(DirectedGraph.Node):
        def __str__(self):
            return f'{self.name}:\n  '\
                   + '\n  '.join(f'{v[1]} {v[0].name}' for v in self.values())

        def add_edge(self, node_name, action):
            node = self._graph[node_name]
            edge = (node, action)
            super().add_edge(node, edge)

    def __str__(self):
        return '\n'.join(str(node) for node in self.values())



def parse_config(cfg):
    graph = RPSGraph()
    for node in cfg.sections():
        graph.add_node(node)

    for winner in cfg.sections():
        for looser, action in cfg[winner].items():
            graph.add_edge(winner, looser, action or 'beats')

    missing = list(graph.missing_edges())
    if missing:
        raise RPSException(f"Invalid configuration file. You're missing vertices between: {missing}")

    return graph


class RPS(cmd.Cmd):
    intro = "Welcome to Rock, Paper, Whatever. Type help or ? to list commands.\n"
    prompt = '> '

    cfg = configparser.ConfigParser(allow_no_value=True)
    cfg.read_string('''\
    [rock]
    scissors: crushes

    [paper]
    rock: covers

    [scissors]
    paper: cuts
    ''')
    graph = parse_config(cfg)
    del cfg

    def do_load_file(self, filename):
        '''Load a custom variation of rock paper scissors from a file.'''
        cfg = configparser.ConfigParser(allow_no_value=True)
        cfg.read(filename)
        self.graph = parse_config(cfg)

    @staticmethod
    def read_lines():
        try:
            while True:
                yield input()
        except KeyboardInterrupt:
            pass


    def do_load_stdin(self, _):
        '''Load a custom variation of rock paper scissors from the command line. Send a keyboard interrupt to end input.'''

        data = '\n'.join(self.read_lines())

        cfg = configparser.ConfigParser(allow_no_value=True)
        cfg.read_string(data)
        self.graph = parse_config(cfg)

    def do_play(self, weapon):
        '''Play a weapon. E.g. play rock'''
        graph = self.graph
        if weapon not in graph:
            print('Invalid weapon.')
            return

        ai = random.choice(list(graph.keys()))
        if ai == weapon:
            print('You draw')
        elif ai in graph[weapon]:
            print(f'You win. {weapon.capitalize()} {graph[weapon][ai][1]} {ai}')
        else:
            print(f'You loose. {ai.capitalize()} {graph[ai][weapon][1]} {weapon}')

    def do_weapons(self, _):
        '''List usable weapons.'''
        print(', '.join(self.graph.keys()))


if __name__ == '__main__':
    RPS().cmdloop()

Example usage:

Welcome to Rock, Paper, Whatever. Type help or ? to list commands.

> help

Documented commands (type help <topic>):
========================================
help  load_file  load_stdin  play  weapons

> help play
Play a weapon. E.g. play rock
> help weapons
List usable weapons.
> weapons
rock, paper, scissors
> play rock
You win. Rock crushes scissors
> load_file rpsls
> weapons
rock, paper, scissors, lizard, spock
> play rock
You loose. Spock vaporizes rock
> load_stdin
[rock]
scissors: crushes

[paper]
rock: covers

[scissors]
paper: cuts
> weapons
rock, paper, scissors
> play rock
You draw
>
\$\endgroup\$
  • \$\begingroup\$ I think some win/loose statistics would be nice :) \$\endgroup\$ – Graipher Feb 20 '17 at 13:19
  • \$\begingroup\$ @Graipher That's a pretty cool suggestion, I guess this'll be an iterative review then :) \$\endgroup\$ – Peilonrayz Feb 20 '17 at 13:22
  • \$\begingroup\$ I would consider swapping the INI file noun/verb to make it more readable... (and *lose) \$\endgroup\$ – Aaron D. Marasco Feb 20 '17 at 13:24
  • \$\begingroup\$ @AaronD.Marasco Suggest an edit, :) \$\endgroup\$ – Peilonrayz Feb 20 '17 at 13:25
  • 1
    \$\begingroup\$ @Quuxplusone I don't know what I done to trigger an answer invalidation comment. But as the question is currently un-answered fixing spelling mistakes is not yet against site policy. But thanks for the heads up :) \$\endgroup\$ – Peilonrayz Feb 21 '17 at 7:51
5
\$\begingroup\$

First things first:

1.PEP8

Please run any linter, since your code violates PEP8.

  • Tripple double quotes should be used in doctrings

  • Linebreaks(backslashes) are not recommended, preferred way is parenthesis

2. Nested classes

Speaking about your graph class structure. I try to avoid inner(nested) classes since they cause more troubles(e.g pickling, testing, less readable for me) rather than helping you defining a scope.

3. Code improvements

class RPS(cmd.Cmd):
    ...
    cfg = configparser.ConfigParser(allow_no_value=True)
    cfg.read_string('''\
    [rock]
    scissors: crushes

    [paper]
    rock: covers

    [scissors]
    paper: cuts
    ''')
    graph = parse_config(cfg)
    del cfg
    ...

I'm not really a fan of having logics within a class body definition, I think better make config a lazy property that is when not defined and called will do this trick for you.

Now here:

ai = random.choice(list(graph.keys()))
print(', '.join(self.graph.keys()))

You don't really have to call .keys() method since by default it iterates over dictionary keys. So you can just do:

print(', '.join(self.graph))

This:

def missing_edges(self):
    for keys in itertools.combinations(self.keys(), 2):
        if not self.adjacent(*keys):
            yield keys

Can be replaced by combination of filter and yield from statement. It's also not clear why you define this as a generator, while in the only place you use it, you cast it to list.

When you have already a function I prefer using map instead of list comprehensions so in places like that:

return '\n'.join(str(node) for node in self.values())

I will go for

return '\n'.join(map(str, self.values()))
\$\endgroup\$
  • \$\begingroup\$ Huh, Prospector -s veryhigh rps.py didn't come back with the PEP8 violations you mentioned. I agree I violated PEP257, and my 'violations' of PEP8; line length, \ , were intentional, and follow PEP8 (I'm no hobgoblin). Also holy war time, I use a generator if I can, why should I make something \$O(n)\$ when, with less code, I can make it \$O(1)\$? Also I stay away from filter and map as I agree with this. I appreciate your time, but I'm sticking to my faith. ): \$\endgroup\$ – Peilonrayz Feb 22 '17 at 12:01
  • 1
    \$\begingroup\$ Casting a generator to a list actually has O(n) time complexity as it has to walk on all results until the generator is consumed. \$\endgroup\$ – Fanis Despoudis Feb 22 '17 at 14:08
  • 1
    \$\begingroup\$ @Peilonrayz even thou it's a generator whenever you cast it to list you still spend you O(n) memory and time to do so. The only difference is that you add additional generator overhead for this. While I prefer using generators as much as I can, in this particular case you should either call just next on you generator and see if there is at least 1 item(this is the check you need) or make that method to return a list, since this is how you use it in your code. \$\endgroup\$ – Alex Feb 22 '17 at 14:22
  • \$\begingroup\$ @Alex I know when I cast it to a list it becomes \$O(n)\$, but as I aim to program for long-term use, rather than short term usages, it doesn't make sense to incur memory costs, when a good initial design would solve it. \$\endgroup\$ – Peilonrayz Feb 22 '17 at 15:09
4
\$\begingroup\$

i think a win/lose/draw statistics would be really nice. It is quite easy to add:

class RPS(cmd.Cmd):
    ...
    def __init__(self, *args, **kwargs):
        self.win = self.lose = self.draw = 0
        super().__init__(*args, **kwargs)

    ...    

    def do_play(self, weapon):
        ...
        if ai == weapon:
            self.draw += 1
            print('You draw')
        elif ai in graph[weapon]:
            self.win += 1
            print(f'You win. {weapon.capitalize()} {graph[weapon][ai][1]} {ai}')
        else:
            self.lose += 1
            print(f'You lose. {ai.capitalize()} {graph[ai][weapon][1]} {weapon}')
        print(f'W/L/D: {self.win}/{self.lose}/{self.draw}')

Otherwise this is really well written. For some time I was contemplating if you really need the RPSGraph class, but the printing and the edge adding is different enough to warrant a separate class, so, well done.

\$\endgroup\$
  • \$\begingroup\$ Your point about RPSGraph is very valid, even more so, that I don't print it. Also, what are your views on putting the statistic information in its own class, rather than binding it to the command line? \$\endgroup\$ – Peilonrayz Feb 21 '17 at 15:27
  • \$\begingroup\$ @Peilonrayz I think for that to make sense you would need a separate RPSGame class for which the current RPS is just an interface. There could then be a second interface allowing two players, for example. If you don't want to print the statistics every turn, just add a do_stats(self) function, which prints the string I added above. \$\endgroup\$ – Graipher Feb 21 '17 at 15:31
  • 1
    \$\begingroup\$ They're some fair points, thanks for your time. (I'm also delaying an accept, as it doesn't feel right accepting that my code's good.) \$\endgroup\$ – Peilonrayz Feb 21 '17 at 15:41
  • \$\begingroup\$ @Peilonrayz Yes, it's probably better to wait if someone else finds something to bash on. :) \$\endgroup\$ – Graipher Feb 21 '17 at 15:46

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.