Skip to main content
edited body
Source Link
Cris Luengo
  • 7k
  • 1
  • 15
  • 37

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 12 always wins.

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 1 always wins.

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.

Source Link
Cris Luengo
  • 7k
  • 1
  • 15
  • 37

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 1 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.