Schere-Stein-Papier-... (Verbesserungen erwünscht)

Stellt hier eure Projekte vor.
Internetseiten, Skripte, und alles andere bzgl. Python.
Antworten
georgebaker
User
Beiträge: 25
Registriert: Freitag 12. April 2013, 19:53

Hallo liebe Python-Community,

Ich habe mich bei coursera.org im Kurs "An Introduction to Interactive Programming in Python" eingeschrieben und gestern das erste Mini-Projekt (Schere-Stein-Papier-Schere-Echse-Spock) abgegeben. Heute habe ich mein Programm nochmal überarbeitet und auf Python 3 umgeschrieben. Ich würde mich über Anmerkungen zum Code und Verbesserungsvorschläge freuen (besonders ob das Ganze PEP-8 konform ist).
Das Spiel ist Abwandlung von Schere-Stein-Papier, textbasiert und wird gegen den Computer gespielt.

Der Kurs basiert auf Python 2, aber ich probiere alles auch in Python 3 aus, um die Unterschiede besser zu verstehen und beide anwenden zu können.

Code: Alles auswählen

# Rock-Paper-Scissors-Lizard-Spock
# A simple text-base game against the computer

# Change order of names because it's easier to evaluate the winner

"""
0 - rock
1 - Spock
2 - paper
3 - lizard
4 - scissors
"""

# import modules
import random
import time

# helper functions
def number_to_name(number):
    """ Converts the computer's choice into names """
    if number == 0:
        name = "Rock"
    elif number == 1:
        name = "Spock"
    elif number == 2:
        name = "Paper"
    elif number == 3:
        name = "Lizard"
    elif number == 4:
        name = "Scissors"
    else:
        print ("Error: It has to be a number between 0 and 4")
    return name

def name_to_number(name):
    """ Converts the player's choice into numbers """
    if name == "Rock":
        number = 0
    elif name == "Spock":
        number = 1
    elif name == "Paper":
        number = 2
    elif name == "Lizard":
        number = 3
    elif name == "Scissors":
        number = 4
    else:
        print ("Error: Eather choose Rock, Spock, Paper, Lizard or Scissors.")
    return number

def rpsls(name):
    """ Function to evaluate the winner """
    # Convert name to player_number using name_to_number
    player_number = name_to_number(name)

    # Compute random guess for comp_number using random.randrange()
    comp_number = random.randrange(0, 5)

    # Compute difference of player_number and comp_number module five
    difference = (player_number - comp_number) % 5

    # Use if/elif/else to determine winner
    if difference == 0:
        winner = "Player and computer tie!"
    elif difference == 1:
        winner = "Player wins!"
    elif difference == 2:
        winner = "Player wins!"
    elif difference == 3:
        winner = "Computer wins!"
    elif difference == 4:
        winner = "Computer wins!"
    else:
        print ("Unknown Error detected")

    # Convert comp_number to name using number_to_name
    comp_name = number_to_name(comp_number)

    # print results
    print ("Player chooses", name)
    time.sleep(0.5)
    print ("Computer chooses", comp_name)
    print (winner, "\n")

# Print out decisions for the player
print ("Welcome to Rock-Paper-Scissors-Lizard-Spock")
print ("Please choose one of the following names:\n")
print ("0 - Rock")
print ("1 - Spock")
print ("2 - Paper")
print ("3 - Lizard")
print ("4 - Scissors\n")

while True:
    decision = int(input("Enter your decision: "))
    
    # End the game if a wrong number is entered
    if (decision > 4 ) or (decision < 0):
        print ("Error: It has to be a number between 0 and 4")
        break
    
    # Convert the entered number to name using number_to_name
    name = number_to_name(decision)
    rpsls(name)
BlackJack

@georgebaker: Kommentare die keinen Mehrwert liefern sollte man weglassen. Du hast da mehrfach welche die das aussagen was direkt in der Zeile danach noch einmal ganz offensichtlich als Quelltext steht. Dazu braucht man keinen Kommentar. Kommentare braucht man wenn man erklären will *warum* etwas so gemacht wird, wie es im Quelltext realisiert wird, und nicht *was* dort gemacht wird. Sollte *das* nicht ersichtlich sein, sollte man erst versuchen den Quelltext verständlicher zu formulieren, bevor man zu Kommentaren zurück greift.

Der Code auf Modulebene sollte in einer Funktion verschwinden. Das verringert die Gefahr, dass eine Funktion auf etwas zugreift, was sie nicht als Argument übergeben bekommen hat. Oder das man sich wenn man den Quelltext liest, fragen muss ob ein Name der in Funktionen und auf Modulebene verwendet wird, in der Funktion nun lokal ist oder nicht. Sowie man innerhalb der Funktion eine Zuweisung sieht ist das klar, aber die muss man unter Umständen halt erst einmal suchen.

Die beiden Umwandlungsfunktionen sind unnötig kompliziert. Insbesondere verstehe ich nicht warum die Benutzereingabe erst von einer Zahl in den Namen und dann sofort wieder der Name in eine Zahl umgewandelt wird. Das ist doch überflüssig. Wenn der Benutzer Zahlen eingibt, und der Computergegner Zahlen „würfelt”, dann kann man intern doch grundsätzlich mit Zahlen arbeiten und braucht nur eine Umwandlung von einer Zahl in eine Zeichenkette zur Anzeige. Und das lässt sich kompakt und einfach mit einer Liste mit den Namen lösen auf die mit der Zahl zugegriffen wird. Da braucht man keine lange Funktion für. Diese Liste kann man auch zur Ausgabe der Auswahl für den Benutzer verwenden, so dass man am Ende die Namen im Quelltext nur *einmal* an einer Stelle stehen hat. Dann lässt sich das ganz einfach an dieser einen Stelle anpassen.

Die Fehlerbehandlung in den beiden Umwandlungsfunktionen ist fehlerhaft. In beiden Fällen würde bei einer falschen Zahl beziehungsweise einem falschen Namen gleich in der Zeile nach dem `print()` ein `NameError` ausgelöst, denn der Name für den Rückgabewert wurde dann ja gar nicht an einen Wert gebunden. ”Fehlerbehandlung” die aus einem `print()` und dann weiter so als wäre nichts geschehen, ist sehr selten eine gute Idee. Dem Spieler zu sagen, dass seine Eingabe falsch war, ist ausserdem eher Aufgabe des Codes der für die Benutzerinteraktion zuständig ist.

Die DocStrings zu den beiden Umwandlungsfunktionen sind unnötig einschränkend. Wie die Zahl oder der Name zustande gekommen sind, ist beiden Funktionen doch eigentlich völlig egal.

Namen sollten dem Leser die Bedeutung des Wertes im Kontext des Programms vermitteln. `rpsls()` hinterlässt eigentlich nur Fragezeichen. Und wenn Du da in einem Jahr noch einmal drauf schaust, dann weisst Du vielleicht selbst nicht mehr was Du Dir *dabei* mal gedacht hast. Nicht ganz so dramatisch ist `comp_player`, aber auch da sollte man um Mehrdeutigkeiten zu vermeiden ('compare'? 'comparable'? 'compulsive'?) die Abkürzung ausschreiben.

Bei einer Funktion die den Gewinner ermittelt, würde ich eine schreiben die *zwei* Eingaben entgegen nimmt, nämlich die Wahl von jedem Spieler. Denn so kann man die Funktion nur für einen Computergegner verwenden und müsste eine neue Funktion schreiben wenn man die Wahl von zwei menschlichen Spielern auswerten möchte. Obwohl da doch letztendlich das gleiche passieren müsste. Hier ist auch wieder Spiellogik und Benutzerinteraktion vermischt. Wenn man beispielsweise eine GUI für das Spiel schreiben wollte, könnte man die Funktion so auch nicht verwenden.

Jeweils zwei Zweige enthalten den gleichen Code — solche Codewiederholungen sollte man vermeiden. Die Bedingungen lassen sich doch zusammenfassen.

`print()` ist in Python 3 eine Funktion wie jede andere. Man sollte den Aufruf deshalb auch nicht mit einem extra, unnötigem Leerzeichen zwischen Funktionsname und öffnender Klammer schreiben.

Wenn man zwei Vergleiche mit dem selben Wert anstellt und die Verknüpft, kann man das oft als verkettete Bedingung schreiben, wie man das aus der Mathematik gewohnt ist. Also statt ``(decision > 4 ) or (decision < 0)`` kann man ``not (0 <= decision <= 4)`` schreiben.

Die Behauptung am Anfang, dass das umsortieren der Namen den Code einfacher macht, bezweifle ich, oder zumindest hätte man es noch einfacher machen können wenn man sie so anordnet, dass der Gewinner daran ermittelt werden kann, ob die Differenz gerade oder ungerade ist.

Ich komme dann ungefähr bei dem hier raus (nahezu ungetestet):

Code: Alles auswählen

#!/usr/bin/env python3
"""Rock-Paper-Scissors-Lizard-Spock

A simple text-base game against the computer.

Change order of names because it's easier to evaluate the winner

0 - rock
1 - Spock
2 - paper
3 - lizard
4 - scissors
"""
import random
import time


NAMES = ['Rock', 'Spock', 'Paper', 'Lizard', 'Scissors']


def determine_winner(choice_player_a, choice_player_b):
    """Function to evaluate the winner."""
    difference = (choice_player_a - choice_player_b) % 5
    assert len(NAMES) == 5
    if difference == 0:
        return None
    elif difference in [1, 2]:
        return 0
    elif difference in [3, 4]:
        return 1
    else:
        assert False


def main():
    player_names = ['Player', 'Computer']
    print('Welcome to {0}'.format('-'.join(NAMES)))
    print('Please choose one of the following names:\n')
    for i, name in enumerate(NAMES):
        print(i, '-', name)
    print()

    while True:
        player_choice = int(input("Enter your decision: "))
        if not (0 <= player_choice < len(NAMES)):
            print(
                'Error: It has to be a number between 0 and {0}'.format(
                    len(NAMES) - 1
                )
            )
            break
        computer_choice = random.randrange(len(NAMES))
        for name, choice in zip(player_names, [player_choice, computer_choice]):
            print(name, 'chooses', NAMES[choice])
            time.sleep(0.5)
        winner = determine_winner(player_choice, computer_choice)
        if winner is None:
            print('{0} tie!'.format(' and '.join(player_names)))
        else:
            print(player_names[winner], 'wins!')


if __name__ == '__main__':
    main()
EyDu
User
Beiträge: 4881
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Hallo.

Da kann man viel vereinfachen. Zunächst bietet sich das Zusammenfassen der Symbole an:

Code: Alles auswählen

SHAPES = "Rock", "Spock", "Paper", "Lizard", "Scissors"
Damit vereinfachen sich dann auch die beiden Umwandlungsfunktionen:

Code: Alles auswählen

def number_to_names(n):
    return SHAPES[n]

def names_to_number(n):
    return SHAPES.index(n)

#oder noch stärker vereinfacht:
number_to_names = SHAPES.__getitem__
names_to_number = SAPES.index
Die else-Fälle in deinen Umwandlungsfunktionen solltest du auslassen, bzw. eine sinnvolle Expection werfen. Eine Fehlerausgabe in einer berechnenden Funktion ist immer ungünstig, da dort Aufgaben erledigt werden, die dort nicht hingehören. Außerdem führt das ganze zu unschönen Ausgaben, wenn man die Eingaben doch vorher prüft. Abgesehen davon, schmiert dein Programm anschließend beim ``return name`` so oder so ab.

Die Auswertung des Gewinners kannst du auch zusammenfassen:

Code: Alles auswählen

if 1 <= difference <= 2:
    winner = "Player wins!"
elif 3 <= difference <= 4:
    winner = "Computer wins!"
else:
    winner = "Tie!"
Auch hier solltest du davon absehen den "Unknown Error" zu "behandeln". An der Stelle kann gar kein Fehler auftreten und mit einer Ausgabe ist niemandem geholfen. Auch bietet es sich an, die Ermittlung des Gewinners in eine eigenständige Funktion zu packen. Der Name "winner" ist auch etwas irreführend, da darin nicht der Gewinner abgelegt wird, sonder eine Ausgabe. Hier solltest du noch mit Konstanten nachbessern.

Allgemein gilt: Berechnung und Ausgabe sollten immer getrennt von einander sein. Vielleicht als kleine Aufgabe für dich: erweitere dein Spiel um eine GUI mit Tkinter. Das ganze solltest du so realisieren, dass der Spieler beim Start des Programms auswählen kann, ob er über Text oder GUI spielen möchte.

Du hast sehr viel Code auf Modulebene. Dies solltest du vermeiden, da man dein Modul sonst nicht importieren kann, ohne dass dein Spiel gestartet wird. Das erschwert die Wiederverwendbarkeit von Code. In python gibt es dafür ein ganz einfaches Idiom:

Code: Alles auswählen

def main():
    ....

if __name__ == "__main__":
    main()
In die main-Funktion packst du nun alles, was bei dir auf modulebene geschieht.

Die Ausgabe der möglichen Symbole kannst du nun auch vereinfachen:

Code: Alles auswählen

for index, shape in enumerate(SHAPES):
    print("{index} - {shape}".format(index=index, shape=shape)
Immer wenn du anfängst zu nummerieren, dann solltest du dir Gedanken über eine Schleife machen.

Code: Alles auswählen

if (decision > 4 ) or (decision < 0):
Hier kannst du die Klammern weglassen. Außerdem bietet es sich an den Test umzudrehen:

Code: Alles auswählen

if not 0 <= decision < len(SHAPES):
Allerdings werden solche Probleme in Python anders gelöst: es wird einfach versucht die Umwandlung durchzuführen und fängt ggf. die Exception ab. Sie die Anmerkungen weiter oben zu den Exeptions in name_to_number und number_to_name.

Allerdings hast du weiter vorher bereits ein Problem: ``int(input(...))`` versucht jede Eingabe in einen Integer umzuwandeln. Gib einfach mal keinen Integer ein, sondern irgendwelche Buchstaben. Hier musst du noch nachbessern.

Der Aufruf "number_to_name" ist überflüssig. Gleich am Anfang von rpsls wandelst du den Namen wieder in den Index um, das kannst du dir also sparen.

Das sind zumindest die groben Probleme, ein paar Feinheiten gibt es auch noch.

Edit: Argh, da war ich heute wohl zu langsam.
Das Leben ist wie ein Tennisball.
georgebaker
User
Beiträge: 25
Registriert: Freitag 12. April 2013, 19:53

Wow, vielen Dank für die schnellen und ausführlichen Antworten. Ich werde mir die Vorschläge und Änderungen ansehen um daraus zu lernen.
Die Kommentare waren eher als Leitfaden für mich gedacht, aber ich werde sie in Zukunft auf ein Minimum beschränken.

Nochmals vielen Dank euch beiden.
chrisi
User
Beiträge: 1
Registriert: Mittwoch 17. November 2021, 07:53

Hallo,

das ganze geht viel einfacher und mit weniger zeilen code:

import random

print("Spiele Schere-Stein_Papier mit mir!")
print("Bitte wähle: Schere (s), Stein (r) oder Papier (p)")

spielzuege = ["s", "r", "p"]

while True:
eingabe = input("Deine Wahl: ")
if eingabe in spielzuege:
random.shuffle(spielzuege)
computer = spielzuege[0]
print("Computer: " + computer)
if computer == eingabe:
print("Unentschieden")
elif computer == "s" and eingabe == "p":
print("Du hast verloren")
elif computer == "s" and eingabe == "r":
print("Du hast gewonnen")
elif computer == "r" and eingabe == "p":
print("Du hast gewonnen")
elif computer == "r" and eingabe == "s":
print("Du hast verloren")
elif computer == "p" and eingabe == "s":
print("Du hast gewonnen")
elif computer == "p" and eingabe == "r":
print("Du hast verloren")
else:
print("Falsche Eingabe")

LG
Christoph :wink:
Sirius3
User
Beiträge: 17712
Registriert: Sonntag 21. Oktober 2012, 17:20

Wenn Du eine beliebiges Element aus einer Liste möchtest, dann brauchst Du nicht erst alle Elemente zu mischen, sondern kannst per random.choice auch einfach nur eins auswählen.
Statt je dreimal zwei identische Texte auszugeben, würde man das ganze kürzer in ein if packen.
.. oder durch eine einfache Rechnung ersetzen.

Code: Alles auswählen

import random

SPIELZUEGE = ["s", "r", "p"]
TEXTE = [
    "Unentschieden",
    "Du hast verloren",
    "Du hast gewonnen",
]

def main():
    print("Spiele Schere-Stein_Papier mit mir!")
    print("Bitte wähle: Schere (s), Stein (r) oder Papier (p)")

    while True:
        try:
            eingabe = input("Deine Wahl: ")
            computer = random.choice(SPIELZUEGE)
            ergebnis = (SPIELZUEGE.index(computer) - SPIELZUEGE.index(eingabe)) % len(SPIELZUEGE))
        except ValueError:
            print("Falsche Eingabe")
        else:
            print("Computer:", computer)
            print(TEXTE[ergebnis])

if __name__ == "__main__":
    main()
Es geht aber nicht um kürze, sondern um lesbaren Code, der auch sinnvolle Fehlermeldungen liefert.
Benutzeravatar
__blackjack__
User
Beiträge: 13006
Registriert: Samstag 2. Juni 2018, 10:21
Wohnort: 127.0.0.1
Kontaktdaten:

Vor allem ist das doch gar nicht äquivalent zum Original — wo sind Spock und die Echse abgeblieben? Sachen vereinfachen Okay, aber doch bitte nicht so, in dem man einfach wichtige Bestandteile weg lässt. Denn sonst würde ich ja einfach sagen ihr macht das alles zu kompliziert, ich habe das mal auf das hier vereinfacht: 😜
“Most people find the concept of programming obvious, but the doing impossible.” — Alan J. Perlis
imonbln
User
Beiträge: 149
Registriert: Freitag 3. Dezember 2021, 17:07

Mein Entwurf für das Spiel sieht ungefähr so aus. Ziel war es, das ganze möglichst Lesebar zu machen, um die Mechanik gut zu verstehen. Auch habe ich versucht, ohne Magicnumbers auszukommen und den Dingen lieber Namen geben. Ich möchte mir nicht merken welche Differenz was bedeutet, dass soll mal schön das Programm für mich machen.

Code: Alles auswählen

from __future__ import annotations
import random
import sys 
import typing as t
from enum import Enum, auto


class GameEnum(Enum):

    def __str__(self) -> str:
        return self.name.capitalize()


class GameState(GameEnum):
    TIE = auto()
    WIN = auto()
    LOSE = auto()


class GameMove(GameEnum):
    ROCK = auto()
    PAPER = auto()
    SICSSOR = auto()
    LIZARD = auto()
    SPOCK = auto()

    def compare(self, other: GameMove) -> GameState:
        win_matrix = { 
            (self.ROCK, other.SICSSOR):      GameState.WIN,
            (self.ROCK, other.LIZARD):       GameState.WIN,

            (self.PAPER, other.ROCK):        GameState.WIN,
            (self.PAPER, other.SPOCK):       GameState.WIN,

            (self.SICSSOR, other.PAPER):     GameState.WIN,
            (self.SICSSOR, other.LIZARD):    GameState.WIN,

            (self.LIZARD, other.PAPER):      GameState.WIN,
            (self.LIZARD, other.SPOCK):      GameState.WIN,

            (self.SPOCK, other.ROCK):        GameState.WIN,
            (self.SPOCK, other.SICSSOR):     GameState.WIN,

        }
        if self == other:
            # Tie if both have the same
            return GameState.TIE
        # if the result is in the dict, the instance has won,
        # otherwise it has lost.
        return win_matrix.get((self, other), GameState.LOSE)

def main():
    moves = list(GameMove)
    computer = random.choice(moves)
    while True:
        try:
            print('Choose wise:')
            for idx, move in enumerate(moves):
                print(f'\t{move:10s} -> {idx}')
            player = int(input('Your Choice: '))
            player = moves[player]

            break
        except ValueError:
            print('Expect a Intger Value, try again', file=sys.stderr)
        except IndexError:
            print('Index out of range, try again', file=sys.stderr)

    print(f'You chose:{player}, computer chose: {computer}')

    game_result = player.compare(computer)
    if game_result == GameState.TIE:
        print('We have a Tie.')
    else:
        print(f'You {game_result}.')


if __name__ == '__main__':
    main()

Benutzeravatar
__blackjack__
User
Beiträge: 13006
Registriert: Samstag 2. Juni 2018, 10:21
Wohnort: 127.0.0.1
Kontaktdaten:

@imonbln: Ein Wörterbuch bei dem alle Werte gleich sind, erscheint mir sehr sinnfrei. Das wäre dann eher ein `set` der Schlüssel. Und man braucht das auch nicht immer neu aufbauen. Insbesondere nicht bevor man auf Unentschieden prüft.

Code: Alles auswählen

#!/usr/bin/env python3
import random
import sys
from enum import Enum, auto


class GameEnum(Enum):
    def __str__(self):
        return self.name.capitalize()


class GameState(GameEnum):
    TIE = auto()
    WIN = auto()
    LOSE = auto()


class GameMove(GameEnum):
    ROCK = auto()
    PAPER = auto()
    SICSSOR = auto()
    LIZARD = auto()
    SPOCK = auto()

    def compare(self, other):
        if self == other:
            return GameState.TIE

        return (
            GameState.WIN
            if (self, other) in WINNING_COMBINATIONS
            else GameState.LOSE
        )


WINNING_COMBINATIONS = {
    (GameMove.ROCK, GameMove.SICSSOR),
    (GameMove.ROCK, GameMove.LIZARD),
    (GameMove.PAPER, GameMove.ROCK),
    (GameMove.PAPER, GameMove.SPOCK),
    (GameMove.SICSSOR, GameMove.PAPER),
    (GameMove.SICSSOR, GameMove.LIZARD),
    (GameMove.LIZARD, GameMove.PAPER),
    (GameMove.LIZARD, GameMove.SPOCK),
    (GameMove.SPOCK, GameMove.ROCK),
    (GameMove.SPOCK, GameMove.SICSSOR),
}


def main():
    moves = list(GameMove)
    computer = random.choice(moves)
    while True:
        try:
            print("Choose wisely:")
            for index, move in enumerate(moves):
                print(f"\t{move:10s} -> {index}")
            player = moves[int(input("Your Choice: "))]
            break
        except ValueError:
            print("Expect a Intger Value, try again.")
        except IndexError:
            print("Index out of range, try again.")

    print(f"You chose: {player}, computer chose: {computer}.")

    game_result = player.compare(computer)
    if game_result == GameState.TIE:
        print("We have a Tie.")
    else:
        print(f"You {game_result}.")


if __name__ == "__main__":
    main()
“Most people find the concept of programming obvious, but the doing impossible.” — Alan J. Perlis
Antworten