3
\$\begingroup\$

I already know that I shouldn't really use the global function and that how I judge who wins is also really inefficient so I would appreciate improvements for that. I watched a tutorial about Object Oriented Programming and this was my first attempt at using what i learned. I redid a game I did without OOP.

The Rules for the Game are :

  • Player 1 takes the top card from the deck.

  • Player 2 takes the next card from the deck. If both players have a card of the same colour, the player with the highest number wins.

  • If both players have cards with different colours, the winning colour is shown in the table.

Card Card Winner
Red Black Red
Yellow Red Yellow
Black Yellow Black
  • The winner of each round keeps both cards.
  • The players keep playing until there are no cards left in the deck
  • Players need to be validated
from random import shuffle 
Ranks = [ "1", "2", "3", "4", "5", "6", "7","8", "9", "10"]
Colours = [ "Y", "R", "B"]

class Card() :
  def __init__(self,rank,colour):
    self.rank = rank
    self.colour = colour
  def get_card(self) :
    return self.rank + self.colour
  def get_move(self) :
    return self.rank
  def get_colour(self) :
    return self.colour

class Deck() :
  def __init__(self) :
    self.deck = []
    self.tempdeck = []
    for Colour in Colours :
      for Rank in Ranks :
        self.tempdeck.append(str(Rank) + "-" + str(Colour))
    for x in self.tempdeck :
      s = x.split("-")
      x = Card(s[0],s[1])
      self.deck.append(x)
    self.shuffle_deck()
    
  def len(self) :
    return len(self.deck)
    
  def add_card(self,card) :
    self.deck.append(card)
    
  def remove_card(self,card) :
    self.deck.remove(card)

  def shuffle_deck(self) :
    shuffle(self.deck)
class Hand(Deck):
  def __init__(self,label) :
    self.deck = []
    self.label = label
    self.Score = 0
  def PrintCard(self) :
    Cards = []
    for x in self.deck :
      Cards.append(x.get_card())
    return str(Cards)
def WinnerDecider() :
  if P1Hand.len() > P2Hand.len() :
    print("Player 1 wins with, " + str(P1Hand.len() - P2Hand.len()) + "More Cards than Player 2" )
    print("Your Cards were:" , P1Hand.PrintCard())
  elif P1Hand.len() < P2Hand.len() :
    print("Player 2 wins with, " + str(P2Hand.len() - P1Hand.len()) + " More Cards than Player 1" )
    print("Your Cards were:" , P2Hand.PrintCard())
  elif P1Hand.len() == P2Hand.len() :
    print("Draw")
def RoundJudge() :
  if P1Move.get_colour() == "R" :
    if P2Move.get_colour() == "B" :
      P1Hand.add_card(P1Move)
      P1Hand.add_card(P2Move)
      print("P1 Won Got these cards:" + P1Move.get_card() + "," + P2Move.get_card())
    else :
      P2Hand.add_card(P1Move)
      P2Hand.add_card(P2Move)
      print("P2 Won Got these cards:" + P1Move.get_card() + "," + P2Move.get_card())
  if P1Move.get_colour() == "Y" :
    if P2Move.get_colour() == "R" :
      P1Hand.add_card(P1Move)
      P1Hand.add_card(P2Move)
      print("P1 Won Got these cards:" + P1Move.get_card() + "," + P2Move.get_card())
    else :
      P2Hand.add_card(P1Move)
      P2Hand.add_card(P2Move)
      print("P2 Won Got these cards:" + P1Move.get_card() + "," + P2Move.get_card())
  if P1Move.get_colour() == "B" :
    if P2Move.get_colour() == "Y" :
      P1Hand.add_card(P1Move)
      P1Hand.add_card(P2Move)
      print("P1 Won Got these cards:" + P1Move.get_card() + "," + P2Move.get_card())
    else :
      P2Hand.add_card(P1Move)
      P2Hand.add_card(P2Move)
      print("P2 Won Got these cards:" + P1Move.get_card() + "," + P2Move.get_card())
def Game() : 
  global P1Move
  global P2Move
  while Deck.len() > 0 :
    P1Choice = input(P1Name + " Press Enter to take your turn: ")
    if P1Choice == "" :
      P1Move = Deck.deck[0]
      Deck.remove_card(P1Move)
      print("You got: " , P1Move.get_card())
    P2Choice = input(P2Name + " Press Enter to take your turn: ")
    if P2Choice == "" :
      P2Move = Deck.deck[0]
      Deck.remove_card(P2Move)
      print("You got: " , P2Move.get_card())
    RoundJudge()
  WinnerDecider()
      
def Start() :
  global P1Hand
  global P2Hand
  global Deck
  global P1Name
  global P2Name
  P1Name = input("Name: ")
  P2Name = input("Name: ")
  Deck = Deck()
  P1Hand = Hand(P1Name)
  P2Hand = Hand(P2Name)
  Game()
def Login() :
  User1 = input("Enter Username:")
  Pass1 = input("Enter Password:")
  User2 = input("Enter Username:")
  Pass2 = input("Enter Password:")
  if User1 == "" and Pass1 == "" and User2 == "" and Pass2 == "" :
    Start()
  else :
    Login()



Login()
Improve this question
\$\endgroup\$
3
  • \$\begingroup\$ Your game is biased! Player 1 always wins if both players draw the same color. \$\endgroup\$
    – Cris Luengo
    Jul 17, 2022 at 15:01
  • \$\begingroup\$ @CrisLuengo Ish. It's biased toward player 2, not player 1. \$\endgroup\$
    – Reinderien
    Jul 18, 2022 at 14:12
  • 1
    \$\begingroup\$ @Reinderien You're right. Good catch. \$\endgroup\$
    – Cris Luengo
    Jul 18, 2022 at 14:35

2 Answers 2

Reset to default
2
\$\begingroup\$

Most Python programmers follow PEP 8 (the official Python style guide) when writing code. I am probably not as concerned as most in following the exact style, but some of those guidelines do improve legibility. For example, adding empty lines around function and class definitions makes it easier to see the code structure. But the most important style guideline, which is true for any language you program in, is consistency within a code base. If you put a space before the : at the end of a def statement, do that for all such statements. Your class Hand(Deck): line breaks the consistency.

This is Deck.__init__():

    self.deck = []
    self.tempdeck = []
    for Colour in Colours :
      for Rank in Ranks :
        self.tempdeck.append(str(Rank) + "-" + str(Colour))
    for x in self.tempdeck :
      s = x.split("-")
      x = Card(s[0],s[1])
      self.deck.append(x)
    self.shuffle_deck()

tempdeck is not used outside this function, then why make it a member of the class? Don't do self.tempdeck, just do tempdeck. Then it will not stick around after the function finishes.

But tempdeck is not needed at all. It just adds confusion and work. Why not just do

    self.deck = []
    self.tempdeck = []
    for Colour in Colours :
      for Rank in Ranks :
        self.deck.append(Card(Rank, Colour))
    self.shuffle_deck()

RoundJudge() has a bug: it does not correctly implement the case where both players get a card with the same color. Instead of letting the higher number win, player 2 always wins.

This function also has a lot of redundancy. The lines

      P1Hand.add_card(P1Move)
      P1Hand.add_card(P2Move)
      print("P1 Won Got these cards:" + P1Move.get_card() + "," + P2Move.get_card())
    else :
      P2Hand.add_card(P1Move)
      P2Hand.add_card(P2Move)
      print("P2 Won Got these cards:" + P1Move.get_card() + "," + P2Move.get_card())

are repeated three times. Instead, first find out who wins, then put the cards in the hand of the winner. For example,

def RoundJudge() :
  if P1Move.get_colour() == "R" :
    if P2Move.get_colour() == "B" :
      winner = P1Hand
    else:
      winner = P2Hand
  if P1Move.get_colour() == "Y" :
    if P2Move.get_colour() == "R" :
      winner = P1Hand
    else :
      winner = P2Hand
  if P1Move.get_colour() == "B" :
    if P2Move.get_colour() == "Y" :
      winner = P1Hand
    else :
      winner = P2Hand
winner.add_card(P1Move)
winner.add_card(P2Move)

(the comparison of the colors could be done in a prettier way as well, but let's keep it simple for now.)

You are using global variables there, as well as in WinnerDecider(). Why not make a class Game that contains the deck and the two player's hands, and make RoundJudge() and WinnerDecider() class methods? I mean, if you go for an OOP design, you might as well go all the way. :)

Note that Hand does not need to inherit from Deck, since it doesn't need to store cards at all. The only important information is how many cards are in the hand. All you need is a counter.

Taking a card from the deck is also awkward:

      P1Move = Deck.deck[0]
      Deck.remove_card(P1Move)

I would prefer something like

      P1Move = Deck.take()

The take() method here is the classical POP operation, it extracts the top element from the deck. Note that your remove_card (which uses list.remove()) needs to do a lookup to find the element P1Move, then then removes it. But you are always removing the top element, so the lookup is redundant. list.pop() is the right operation to uses. Without arguments, pop() removes and returns the last element in the list. You could do pop(0), or you could just do the natural thing and take cards from the end of the list. It does not matter which way you read the list anyway.

Improve this answer
\$\endgroup\$
1
  • \$\begingroup\$ Thank you, I really wished that I used what you said about a game class would've make things easier. I stored the cards so that I could print them out the end, it was probably unnecessary though.Thanks again! \$\endgroup\$
    – Flavour
    Jul 19, 2022 at 9:18
2
\$\begingroup\$

The formatting is pretty rough; follow PEP8 and get an IDE that points out formatting suggestions to you.

Ranks should be RANKS (due to being a global constant) and should only be a call to range(); its elements should be integers and not strings. But also, your game logic ignores the rank entirely. You should probably pay attention to it when the dealt colours match.

Colours should be an Enum.

get_move and get_colour should be deleted. get_card should be re-framed as an override of __str__. Consider representing Card as a NamedTuple for immutability and simple construction.

Hand ignores the card initialisation code in Deck, which suggests that there should be a third abstract parent of the two classes that doesn't take an opinion on card initialisation.

PrintCard doesn't print, so just call it __str__.

Your function names need to be action phrases; so decide_winner instead of WinnerDecider.

Simplify your win decision logic by comparing colour values; one way to do this is normalise the values so that the distance between them is always 1.

Don't use globals.

You don't use the result from Press enter to take your turn, so don't save it to a variable.

Never input() to get a password; use getpass() instead.

Don't print P1/P2; use the player labels that you collected earlier.

Add a __main__ guard.

If login fails, don't recurse; just loop.

Suggested

from enum import Enum
from getpass import getpass
from random import shuffle
from time import sleep
from typing import NamedTuple


class Colour(Enum):
    # Red > Black, Yellow > Red, Black > Yellow
    YELLOW = 0
    BLACK = 1
    RED = 2


RANKS = range(1, 11)


class Card(NamedTuple):
    rank: int
    colour: Colour

    def __str__(self) -> str:
        return f'{self.rank}{self.colour.name[0]}'


class Pile:
    cards: list[Card]

    def __init__(self) -> None:
        raise NotImplementedError()

    def len(self) -> int:
        return len(self.cards)

    def add_card(self, card: Card) -> None:
        self.cards.append(card)

    def remove_card(self, card: Card) -> None:
        self.cards.remove(card)

    def shuffle(self) -> None:
        shuffle(self.cards)

    def deal(self) -> Card:
        return self.cards.pop(0)

    def __str__(self) -> str:
        return ', '.join(str(card) for card in self.cards)


class Deck(Pile):
    def __init__(self) -> None:
        self.cards = [
            Card(rank, colour)
            for colour in Colour
            for rank in RANKS
        ]
        self.shuffle()


class Hand(Pile):
    def __init__(self, name: str) -> None:
        self.cards = []
        self.label = name


def decide_winner(p1_hand: Hand, p2_hand: Hand) -> None:
    if p1_hand.len() == p2_hand.len():
        print('Draw')
        return

    if p1_hand.len() > p2_hand.len():
        winner, loser = p1_hand, p2_hand
    else:
        loser, winner = p1_hand, p2_hand

    diff = winner.len() - loser.len()
    print(f'{winner.label} wins with {diff} more cards than {loser.label}')
    print(f"{winner.label}'s cards were: {winner}")


def judge_round(
    p1_move: Card, p2_move: Card,
    p1_hand: Hand, p2_hand: Hand,
) -> None:
    c1, c2 = p1_move.colour.value, p2_move.colour.value

    if c1 == c2:
        # winner = p2_hand  # p1 == p2: p2 wins
        # alternative tie-breaker: compare rank
        winning_rank, winner = max(
            (p1_move.rank, p1_hand),
            (p2_move.rank, p2_hand),
        )
    else:
        # Red > Black, Yellow > Red, Black > Yellow
        if max(c1, c2) == 2:
            if c1 == 0: c1 = 3
            elif c2 == 0: c2 = 3
        # At this point, the distance between c1 and c2 will always be 1, and
        # the higher value will be the winner
        winning_colour, winner = max(
            (c1, p1_hand),
            (c2, p2_hand),
        )

    winner.add_card(p1_move)
    winner.add_card(p2_move)
    print(f'{winner.label} won, got these cards: {p1_move}, {p2_move}\n')


def run_game(deck: Deck, p1_hand: Hand, p2_hand: Hand) -> None:
    while deck.len() > 0:
        moves = []

        for hand in (p1_hand, p2_hand):
            input(f'{hand.label}, press enter to take your turn.')
            move = deck.deal()
            print(f'You got: {move}')
            moves.append(move)

        judge_round(*moves, p1_hand, p2_hand)

    decide_winner(p1_hand, p2_hand)


def login() -> None:
    for player in range(1, 3):
        while True:
            username = input(f'Enter username for player {player}: ')
            password = getpass(f'Enter password for player {player}: ')
            if username == '' and password == '':
                break
            print('Invalid credentials')
            sleep(1)
        print('Logged in')


def main() -> None:
    login()
    p1_name = input('Player 1 Name: ')
    p2_name = input('Player 2 Name: ')
    print()

    run_game(
        deck=Deck(),
        p1_hand=Hand(p1_name),
        p2_hand=Hand(p2_name),
    )


if __name__ == '__main__':
    main()

Output

Enter username for player 1: 
Enter password for player 1: 
Logged in
Enter username for player 2: 
Enter password for player 2: 
Logged in
Player 1 Name: Julio
Player 2 Name: Fred

Julio, press enter to take your turn.
You got: 8B
Fred, press enter to take your turn.
You got: 5R
Fred won, got these cards: 8B, 5R

Julio, press enter to take your turn.
You got: 4R
Fred, press enter to take your turn.
You got: 4Y
Fred won, got these cards: 4R, 4Y

Julio, press enter to take your turn.
You got: 3B
Fred, press enter to take your turn.
You got: 7B
Fred won, got these cards: 3B, 7B

Julio, press enter to take your turn.
You got: 1R
Fred, press enter to take your turn.
You got: 3Y
Fred won, got these cards: 1R, 3Y

Julio, press enter to take your turn.
You got: 6R
Fred, press enter to take your turn.
You got: 8R
Fred won, got these cards: 6R, 8R

Julio, press enter to take your turn.
You got: 8Y
Fred, press enter to take your turn.
You got: 2B
Fred won, got these cards: 8Y, 2B

Julio, press enter to take your turn.
You got: 10B
Fred, press enter to take your turn.
You got: 9B
Julio won, got these cards: 10B, 9B

Julio, press enter to take your turn.
You got: 7R
Fred, press enter to take your turn.
You got: 7Y
Fred won, got these cards: 7R, 7Y

Julio, press enter to take your turn.
You got: 10R
Fred, press enter to take your turn.
You got: 2R
Julio won, got these cards: 10R, 2R

Julio, press enter to take your turn.
You got: 2Y
Fred, press enter to take your turn.
You got: 3R
Julio won, got these cards: 2Y, 3R

Julio, press enter to take your turn.
You got: 1Y
Fred, press enter to take your turn.
You got: 5B
Fred won, got these cards: 1Y, 5B

Julio, press enter to take your turn.
You got: 9R
Fred, press enter to take your turn.
You got: 4B
Julio won, got these cards: 9R, 4B

Julio, press enter to take your turn.
You got: 10Y
Fred, press enter to take your turn.
You got: 5Y
Julio won, got these cards: 10Y, 5Y

Julio, press enter to take your turn.
You got: 1B
Fred, press enter to take your turn.
You got: 6B
Fred won, got these cards: 1B, 6B

Julio, press enter to take your turn.
You got: 6Y
Fred, press enter to take your turn.
You got: 9Y
Fred won, got these cards: 6Y, 9Y

Fred wins with 10 more cards than Julio
Fred's cards were: 8B, 5R, 4R, 4Y, 3B, 7B, 1R, 3Y, 6R, 8R, 8Y, 2B, 7R, 7Y, 1Y, 5B, 1B, 6B, 6Y, 9Y
Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ Thank you I don't really know all the coding conventions for like names of class that's why my name's are all over the place and I didn't even know getpass() existed. Can I ask what you mean by making colours an enum and what you meant by a adding main guard \$\endgroup\$
    – Flavour
    Jul 19, 2022 at 9:12
  • \$\begingroup\$ class Colour(Enum):; see docs.python.org/3/library/enum.html \$\endgroup\$
    – Reinderien
    Jul 19, 2022 at 11:38
  • \$\begingroup\$ realpython.com/python-main-function \$\endgroup\$
    – Reinderien
    Jul 19, 2022 at 11:40

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.