7
\$\begingroup\$

I am new to Python but I aspire to become a 'proper' Python coder, (maybe even one day help the noobs on CodeReview :)

Therefore I want to get into good habits early, and would really appreciate some feedback on my first mini-project.

Notes

  1. The code deals cards for a simple board game. If you are interested, this is explained in more detail below under 'Rules'. Full game rules here.
  2. There are no bugs or problems I can detect.
  3. I have made extensive use of tkinter as a means-to-an-end, however I am less interested in tkinter-specific feedback, but rather the fundamentals of Python itself: classes/objects, decorators, scope, code design/efficiency/elegance. Please point out any barn-door design mistakes, stylistic shortfalls, areas for improvement.
  4. I know i need to change some function names in line with PEP.

Rules (What the Code Does)

  • The user selects their team type and team colour from 'launch window'.
  • A 'game window' then appears, showing two decks of shuffled cards, 'R' and 'S', 15 per deck.
  • User can click either deck to draw a hand of 4 cards.
  • User can click on any of the 4 cards to 'play' the card, whereafter the card is discarded from the game, and the 3 remaining cards are set aside in a hidden 'recycle' deck (one for R, one for S).
  • When fewer than 4 cards remain in either of the main 'R' or 'S' decks, the R or S hidden recycled deck is shuffled and its cards are re-added to the bottom of the respective 'R' or 'S' main deck, and can be drawn on demand, as before.
  • Sometimes, a user is compelled to collect an 'exhaustion' card for R or for S, which is placed directly into the respective hidden recycle deck.
  • If the user can not draw 4 cards even after replenishing the R or S deck with its respective hidden recycle deck, the shortfall is made up by taking further 'exhaustion' cards as required.
  • There is no mixing between R and S decks.
  • Local image files of the cards are used as the 'buttons', so they can click on the card they want to choose it.

That's it! Simple stuff. The game itself is not coded here. The reason for the lengthy code is mainly due to the graphical elements.


The Code

Am I using @property appropriately/excessively/unneccessarily below?

################
# GameLogic.py #
################
import random

# Global constants
# (The lists represent the decks, the elements are the numbers shown on each card.
ROULEUR = [3, 3, 3, 4, 4, 4, 5, 5, 5, 6, 6, 6, 7, 7, 7]
SPRINTER =[2, 2, 2, 3, 3, 3, 4, 4, 4, 5, 5, 5, 9, 9, 9]


class Deck:
    def __init__(self, team: str, rider: str, teamtype: str):
        self._team = team
        self._rider = rider
        self._cards = []
        self._teamtype = teamtype

    @property
    def show(self):
        i = 0
        total = self.count_cards
        for crd in self.cards:
            i += 1
            print(f'Value of card {i} of {total} in {self.team} {self.rider} deck is {crd.value}')
            if not isinstance(self,MainDeck):
                print('This is not the main deck')
    # Getters
    @property
    def team(self):
        return self._team

    @property
    def rider(self):
        return self._rider

    @property
    def cards(self):
        return self._cards  # Returns list of Card objects

    @property
    def teamtype(self):
        return self._teamtype

    # Functions
    def get_card(self, index: int):
        return self.cards[index]

    @property
    def count_cards(self):
        return len(self.cards)

    def pick_cards(self, num: int):
        return self.cards[0:num]

    @property
    def shuffle(self):
        for i in range(self.count_cards -1, -1, -1):
            rnd = random.randint(0, i)
            self.cards[i], self.cards[rnd] = self.cards[rnd], self.cards[i]

    def add(self, newCard: object):
        self.cards.append(newCard)

    def remove_card(self, index: int):
        return self.cards.pop(index)

    @property
    def countExhaustion(self) -> int:
        cnt = 0
        for crd in self.cards:
            if isinstance(crd, ExhaustCard):
                cnt += 1
        return cnt

    def getFilePaths(self) -> str:
        filepaths = []
        for card in self.cards:
            if isinstance(card,ExhaustCard):
                filepaths.append(f'Images/exhaust_{self.rider[0:1].upper()}_{str(card.value)}.jpg')
            else:
                filepaths.append(f'Images/{self.team.lower()}_{self.rider[0:1].upper()}_{str(card.value)}.jpg')
        return filepaths

    # For a given card[index = i], return a tuple with all the remaining original indices of the other cards in that deck. Used for recycling hands of variable size. Reverse order for ease of processing later.
    def getRemainders(self, i: int) -> int:
        indices = list(range(self.count_cards))
        del indices[i]
        return indices[::-1]

class MainDeck(Deck):
    def __init__(self, team: str, rider: str, teamtype: str):
        self._team = team
        self._rider = rider
        self._cards = []
        self._teamtype = teamtype
        self._recycleDeck = Deck(team,rider,teamtype)

        if self.teamtype.lower() == 'player':
            if rider[0:1].upper() == 'R':
                for i in range(len(ROULEUR)):
                    self._cards.append(Card(ROULEUR[i]))
            elif rider[0:1].upper() == 'S':
                for i in range(len(SPRINTER)):
                    self._cards.append(Card(SPRINTER[i]))

    @property
    def recycleDeck(self):
        return self._recycleDeck

    def recycle(self, index: int):
        self.recycleDeck.cards.append(self.remove_card(index))

    def refresh_deck(self):
        self.recycleDeck.shuffle  # Shuffle the recycled cards (not those remaining in main deck)
        for i in range(self.recycleDeck.count_cards -1, -1,-1): # Add the shuffled recycled cards back in to the back of the main deck
            self.cards.append(self.recycleDeck.remove_card(i))


class Card():
    def __init__(self, value):
        self._value = value

    @property
    def value(self):
        return self._value

class ExhaustCard(Card):
    def __init__(self):
        self._value = 2


# TO DO:
# - error handling on init verificaction

By contrast the file below feels bloated and messy. There must be a cleaner way to perform the interactive image initialisation instead of laboriously coding each item individually?

############
# GUI Module
############
import tkinter as tk
from PIL import Image, ImageTk
import GameLogic as GL
from tkinter import messagebox


# Constants
CARD_W = 180
CARD_H = 300
W_MULT = 1.2
TEAMS = ('Blue', 'Red', 'Green', 'Black', 'White', 'Pink')
RIDERS = ('Rouleur', 'Sprinter')
PLAYER = ('Player', 'Bot: Muscle Team', 'Bot: Peloton Team')
HANDSIZE = 4
DECK_PATHS = ('deck_main_R', 'deck_exhaust_R', 'FR_logo', 'deck_main_S', 'deck_exhaust_S')


##################
# LaunchWindow Class
##################
class LaunchWindow:

    def __init__(self, master):
        self.master = master

        # Make the frames
        self.frm_header = tk.Frame(self.master)
        self.frm_options = tk.Frame(self.master)
        self.frm_colour = tk.Frame(master=self.frm_options)
        self.frm_player = tk.Frame(master=self.frm_options)
        self.frm_footer = tk.Frame(master=self.master)

        # (1) Header widgets
        tk.Label(master=self.frm_header, text='Flamme Rouge', anchor=tk.NW, font=('Verdana', 50), fg='red').pack(side=tk.TOP, pady=10, padx=10)
        tk.Label(master=self.frm_header, text='Select team type options', anchor=tk.NW, font=('Verdana', 25)).pack(side=tk.TOP,pady=5)

        # (2) Team Choice Widgets
        #   (A) Team Colour Choice Input
        colour_choice = tk.StringVar()
        tk.Label(master= self.frm_colour, text='Team colour:').pack(side=tk.TOP, pady = 3)
        for team in TEAMS:
            textcolour = 'White'
            if team == 'White' or team == 'Pink':
                textcolour = 'Black'
            rb_teams = tk.Radiobutton(master= self.frm_colour, text=team, value=team, variable=colour_choice,  bg=team, fg=textcolour)
            rb_teams.pack(anchor=tk.N)

        #   (B) Player (Human or Bot) Choice Input
        player_choice = tk.StringVar()
        tk.Label(master=self.frm_player, text='Team Type:').pack(side=tk.TOP, pady=3)
        for p in PLAYER:
            rb_teams = tk.Radiobutton(master=self.frm_player, text=p, value=p, variable=player_choice, indicatoron=0)
            rb_teams.pack(anchor=tk.N)

        # (3)  Footer Widgets
        self.btn_start = tk.Button(master=self.frm_footer, text='Start Game', anchor=tk.S,
                                   relief=tk.RAISED, font=('Verdana', 35),
                                   command=lambda: self.start_game(colour_choice.get(), player_choice.get()),
                                   padx=20, pady=10).pack(side=tk.LEFT)
        self.btn_close = tk.Button(master=self.frm_footer, text='Close', anchor=tk.S,  padx=20, pady=10,
                                   relief=tk.RAISED, font=('Verdana', 25), command=self.handle_close).pack(side=tk.LEFT)

        #Pack the frames
        self.frm_header.pack(side=tk.TOP)
        self.frm_colour.pack(side=tk.LEFT)
        self.frm_player.pack(side=tk.LEFT)
        self.frm_options.pack(side=tk.TOP)
        self.frm_footer.pack(side=tk.BOTTOM, pady = 10)

    # Handle events, clicks etc
    def handle_close(self):
        self.master.destroy()

    def start_game(self, colour: str, team_type: str):
        self.master.destroy()
        game_window = tk.Tk()
        game = GameWindow(game_window, colour, team_type)



##################
# GameWindow Class
##################
class GameWindow:
    def __init__(self, master, colour, teamtype): # Create labels, entries,buttons
        self.master = master

        # Make the frames
        self.frm_header = tk.Frame(self.master)
        self.frm_decks = tk.Frame(self.master)
        self.frm_cards = tk.Frame(self.master)
        self.frm_footer = tk.Frame(self.master)

        # Initialise the Card decks according to user input on launch window
        self._deck_R = GL.MainDeck(colour, 'Rouleur', teamtype)
        self._deck_S = GL.MainDeck(colour, 'Sprinter', teamtype)
        self.deck_R.shuffle
        self.deck_S.shuffle

        # (1) Header Widgets
        tk.Label(master=self.frm_header, text=colour+' Team', fg=colour, bg='grey', anchor=tk.NW, font=('Verdana', 35, 'bold')).pack(side=tk.TOP, pady=10)

        # (2) Deck Widgets
        deck_images = []
        deck_btns = []
            # (a) Rouleur deck
        deck_images.append(ImageTk.PhotoImage(Image.open(f'Images/{DECK_PATHS[0]}.jpg').resize((int(CARD_W*W_MULT), CARD_H))))
        deck_btns.append(tk.Button(master=self.frm_decks, width=int(CARD_W*W_MULT), height=CARD_H, image=deck_images[0],
                                    command=lambda:self.show_hand(self.deck_R)))
        deck_btns[0].image = deck_images[0]
        deck_btns[0].grid(row=2, column=1, padx=20, pady=20)

            # (b) Rouleur Exhaustion
        deck_images.append(ImageTk.PhotoImage(Image.open(f'Images/{DECK_PATHS[1]}.jpg').resize((int(CARD_W*W_MULT), CARD_H))))
        deck_btns.append(tk.Button(master=self.frm_decks, width=int(CARD_W*W_MULT), height=CARD_H, image=deck_images[1],
                                    command=lambda:self.pickup_exhaust(self.deck_R.recycleDeck)))
        deck_btns[1].image = deck_images[1]
        deck_btns[1].grid(row=2, column=2, padx=20, pady=20)

            # (c) FR Logo
        deck_images.append(ImageTk.PhotoImage(Image.open(f'Images/{DECK_PATHS[2]}.jpg').resize((int(CARD_W*2), CARD_H))))
        deck_btns.append(tk.Label(master=self.frm_decks, width=int(CARD_W*2.2), height=CARD_H, image=deck_images[2]))
        deck_btns[2].image = deck_images[2]
        deck_btns[2].grid(row=2, column=3, padx=20, pady=20)

            # (d) Sprinter Deck
        deck_images.append(ImageTk.PhotoImage(Image.open(f'Images/{DECK_PATHS[3]}.jpg').resize((int(CARD_W * W_MULT), CARD_H))))
        deck_btns.append(tk.Button(master=self.frm_decks, width=int(CARD_W * W_MULT), height=CARD_H, image=deck_images[3],
                                   command=lambda: self.show_hand(self.deck_S)))
        deck_btns[3].image = deck_images[3]
        deck_btns[3].grid(row=2, column=4, padx=20, pady=20)

            # (e) Sprinter Exhaustion
        deck_images.append(ImageTk.PhotoImage(Image.open(f'Images/{DECK_PATHS[4]}.jpg').resize((int(CARD_W * W_MULT), CARD_H))))
        deck_btns.append(tk.Button(master=self.frm_decks, width=int(CARD_W * W_MULT), height=CARD_H, image=deck_images[4],
                                   command=lambda: self.pickup_exhaust(self.deck_S.recycleDeck)))
        deck_btns[4].image = deck_images[4]
        deck_btns[4].grid(row=2, column=5, padx=20, pady=20)

        # (f) Rouleur: deck label & card count
        tk.Label(master=self.frm_decks, text='Rouleur Deck', font=('Verdana', 20), anchor=tk.S).grid(row=1,column=1)
        self.vR = tk.StringVar()
        tk.Label(master=self.frm_decks, textvariable=self.vR, font=('Verdana', 12), anchor=tk.N).grid(row=3,column=1)
        self.vRrec = tk.StringVar()
        tk.Label(master=self.frm_decks, textvariable=self.vRrec, font=('Verdana', 12), anchor=tk.N).grid(row=4, column=1)

        # (g) Sprinter: deck label &  card count
        tk.Label(master=self.frm_decks, text='Sprinter Deck', font=('Verdana', 20), anchor=tk.S).grid(row=1, column=4)
        self.vS = tk.StringVar()
        tk.Label(master=self.frm_decks, textvariable=self.vS, font=('Verdana', 12), anchor=tk.N).grid(row=3, column=4)
        self.vSrec = tk.StringVar()
        tk.Label(master=self.frm_decks, textvariable=self.vSrec, font=('Verdana', 12), anchor=tk.N).grid(row=4, column=4)

        # (h) Exhaustion labels
        tk.Label(master=self.frm_decks, text='Rouleur Exhaustion Cards', font=('Verdana', 20), anchor=tk.S).grid(row=1, column=2)
        tk.Label(master=self.frm_decks, text='Sprinter Exhaustion Cards', font=('Verdana', 20), anchor=tk.S).grid(row=1, column=5)

        # (4) Footer Widgets
        self.btn_close = tk.Button(master=self.frm_footer, text='Close', anchor=tk.S,
                                   relief=tk.RAISED, font=('Verdana', 25), command=self.handle_close,
                                   padx=10, pady=10).pack(side=tk.BOTTOM)

        # Pack the frames
        self.frm_header.pack(side=tk.TOP)
        self.frm_decks.pack(side=tk.TOP)
        self.frm_cards.pack(side=tk.TOP)
        self.frm_footer.pack(side=tk.BOTTOM)
        self.update_counts()

    ### END INIT

    #Properties, getters
    @property
    def deck_R(self):
        return self._deck_R

    @property
    def deck_S(self):
        return self._deck_S


    # Handle events, clicks etc
    def handle_close(self):
        self.master.destroy()

    def show_hand(self, deck: GL.MainDeck, num=HANDSIZE):
        # Check we have enough cards, if not, shuffle and reintroduce cards in the recycle deck
        if deck.count_cards < num:
            messagebox.askokcancel('information', f'You have used all your {deck.rider} cards!  Shuffling and reintroducing recycled cards...',)
            deck.refresh_deck()
            self.update_counts()

        hand = GL.Deck(deck.team, deck.rider, deck.teamtype)
        self.frm_cards.pack()

        for i in range(0,num):
            crd = deck.get_card(i)
            hand.add(crd)

        # Hand widgets
        imgs = hand.getFilePaths()
        hand_photos = []
        for im in imgs:
            hand_photos.append(ImageTk.PhotoImage(Image.open(im).resize((CARD_W, CARD_H))))

        hand_btns = []
        for i in range(len(imgs)):
            hand_btns.append(tk.Button(master=self.frm_cards, width=CARD_W, height=CARD_H,
                                       image=hand_photos[i],
                                       command=lambda i=i: self.playthecard(hand, i)))
            hand_btns[i].image = hand_photos[i]
            hand_btns[i].grid(row=1, column=i, padx=20, pady=20)

    # Pick up an exhaustion card and add to the recycle deck for that rider
    def pickup_exhaust(self, deck: GL.Deck, num=1):
        for i in range(0,num):
            answer = messagebox.askokcancel("Question", f'Collect an exhaustion card for your {deck.rider.upper()}?')
            if answer:
                ex_crd = GL.ExhaustCard()
                deck.add(ex_crd)
        self.update_counts()

    # Given a subdeck (e.g. a hand), returns the relevant main deck of cards, i.e. either the player's Rouleur deck or Sprinter Deck
    def getTargetDeck(self, D: GL.Deck) -> GL.MainDeck:
        if D.rider[0:1].upper() == 'R':
            return self.deck_R
        elif D.rider[0:1].upper() == 'S':
            return self.deck_S

    # Play the selected card; i.e. burn it, eliminate from the deck without recycling. Recycle the rest of the hand.
    def playthecard(self, hand: GL.Deck, i: int):
        answer = messagebox.askokcancel('information', f'Play the {hand.rider} {hand.get_card(i).value} card and recycle the other cards?', )
        if answer:
            currentDeck = self.getTargetDeck(hand)
            ToRecycle = hand.getRemainders(i)
            for index in ToRecycle:
                currentDeck.recycle(index) #Recycle the unused cards into recycle deck
            currentDeck.remove_card(0)  # Index 0 not index i. After remainder cards have been recycled (above), the first card in the Maindeck list will be that selected by the player.
            self.frm_cards.pack_forget()  # Clear the hand widgets from view (does not destroy!)
            self.update_counts()

    # Update the main deck card counts on the GUI
    def update_counts(self):
        self.vR.set(f'{self.deck_R.count_cards} cards in Rouleur deck')
        self.vS.set(f'{self.deck_S.count_cards} cards in Sprinter deck')
        self.vRrec.set(f'and {self.deck_R.recycleDeck.count_cards} recycled cards')
        self.vSrec.set(f'and {self.deck_S.recycleDeck.count_cards} recycled cards')

# Run it
def main():
    window = tk.Tk()
    app = GameWindow(window, 'Pink', 'Player')
    window.mainloop()

if __name__ == '__main__':
    print('GUI Main called')
    main()

And finally... this probably didn't need a file of it's own did it lololol..

import GUI
import tkinter as tk

# Launch the game window
window = tk.Tk()
app = GUI.LaunchWindow(window)
window.mainloop()

Thank you very much in advance!

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Hey RoadBadger :) There's one important skill you can learn using the CodeReview website and it's to give good context on what code is supposed to do :) Right now, you state that game rules aren't important, but that's you'd like to make sure your code doesn't have bugs you didn't see. But how can we know this if we don't know the rules :) You'll need to give us the context of the code (and change your post title to what the code does!) in order for us to help! \$\endgroup\$
    – IEatBagels
    Nov 26, 2020 at 18:53
  • 1
    \$\begingroup\$ I don't want to vote to close this question, since a little edit that adds proper context will make it good! \$\endgroup\$
    – user228914
    Nov 26, 2020 at 19:15
  • 1
    \$\begingroup\$ Thanks for the feedback guys - will edit the question to explain what the code is supposed to do. \$\endgroup\$
    – RoadBadger
    Nov 26, 2020 at 20:07

2 Answers 2

1
\$\begingroup\$

I'll include inline comments on your second file.

############
# GUI Module
############
import tkinter as tk
from PIL import Image, ImageTk
import GameLogic as GL
from tkinter import messagebox

# Constants
CARD_W = 180
CARD_H = 300
W_MULT = 1.2
TEAMS = ('Blue', 'Red', 'Green', 'Black', 'White', 'Pink')
RIDERS = ('Rouleur', 'Sprinter')
PLAYER = ('Player', 'Bot: Muscle Team', 'Bot: Peloton Team')
HANDSIZE = 4
DECK_PATHS = ('deck_main_R', 'deck_exhaust_R', 'FR_logo', 'deck_main_S', 'deck_exhaust_S')

Parentheses are the syntax for a tuple, which usually signifies a heterogeneous bunch of related things that are parts of a whole. But it would be more idiomatic here to use square brackets and create a list, since it is a bunch of things of the same type which are accessed sequentially and are distinct from each other. It doesn't change the functionality.

##################
# LaunchWindow Class
##################
class LaunchWindow:

    def __init__(self, master):
        self.master = master

        # Make the frames
        self.frm_header = tk.Frame(self.master)
        self.frm_options = tk.Frame(self.master)
        self.frm_colour = tk.Frame(master=self.frm_options)
        self.frm_player = tk.Frame(master=self.frm_options)
        self.frm_footer = tk.Frame(master=self.master)

        # (1) Header widgets
        tk.Label(master=self.frm_header, text='Flamme Rouge', anchor=tk.NW, font=('Verdana', 50), fg='red').pack(side=tk.TOP, pady=10, padx=10)
        tk.Label(master=self.frm_header, text='Select team type options', anchor=tk.NW, font=('Verdana', 25)).pack(side=tk.TOP,pady=5)

        # (2) Team Choice Widgets
        #   (A) Team Colour Choice Input
        colour_choice = tk.StringVar()
        tk.Label(master= self.frm_colour, text='Team colour:').pack(side=tk.TOP, pady = 3)

Both here and in other places, the spacing around the named parameters' =s is inconsistent -- pick a style and stick with it.

        for team in TEAMS:
            textcolour = 'White'
            if team == 'White' or team == 'Pink':
                textcolour = 'Black'

It would be slightly more idiomatic to define LIGHT_COLOURS outside the loop and then condense this to one line, e.g. textcolour = 'Black' if team in LIGHT_COLOURS else 'White' to avoid repeating team and textcolour

            rb_teams = tk.Radiobutton(master= self.frm_colour, text=team, value=team, variable=colour_choice,  bg=team, fg=textcolour)
            rb_teams.pack(anchor=tk.N)

Why do you create a variable rb_teams only to immediately throw it away? Just put the .pack() at the end of the line like you did with the other UI elements you're not concerned with.

        #   (B) Player (Human or Bot) Choice Input
        player_choice = tk.StringVar()
        tk.Label(master=self.frm_player, text='Team Type:').pack(side=tk.TOP, pady=3)
        for p in PLAYER:
            rb_teams = tk.Radiobutton(master=self.frm_player, text=p, value=p, variable=player_choice, indicatoron=0)
            rb_teams.pack(anchor=tk.N)

This is wrong and confusing; these aren't teams buttons but player buttons. Lose the variable name and put the .pack on the same line.

        # (3)  Footer Widgets
        self.btn_start = tk.Button(master=self.frm_footer, text='Start Game', anchor=tk.S,
                                   relief=tk.RAISED, font=('Verdana', 35),
                                   command=lambda: self.start_game(colour_choice.get(), player_choice.get()),
                                   padx=20, pady=10).pack(side=tk.LEFT)

The lambda here is confusing, as you're mixing program logic into the middle of UI definitions. Create a separate method.

        self.btn_close = tk.Button(master=self.frm_footer, text='Close', anchor=tk.S,  padx=20, pady=10,
                                   relief=tk.RAISED, font=('Verdana', 25), command=self.handle_close).pack(side=tk.LEFT)
        #Pack the frames
        self.frm_header.pack(side=tk.TOP)
        self.frm_colour.pack(side=tk.LEFT)
        self.frm_player.pack(side=tk.LEFT)
        self.frm_options.pack(side=tk.TOP)
        self.frm_footer.pack(side=tk.BOTTOM, pady = 10)

This is confusing; other items were packed at the time of creation, but these are packed after a bunch of other stuff is done. Also the comment doesn't add anything, it's obvious from the code. Comments should explain why something is being done. The code itself should make it clear what is being done.

    # Handle events, clicks etc
    def handle_close(self):
        self.master.destroy()

    def start_game(self, colour: str, team_type: str):
        self.master.destroy()
        game_window = tk.Tk()
        game = GameWindow(game_window, colour, team_type)



##################
# GameWindow Class
##################
class GameWindow:

One class per file usually, please.

    def __init__(self, master, colour, teamtype): # Create labels, entries,buttons

Again this comment is more confusing than helpful. Anyone looking at the code and see that's what it's doing. Don't add comments just for the sake of having comments; the comments should explain your thinking and make it easier for someone to understand the code.

        self.master = master

        # Make the frames
        self.frm_header = tk.Frame(self.master)
        self.frm_decks = tk.Frame(self.master)
        self.frm_cards = tk.Frame(self.master)
        self.frm_footer = tk.Frame(self.master)

        # Initialise the Card decks according to user input on launch window

Except you don't know that. Classes/methods/functions called with parameters shouldn't know or care where their parameters came from. What if you at some point created a "game replay" feature that allowed replaying of a past game -- then the params would not come from user input in the launch window.

        self._deck_R = GL.MainDeck(colour, 'Rouleur', teamtype)
        self._deck_S = GL.MainDeck(colour, 'Sprinter', teamtype)
        self.deck_R.shuffle
        self.deck_S.shuffle

This is confusing; these two decks are clearly the same kind of thing. Looks like the code will alternate between them. Why not put them in a list that you then index into? Then you could do things like for deck in self.decks: deck.shuffle rather than duplicating code.

Also I don't understand why you're sometimes using the underscore version and sometimes the non-underscore version. What's the benefit of making a member private if you then go and make it completely accessible?

        # (1) Header Widgets
        tk.Label(master=self.frm_header, text=colour+' Team', fg=colour, bg='grey', anchor=tk.NW, font=('Verdana', 35, 'bold')).pack(side=tk.TOP, pady=10)

I don't like repeated strings 'Verdana' -- you should put the font name in a constant so you can easily change it everywhere.

        # (2) Deck Widgets
        deck_images = []
        deck_btns = []

Okay. Here is a key clue about what you're doing wrong. Python has very strong list manipulation techniques. It's almost never ideal to start with an empty list and then add items to it. Instead, create a list that defines how it will be created, and then use list comprehensions.

A related issue is, are deck_images and deck_btns the same size and corresponding to each other? If so they should probably be a single list of tuples, classes, or collections.namedtuples.

e.g. DECK_CONFIG as a mixture of DECK_PATHS and DECK_IMAGES, then say deck_ui = [(ImageTk.PhotoImage(Image.open(f'Images/{dc.path}.jpg').resize(dc.dimensions), tk.Button(..., image=dc.image, command=dc.command) for dc in deck_config]

            # (a) Rouleur deck
        deck_images.append(ImageTk.PhotoImage(Image.open(f'Images/{DECK_PATHS[0]}.jpg').resize((int(CARD_W*W_MULT), CARD_H))))
        deck_btns.append(tk.Button(master=self.frm_decks, width=int(CARD_W*W_MULT), height=CARD_H, image=deck_images[0],
                                    command=lambda:self.show_hand(self.deck_R)))
        deck_btns[0].image = deck_images[0]

You already set that in the previous line.

        deck_btns[0].grid(row=2, column=1, padx=20, pady=20)

            # (b) Rouleur Exhaustion
        deck_images.append(ImageTk.PhotoImage(Image.open(f'Images/{DECK_PATHS[1]}.jpg').resize((int(CARD_W*W_MULT), CARD_H))))
        deck_btns.append(tk.Button(master=self.frm_decks, width=int(CARD_W*W_MULT), height=CARD_H, image=deck_images[1],
                                    command=lambda:self.pickup_exhaust(self.deck_R.recycleDeck)))
        deck_btns[1].image = deck_images[1]
        deck_btns[1].grid(row=2, column=2, padx=20, pady=20)

            # (c) FR Logo
        deck_images.append(ImageTk.PhotoImage(Image.open(f'Images/{DECK_PATHS[2]}.jpg').resize((int(CARD_W*2), CARD_H))))
        deck_btns.append(tk.Label(master=self.frm_decks, width=int(CARD_W*2.2), height=CARD_H, image=deck_images[2]))
        deck_btns[2].image = deck_images[2]
        deck_btns[2].grid(row=2, column=3, padx=20, pady=20)

            # (d) Sprinter Deck
        deck_images.append(ImageTk.PhotoImage(Image.open(f'Images/{DECK_PATHS[3]}.jpg').resize((int(CARD_W * W_MULT), CARD_H))))
        deck_btns.append(tk.Button(master=self.frm_decks, width=int(CARD_W * W_MULT), height=CARD_H, image=deck_images[3],
                                   command=lambda: self.show_hand(self.deck_S)))
        deck_btns[3].image = deck_images[3]
        deck_btns[3].grid(row=2, column=4, padx=20, pady=20)

            # (e) Sprinter Exhaustion
        deck_images.append(ImageTk.PhotoImage(Image.open(f'Images/{DECK_PATHS[4]}.jpg').resize((int(CARD_W * W_MULT), CARD_H))))
        deck_btns.append(tk.Button(master=self.frm_decks, width=int(CARD_W * W_MULT), height=CARD_H, image=deck_images[4],
                                   command=lambda: self.pickup_exhaust(self.deck_S.recycleDeck)))
        deck_btns[4].image = deck_images[4]
        deck_btns[4].grid(row=2, column=5, padx=20, pady=20)

        # (f) Rouleur: deck label & card count
        tk.Label(master=self.frm_decks, text='Rouleur Deck', font=('Verdana', 20), anchor=tk.S).grid(row=1,column=1)
        self.vR = tk.StringVar()
        tk.Label(master=self.frm_decks, textvariable=self.vR, font=('Verdana', 12), anchor=tk.N).grid(row=3,column=1)
        self.vRrec = tk.StringVar()
        tk.Label(master=self.frm_decks, textvariable=self.vRrec, font=('Verdana', 12), anchor=tk.N).grid(row=4, column=1)

It's confusing here to intersperse UI setup and setting members of the class. Again this should probably be a config list and then a loop.

        # (g) Sprinter: deck label &  card count
        tk.Label(master=self.frm_decks, text='Sprinter Deck', font=('Verdana', 20), anchor=tk.S).grid(row=1, column=4)
        self.vS = tk.StringVar()
        tk.Label(master=self.frm_decks, textvariable=self.vS, font=('Verdana', 12), anchor=tk.N).grid(row=3, column=4)
        self.vSrec = tk.StringVar()
        tk.Label(master=self.frm_decks, textvariable=self.vSrec, font=('Verdana', 12), anchor=tk.N).grid(row=4, column=4)

I really suggest putting these v* elements into a list or map rather than having a bunch of separate variables.

        # (h) Exhaustion labels
        tk.Label(master=self.frm_decks, text='Rouleur Exhaustion Cards', font=('Verdana', 20), anchor=tk.S).grid(row=1, column=2)
        tk.Label(master=self.frm_decks, text='Sprinter Exhaustion Cards', font=('Verdana', 20), anchor=tk.S).grid(row=1, column=5)

        # (4) Footer Widgets
        self.btn_close = tk.Button(master=self.frm_footer, text='Close', anchor=tk.S,
                                   relief=tk.RAISED, font=('Verdana', 25), command=self.handle_close,
                                   padx=10, pady=10).pack(side=tk.BOTTOM)

        # Pack the frames
        self.frm_header.pack(side=tk.TOP)
        self.frm_decks.pack(side=tk.TOP)
        self.frm_cards.pack(side=tk.TOP)
        self.frm_footer.pack(side=tk.BOTTOM)
        self.update_counts()

    ### END INIT

    #Properties, getters
    @property
    def deck_R(self):
        return self._deck_R

This is useless. The purpose of having a getter is to control access to the member and make sure that only authorized actions are performed. But you're allowing anyone to perform any action. Better to not even have it.

    @property
    def deck_S(self):
        return self._deck_S


    # Handle events, clicks etc
    def handle_close(self):
        self.master.destroy()

    def show_hand(self, deck: GL.MainDeck, num=HANDSIZE):
        # Check we have enough cards, if not, shuffle and reintroduce cards in the recycle deck
        if deck.count_cards < num:
            messagebox.askokcancel('information', f'You have used all your {deck.rider} cards!  Shuffling and reintroducing recycled cards...',)
            deck.refresh_deck()
            self.update_counts()

        hand = GL.Deck(deck.team, deck.rider, deck.teamtype)
        self.frm_cards.pack()

        for i in range(0,num):

Just use range(num); 0 is the default start

            crd = deck.get_card(i)
            hand.add(crd)

        # Hand widgets
        imgs = hand.getFilePaths()
        hand_photos = []
        for im in imgs:
            hand_photos.append(ImageTk.PhotoImage(Image.open(im).resize((CARD_W, CARD_H))))
        hand_btns = []
        for i in range(len(imgs)):
            hand_btns.append()
            hand_btns[i].image = hand_photos[i]
            hand_btns[i].grid(row=1, column=i, padx=20, pady=20)

Use list comprehension: hand_btns = [tk.Button(master=self.frm_cards, width=CARD_W, height=CARD_H, image=ImageTk.PhotoImage(Image.open(im).resize((CARD_W, CARD_H))), command=lambda i=i: self.playthecard(hand, i)) for (i,im) in enumerate(imgs)]

    # Pick up an exhaustion card and add to the recycle deck for that rider
    def pickup_exhaust(self, deck: GL.Deck, num=1):
        for i in range(0,num):
            answer = messagebox.askokcancel("Question", f'Collect an exhaustion card for your {deck.rider.upper()}?')
            if answer:
                ex_crd = GL.ExhaustCard()
                deck.add(ex_crd)
        self.update_counts()

    # Given a subdeck (e.g. a hand), returns the relevant main deck of cards, i.e. either the player's Rouleur deck or Sprinter Deck
    def getTargetDeck(self, D: GL.Deck) -> GL.MainDeck:
        if D.rider[0:1].upper() == 'R':

You can just use D.rider[0].upper(), but this makes it clear that deck_R and deck_S should be in a map that's indexed by 'R' or 'S'

            return self.deck_R
        elif D.rider[0:1].upper() == 'S':
            return self.deck_S

    # Play the selected card; i.e. burn it, eliminate from the deck without recycling. Recycle the rest of the hand.
    def playthecard(self, hand: GL.Deck, i: int):
        answer = messagebox.askokcancel('information', f'Play the {hand.rider} {hand.get_card(i).value} card and recycle the other cards?', )
        if answer:
            currentDeck = self.getTargetDeck(hand)
            ToRecycle = hand.getRemainders(i)
            for index in ToRecycle:
                currentDeck.recycle(index) #Recycle the unused cards into recycle deck
            currentDeck.remove_card(0)  # Index 0 not index i. After remainder cards have been recycled (above), the first card in the Maindeck list will be that selected by the player.
            self.frm_cards.pack_forget()  # Clear the hand widgets from view (does not destroy!)
            self.update_counts()

    # Update the main deck card counts on the GUI
    def update_counts(self):
        self.vR.set(f'{self.deck_R.count_cards} cards in Rouleur deck')
        self.vS.set(f'{self.deck_S.count_cards} cards in Sprinter deck')
        self.vRrec.set(f'and {self.deck_R.recycleDeck.count_cards} recycled cards')
        self.vSrec.set(f'and {self.deck_S.recycleDeck.count_cards} recycled cards')

# Run it
def main():
    window = tk.Tk()
    app = GameWindow(window, 'Pink', 'Player')
    window.mainloop()

if __name__ == '__main__':
    print('GUI Main called')
    main()
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Hi @Snowbody thank you so much! That's just what I needed. Your comments are insightful and clear. I am delighted with the feedback. You clearly know your stuff. Just one question: I completely agree with the sentiment of "why make a member private if you then go and make it completely accessible?" and "the purpose of a getter is to control access to the member and make sure that only authorised actions are performed but you're allowing anyone to perform any action", but I don't quite see how/where I am doing that? I am so grateful, please let me know if I can return the favour somehow. \$\endgroup\$
    – RoadBadger
    Dec 2, 2020 at 17:33
  • \$\begingroup\$ Yeah, I think you're right. I misunderstood that one. You can return the favor by becoming a better programmer day by day, and when you have spare time helping out other programmers become better, in whatever location suits you. \$\endgroup\$
    – Snowbody
    Dec 4, 2020 at 6:45
  • \$\begingroup\$ Ok, it's a deal! Thanks again \$\endgroup\$
    – RoadBadger
    Dec 12, 2020 at 20:15
1
\$\begingroup\$

I can point out a few Python language features that will make the code a bit simpler. In show():

@property
def show(self):
    i = 0
    total = self.count_cards
    for crd in self.cards:
        i += 1
        print(f'Value of card {i} of {total} in {self.team} {self.rider} deck is {crd.value}')
        if not isinstance(self,MainDeck):
            print('This is not the main deck')
  1. Not a @property since it doesn't return anything
  2. Use the enumerate() function, let python handle the index variable
  3. The "not main deck" warning should be shown just once, not every card.

So, for instance:

    def show(self):
        total = self.count_cards
        for (i, crd) in enumerate(self.cards):
            print(f'Value of card {i} of {total} in {self.team} {self.rider} deck is {crd.value}')
        if not isinstance(self,MainDeck):
            print('This is not the main deck')

In shuffle():

@property
def shuffle(self):
    for i in range(self.count_cards -1, -1, -1):
        rnd = random.randint(0, i)
        self.cards[i], self.cards[rnd] = self.cards[rnd], self.cards[i]
  1. Again, definitely not a @property

  2. Python has a shuffle() in the random module, no need to reinvent the wheel

     import random
    
     ...
    
         def shuffle(self):
             random.shuffle(self.cards)
    

In countExhaustion():

@property
def countExhaustion(self) -> int:
    cnt = 0
    for crd in self.cards:
        if isinstance(crd, ExhaustCard):
            cnt += 1
    return cnt
  1. Yes this is a @property
  2. But it might be easier to just have an additional variable and keep it updated, since you know exactly when cards are added or removed whether it's an ExhaustCard
  3. If you decide not to do that, use sum() to count:
    @property
    def countExhaustion(self) -> int:
        return sum(1 for crd in self.cards if isinstance(crd, ExhaustCard))

In MainDeck.__init__:

        if rider[0:1].upper() == 'R':
            for i in range(len(ROULEUR)):
                self._cards.append(Card(ROULEUR[i]))
        elif rider[0:1].upper() == 'S':
            for i in range(len(SPRINTER)):
                self._cards.append(Card(SPRINTER[i]))

This code is duplicated. You should write the code only once and use a dict to map between the first character of rider and which deck. Also the extend() method is defined for sequences which should be preferred over append() in a loop.

deckMap = {'R': ROULEUR, 'S': SPRINTER}
deck = deckMap[rider[0]].upper()
self._cards.extend(deck)
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Just a small nitpick: there's no need to wrap (i, crd) around parentheses here: for (i, crd) in enumerate(self.cards). \$\endgroup\$ Nov 27, 2020 at 7:47
  • \$\begingroup\$ Hi @Snowbody, thank you very much for the useful feedback! Much appreciated. \$\endgroup\$
    – RoadBadger
    Nov 28, 2020 at 15:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.