Dealing cards for a game - my first Python project

Question

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!

• IEatBagels: 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!
• user228914: I don't want to vote to close this question, since a little edit that adds proper context will make it good!
• RoadBadger: Thanks for the feedback guys - will edit the question to explain what the code is supposed to do.

Answer 1

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()

• RoadBadger: 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.
• Snowbody: 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.
• RoadBadger: Ok, it's a deal! Thanks again

Answer 2

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)

• Grajdeanu Alex: Just a small nitpick: there's no need to wrap (i, crd) around parentheses here: for (i, crd) in enumerate(self.cards).
• RoadBadger: Hi @Snowbody, thank you very much for the useful feedback! Much appreciated.