Erster Python Code

[ChrisLerntPython]

Moin zusammen,

ich habe angefangen mich mit Programmierung zu beschäftigen und frisch hier im Forum registriert. Bisher habe ich nur Kleinkram mit Powershell im täglichen Doing gemacht und vor vielen Jahren mal TurboPascal in der Schule gelernt.
Als Einsteigerthema habe ich mir ein bekanntes Würfelspiel mit 5 Würfeln ausgesucht. Die Logik funktioniert und ich kann mit einem Spieler eine Runde durchspielen. Da ich mich als nächstes mit einer passenden GUI beschäftigen möchte, ist die Ausgabe ziemlich rudimentär.

Da ich noch kompletter Neuling bin, würde ich mich sehr über ein paar Hinweise zur Verbesserung freuen. Das ganze ist noch recht sperrig leider. Ein paar Dinge sind aktuell auch nur Test, z.B. die Möglichkeit auch andere Würfel als 5W6 zu benutzen oder mehr als 5 Würfel.

Die nächsten Schritte sind
- Mehr als 1 Spieler (lokal)
- Grafische Oberfläche
- Anbindung Datenbank für die Speicherung von Ergebnissen und co.

Da ich damit bestimmt noch eine ganze Weile beschäftigt bin, würde ich gerne jetzt schon lernen etwas eleganter und strukturierter die Möglichkeiten zu nutzen und mir klassische Anfängerfehler gar nicht erst antrainieren

Vielen Dank für eure Hilfe!

VG
Chris

Code: Alles auswählen

# Define the Dice
class Dice:        
    #just forward the number of die sites and how many you want
    def __init__(self, seiten, anzahl):
        self.seiten = seiten
        self.anzahl = anzahl
    def getSeiten(self):
        return self.seiten
    def getAnzahl(self):
        return self.anzahl
    
# first time init result list -> [0] will keep the die results [1] the number of sites (maybe there will be different dies later) [2] will track the dice that are choosen by the player
def initDie():
    for rollW6 in range(W6.anzahl):
        result[0].append(0)
        result[1].append(W6.seiten)
        result[2].append(False)

# roll the Dice and set temp entries in rollCache and countNumbers from previous rolls to Zero
def rollDice():
    import random
    global countNumbers
    global rollCache
    countNumbers = {1: 0, 2: 0, 3: 0, 4: 0, 5: 0, 6: 0, 7: 0, 8: 0, 9: 0, 10: 0, 11: 0, 12: 0, 13: 0}
    rollCache = {1: 0, 2: 0, 3: 0, 4: 0, 5: 0, 6: 0}
    for rollW6 in range(W6.anzahl):
        if result[2][rollW6] == False:
            result[0][rollW6] = random.randint(1,W6.seiten)
    for rollW10 in range(W10.anzahl):
         if result[2][rollW10] == False:
            result[0][rollW10] = random.randint(1,W10.seiten)

# check possible results after max 3 rolls or player want to pickup the result
def analyzeResults():
    for die in range(0,len(result[0])):
        rollCache[result[0][die]] += 1
    for die in range(1,W6.seiten+1):
        countNumbers[die] = rollCache[die] * die
    if max(rollCache.values()) >= 3 :
        countNumbers[7] = sum(result[0])
    if max(rollCache.values()) >= 4 : 
        countNumbers[8] = sum(result[0])
    if 3 in rollCache.values() and 2 in rollCache.values():
        countNumbers[9] = 25
    streetCheck = sorted(set(result[0]))
    if streetCheck == [1, 2, 3, 4] or streetCheck == [2, 3, 4, 5] or streetCheck == [3, 4, 5, 6] or streetCheck == [1, 3, 4, 5, 6] or streetCheck == [1, 2, 3, 4, 6]:
        countNumbers[10] = 30
    if streetCheck == [1, 2, 3, 4, 5] or streetCheck == [2, 3, 4, 5, 6]:
        countNumbers[11] = 40
        countNumbers[10] = 30
    if max(rollCache.values()) == 5:
        countNumbers[12] = 50
    countNumbers[13] = countNumbers[1]+countNumbers[2]+countNumbers[3]+countNumbers[4]+countNumbers[5]+countNumbers[6]
    # some debugging prints till a GUI is implemented
    print(result[0])
    print(rollCache)   
    print(countNumbers) 

# player will choose which die should not be rolled again the next roll. He can also put a Die back for roll again       
def checkDie():
    rollAgain = False
    while not rollAgain:
        choose = int(input('1-5 or w >>> '))
        if choose == 1:
            if result[2][0] == True:
                result[2][0] = False
            else:
                result[2][0] = True
        if choose == 2:
            if result[2][1] == True:
                result[2][1] = False
            else:
                result[2][1] = True
        if choose == 3:
            if result[2][2] == True:
                result[2][2] = False
            else:
                result[2][2] = True
        if choose == 4:
            if result[2][3] == True:
                result[2][3] = False
            else:
                result[2][3] = True
        if choose == 5:
            if result[2][4] == True:
                result[2][4] = False
            else:
                result[2][4] = True
        if choose == 6:
            rollAgain = True
        print(result[2])

# pick up a result and write it to the final result list
def pickResult():
    picked = False
    nextRoll = False
    x=1
    notok = False
    toblock = 0
    #check for wrong player entries -> its not possible to override an existing result
    while not notok:
        toblock = int(input("Blockeintrag oder mit Eingabe 14 weiterwürfeln >>> "))
        if toblock < 14:
            if block[toblock] == 0:
                notok = True
        if toblock == 14:
            notok = True
    while not picked:
        if toblock != 14:          
            if toblock == x and block[toblock] == 0:
                if countNumbers[x] > 0:
                    block[x] = countNumbers[x]
                else:
                    block[x] = 9999
                picked = True
            x += 1
            nextRoll = True
        else:
            picked = True
    print("Nextroll", nextRoll)
    return(nextRoll)  

# when game is finished count the "upper" part. if the result is higher 65 the player will gain a bonus of 35
def countUpper():
    result = 0
    for x in range(1,7):
        if block[x] < 9999:
            result += block[x]
    if result > 65:
        result += 35
    return(result)

# and count the lower part
def countLower():
    result = 0
    for x in range(7,14):
        if block[x] < 9999:
            result += block[x]
    return(result)

# Initialize the dice and the result list and some variables
W6 = Dice(6,5)
W10 = Dice(10,0)
countNumbers = {1: 0, 2: 0, 3: 0, 4: 0, 5: 0, 6: 0, 7: 0, 8: 0, 9: 0, 10: 0, 11: 0, 12: 0, 13: 0}
rollCache = {1: 0, 2: 0, 3: 0, 4: 0, 5: 0, 6: 0}
result = [[],[],[]]
block = {1: 0, 2: 0, 3: 0, 4: 0, 5: 0, 6: 0, 7: 0, 8: 0, 9: 0, 10: 0, 11: 0, 12: 0, 13: 0}
rollCount = 0         
blockFinished = False

# start the game
initDie()
while not blockFinished:  
    rollCount = 1
    while rollCount <= 3:
        if rollCount == 1:
            for x in range (0, len(result[2])):
                result[2][x] = False
        if rollCount in range (1,4):
            rollDice()
            analyzeResults()
            if rollCount < 3:
                checkDie()
            print(result[2])
            alreadyPicked = pickResult()
            if alreadyPicked:
                print(block)
                rollCount = 4
            else:
                rollCount += 1
        while rollCount > 3 and not alreadyPicked:
            print("Kein Wurf übrig. Bitte Wurf auf den Block übertragen")
            alreadyPicked = pickResult()
    if min(block.values()) > 0:
        print(block)
        oben = countUpper()
        unten = countLower()
        if oben > 65:
            print("Oben mit Bonus +35", oben)
        else:
            print("Oben kein Bonus", oben)
        print("Unten ", unten)
        print("Gesamtpunktzahl ", oben+unten)
        print("Spiel vorbei")
        blockFinished = True

[sparrow]

Hallo Chris,

da du noch ganz am Anfang stehst, wünsche ich schone einmal viel Spaß.

Programmcode sollte eine gewisse Struktur haben und Konventionen folgen, damit er allgemein gut lesbar ist.

Lies dir PEP 8 an. Intensiv.
Namen werden in Python klein_mit_unterstrich geschrieben. Außer die Namen von Klassen (PascalCase) oder Konstanten (KOMPLETT_GROSS).
Die Namen von Funktionen beschreiben Tätigkeiten.

Importe gehören an den Anfang des Moduls. Nicht irgendwo in den Code - und schon gar nicht in irgendwelche Funktionen.
Vergiss, dass es global gibt. Verwende keine globalen Variablen. Nirgends. Das ist auch der Grund, warum ich mir deinen Code nicht näher anschaue. Die musst du los werden. Grundsätzlich gilt:
Auf Modulebene (also nicht eingerückt) stehen nur die Shebang, Importe, die Definition von Konstanten (die ihren Namen daher haben, dass sie sich NIE ändern), die Definition von Klasse, die Definition von Funltionen und der übliche Startpunkt des Programms, der die Funktion aufruft, in der das Programm beginnt. In der Regel heißt die Funktion main und wird so aufgerufen:

Code: Alles auswählen

if __name__ == "__main__":
    main()

Wenn du das konsequent tust, kannst du gar keine globalen Variablen verwenden.

Ebenfalls grundsätzlich gilt: Funktionen bekommen alles, was sie brauchen, als Parameter übergeben und geben das Ergebnis per return zurück. Es gibt also gar keine Notwendigkeit für globale Variablen. Wenn man den Status innerhalb eines Objektes über mehrere Methoden hinweg "behalten" muss, ist das ein Hinweise darauf, dass der Einsatz von Objektorientierter Programmierung sinnvoll wäre.

return ist keine Funktion. Es werden danach in der Zeile also keine Klammern verwendet.

Und ich erlaube mir den Hinweise, dass die Programmierung von grafischen Oberflächen nicht trivial sondern recht komplex ist.

[__blackjack__]

@ChrisLerntPython: Weitere Anmerkungen: Kommentare sollen dem Leser einen Mehrwert über den Code geben. Faustregel: Kommentare beschreiben nicht *was* der Code macht, denn das steht da bereits als Code, sondern warum er das macht. Sofern das nicht offensichtlich ist. Offensichtlich ist in aller Regel auch was in der Dokumentation von Python und den verwendeten Bibliotheken steht.

Schau Dir mal DocStrings an. Einige von den Kommentaren sollten wohl welche sein.

In Sprachen wo man dazu nicht gezwungen ist, definiert man nicht alle Variablen am Anfang, sondern dort wo sie tatsächlich verwendet werden.

Und man sollte da so strukturieren, dass kein Code/keine Daten wiederholt werden. `countNumbers` und `rollCache` werden beispielsweise an zwei Stellen im Code auf die gleiche weise initialisiert.

`rollCache` schein mir auch nur lokal in `analyzeResults()` benutzt zu werden. Das ist aber global und wird in `rollDice()` initialisiert. Warum? Das ist unerwartet/unübersichtlich eine Datenstruktur die nur in einer Funktion verwendet wird, in einer anderen Funktion, die damit gar nichts zu tun hat, zu initialisieren. Dazu noch global, so dass man diesen Zusammenhang gar nicht sehen kann, wenn man nicht das gesamte Programm durchsucht hat.

Statt Datenstrukturen ”leer” zu erzeugen und dann in Funktionen zu ”füllen” erzeugt man besser die Datenstrukturen dort direkt. Es macht keinen Sinn `result` als Liste mit leeren Listen zu erzeugen um dann in einer Funktion da Werte rein zu stecken, statt einfach direkt die Liste mit den Werten dort zu erstellen. Also statt:

Code: Alles auswählen

def init_die(dice, result):
    for _ in range(dice.anzahl):
        result[0].append(0)
        result[1].append(dice.seiten)
        result[2].append(False)

def main():
    ...
    result = [[], [], []]
    init_die(w6_dice, result)

# =>

def init_die(dice, result):
    return [
      [0] * dice.anzahl, [dice.seiten] * dice.anzahl, [False] * dice.anzahl
    ]


def main():
    ...
    result = init_die(w6_dice)

Daten die zusammengehören sollten nicht in parallelen Strukturen stehen wo Elemente am gleichen Index zusammengehören. Das macht alles komlizierter, unübersichtlicher/unverständlicher wenn man mit magischen Indexwerten hantiert, und fehleranfälliger. `result` sollte keine drei magischen Listen enthalten die man irgendwo erklären muss, damit der Leser weiss was die Indizes 0, 1, und 2 bedeuten. Listen sollten in der Regel nur Elemente enthalten die alle die gleiche Bedeutung haben.

Man macht keine Vergleiche mit literalen Wahrheitswerten. Bei dem Vergleich kommt doch nur wieder ein Wahrheitswert bei heraus. Entweder der, den man sowieso schon hatte; dann kann man den auch gleich nehmen. Oder das Gegenteil davon; dafür gibt es ``not``.

Triviale Getter (und Setter) sind „unpythonisch“. Die `getSeiten()`- und `getAnzahl()`-Methoden werden auch nirgends verwendet. Damit wird der Datentyp so einfach, dass man den mit `collections.namedtuple` erstellen könnte, statt mit def ``class``-Anweisung.

Eventuell könnte ein Blick auf `collections.defaultdict` interessant sein.

[ChrisLerntPython]

Moin zusammen,

Danke für das Feedback. Das mit den globalen Variablen hat mir auch nicht gefallen, wusste in dem Moment aber keine bessere Lösung, da ich das mit der Übergabe nicht hinbekommen habe irgendwie. Werde ich mir anschauen.

Als erstes habe ich mir jetzt pylint und pep8 in meinem Visual Studio Code installiert und werd mich mal durch die Trillionen Meldungen ackern Ich hatte ja die Hoffnung, dass die Beschreibungen im Web nicht ganz so verkehrt sind... Naja... "Verwende keine Unterstriche" hatte ich auf der ersten Seite gelesen, die ich mir angeschaut habe

Ich passe mal alles an und poste dann mal eine aktualisierte Version.

VG
Chris

[ChrisLerntPython]

So, da bin ich wieder.

Ich habe den Code jetzt einmal grundlegend überarbeitet und ein paar Testdinge eliminiert, Redundanzen entfernt, die globalen Variablen entfernt und alles in entsprechende Funktionen verpackt.
Außerdem habe ich mit Pylint eine Codekontrolle eingebaut. Bis auf 1-2 Kleinigkeiten meckert VSC da jetzt nicht mehr, scheint also ganz OK zu sein.

Das if __name__ == "__main__": habe ich noch nicht ganz verstanden, liegt aber wahrscheinlich daran, dass ich das Tool bisher nur im Visual Studio Code laufen lasse und nicht von "außen" starte?
Das mit den DocStrings muss ich mir auch noch anschauen. Aber ich hoffe die Namen und der Code sind halbwegs aussagefähig. Wird spannend, wenn ich in einer Woche draufschaue und nicht mehr verstehe, was ich da getan habe...

Sieht für mich jetzt alles wesentlich strukturierter und sauberer aus. Aber ich habe auch nicht den Anspruch in näherer Zukunft Softwareentwickler zu werden. Aber da mein Sohn sich gerne in diese Richtung entwickeln möchte nach der Schule, hab ich ihm gezeigt wie er mit Python erstmal anfangen kann. Da ich ihm auch keinen Mist erzählen möchte, wenn er Hilfe braucht, sollte es schon halbwegs brauchbar sein.

Von daher bitte sehr gerne Feedback, ob es sich in die richtige Richtung entwickelt und was ich noch besser machen kann. Das hilft mir ungemein.

Danke euch!
VG
Chris

Code: Alles auswählen

import random

def choose_a_die_to_keep():
    '''Player Input'''
    wrong_choose = True
    while wrong_choose:
        pick = int(input("Mit 1-5 die Würfel auswählen oder mit 6 weiterwürfeln >>> "))
        if pick <= 6:
            wrong_choose = False
    return pick

def roll_the_dice(die_count, die_site):
    '''roll the dice 3 times and choose the Die to keep'''
    roll = [0]*die_count,[False]*die_count
    rolled = 1
    while rolled < 3:
        for _ in range(0,die_count):
            if roll[1][_] is False:
                roll[0][_] = random.randint(1,die_site)
        print(roll[0])
        not_finished = True
        while not_finished:
            player_input = choose_a_die_to_keep()
            if player_input == 6:
                not_finished = False
            else:
                if roll[1][player_input-1] is True:
                    roll[1][player_input-1] = False
                else:
                    roll[1][player_input-1] = True
            print(roll[1])
        rolled += 1
    return roll[0]

def analyze_results(possible_results, die_site):
    '''check which results are possible with the final roll'''
    roll_cache = {
        1: 0, 2: 0, 3: 0, 4: 0, 5: 0, 6: 0
        }
    count_numbers = {
        1: 0, 2: 0, 3: 0, 4: 0, 5: 0, 6: 0, 7: 0, 8: 0, 9: 0, 10: 0, 11: 0, 12: 0, 13: 0
        }
    for _ in possible_results:
        roll_cache[_] += 1
    for _ in range(1,die_site+1):
        count_numbers[_] = roll_cache[_] * _
    if max(roll_cache.values()) >= 3 :
        count_numbers[7] = sum(possible_results)
    if max(roll_cache.values()) >= 4 :
        count_numbers[8] = sum(possible_results)
    if 3 in roll_cache.values() and 2 in roll_cache.values():
        count_numbers[9] = 25
    street_check = sorted(set(possible_results))
    if street_check in ([1, 2, 3, 4], [2, 3, 4, 5], [3, 4, 5, 6], [1, 3, 4, 5, 6], [1, 2, 3, 4, 6]):
        count_numbers[10] = 30
    if street_check in ([1, 2, 3, 4, 5], [2, 3, 4, 5, 6]):
        count_numbers[11] = 40
        count_numbers[10] = 30
    if max(roll_cache.values()) == 5:
        count_numbers[12] = 50
    count_numbers[13] = count_numbers[1]+count_numbers[2]+count_numbers[3]+count_numbers[4]+count_numbers[5]+count_numbers[6]
    return count_numbers

def write_result_to_the_block(possible_results, block):
    '''When a player finished the roll, put the result to the block'''
    wrong_input = True
    while wrong_input:
        toblock = int(input("Blockeintrag 1-13 >>> "))
        if block[toblock] == 0:
            wrong_input = False
    if possible_results[toblock] > 0:
        block[toblock] = possible_results[toblock]
    else:
        block[toblock] = 9999
    return block

def count_upper(block):
    '''when game is finished count the "upper" part.
    If the result is higher then 65 the player will gain a bonus of 35'''
    upper_result = 0
    for _ in range(1,7):
        if block[_] < 9999:
            upper_result += block[_]
    if upper_result > 65:
        upper_result += 35
    return upper_result

def count_lower(block):
    '''and count the lower part'''
    lower_result = 0
    for _ in range(7,14):
        if block[_] < 9999:
            lower_result += block[_]
    return lower_result

def main(die_count,die_site):
    '''Start the game'''
    block = {1: 0, 2: 0, 3: 0, 4: 0, 5: 0, 6: 0, 7: 0, 8: 0, 9: 0, 10: 0, 11: 0, 12: 0, 13: 0}
    block_finished = False
    while not block_finished:
        rolled_dice = roll_the_dice(die_count, die_site)
        possible_results = analyze_results(rolled_dice, die_site)
        print(possible_results)
        block = write_result_to_the_block(possible_results, block)
        print (block)
        if min(block.values()) > 0:
            block_finished = True
    oben = count_upper(block)
    unten = count_lower(block)
    if oben > 65:
        print("Oben mit Bonus +35", oben)
    else:
        print("Oben kein Bonus", oben)
    print("Unten ", unten)
    print("Gesamtpunktzahl ", oben+unten)
    print("Spiel vorbei")

main(5,6)

[Sirius3]

Doppelte Verneinungen sind immer schwierig zu verstehen (`wrong_choose = False`).
Am besten benutzt man gar kein Flag sondern verläßt die Schleife per `break`:

Code: Alles auswählen

def choose_a_die_to_keep():
    '''Player Input'''
    while True:
        pick = int(input("Mit 1-5 die Würfel auswählen oder mit 6 weiterwürfeln >>> "))
        if pick <= 6:
            break
    return pick

Was hier noch fehlt, ist eine Fehlerbehandlung, falls keine Zahl eingegeben wird.

In `roll_the_dice` ist so einiges falsch. `roll` enthält zwei unabhängige Liste, die besser in zwei Variablen stehen würden, dann hätten die beiden Variablen auch sprechende Namen.
Statt der äußersten while-Schleife wäre eine for-Schleife richtig. Der Doc-String behauptet, dass man dreimal würfeln darf, der Code sagt aber, dass es nur zwei sind!
Und _ sagt dem Leser, dass diese Variable nicht verwendet wird, Du verwendest die aber! `not_finished` ist wieder diese doppelte Verneinung.
Alles außer None vergleicht man nicht mit `is` sondern per `==`, oder bei `True` durch ``.

Code: Alles auswählen

def roll_the_dice(die_count, die_site):
    '''roll the dice 3 times and choose the Die to keep'''
    dice = [0] * die_count
    keep_die = [False] * die_count
    for _ in range(3):
        for i, keep in enumerate(keep_die):
            if not keep:
                dice[i] = random.randint(1, die_site)
        while True:
            player_input = choose_a_die_to_keep()
            if player_input == 6:
                break
            keep_die[player_input - 1] = not keep_die[player_input - 1]
    return dice

Man benutzt keine magischen Zahlenwerte wie 9999. Der Wert für "fehlender Wert" ist None.
Variablen, die man als Argumente übergibt, muß man nicht zurückgeben.

Code: Alles auswählen

def write_result_to_the_block(possible_results, block):
    '''When a player finished the roll, put the result to the block'''
    while True:
        toblock = int(input("Blockeintrag 1-13 >>> "))
        if block[toblock] is None:
            break
    block[toblock] = possible_results[toblock]

Das macht die folgenden Funktionen auch viel einfacher:

Code: Alles auswählen

def count_upper(block):
    '''when game is finished count the "upper" part.
    If the result is higher then 65 the player will gain a bonus of 35'''
    upper_result = sum(block[index] for range(1, 7))
    if upper_result > 65:
        upper_result += 35
    return upper_result

def count_lower(block):
    '''and count the lower part'''
    return sum(block[index] for index in range(7, 14))

In `main` ist `block_finished` überflüssig, da man dieses Flag einfach berechnen kann:

Code: Alles auswählen

def main(die_count, die_site):
    '''Start the game'''
    block = dict.fromkeys(range(1,14))
    while any(value is None for value in block.values()):
        rolled_dice = roll_the_dice(die_count, die_site)
        possible_results = analyze_results(rolled_dice, die_site)
        print(possible_results)
        write_result_to_the_block(possible_results, block)
        print(block)
    oben = count_upper(block)
    unten = count_lower(block)
    if oben > 65:
        print("Oben mit Bonus +35", oben)
    else:
        print("Oben kein Bonus", oben)
    print("Unten ", unten)
    print("Gesamtpunktzahl ", oben + unten)
    print("Spiel vorbei")

Sowohl `block` als auch die Prüfungen in `analyze_results` setzen vorraus, dass die Würfel 6 Seiten haben, und dass es 5 Würfel gibt. Die Variablen die_count und die_site suggerieren, dass man darauf Einfluß hat. Da das nicht der Fall ist, können sie weg.

Hier noch eine angepasste Variante von `analyze_results`:

Code: Alles auswählen

def analyze_results(dice):
    '''check which results are possible with the final roll'''
    roll_cache = Counter(dice)
    max_count = max(roll_cache.values())
    possible_results = {
        value: roll_cache.get(value, 0)
        for value in range(1, 7)
    }
    dice_set = set(dice)
    possible_results.update({
        7: sum(dice) if max_count >= 3 else 0,
        8: sum(dice) if max_count >= 4 else 0,
        9: 25 if set(roll_cache.values()) == {2, 3} else 0,
        10: 30 if any(dice_set.issuperset(street)
            for street in [{1, 2, 3, 4}, {2, 3, 4, 5}, {3, 4, 5, 6}]) else 0,
        11: 40 if dice_set in [{1, 2, 3, 4, 5}, {2, 3, 4, 5, 6}] else 0,
        12: 50 if max_count == 5 else 0,
        13: sum(dice),
    })
    return possible_results