Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

My goal was to get my hands dirty in OOP by designing and using classes and getting started with inheritance and other OOP concepts.

I have written a very small code to play a card Game called "War". The rules are simple. Each person playing the game is given 1 card. The person with the highest card wins. I am not worrying too much about Ties right now. My code does work but I wanted feedback on my OOP usage.


import itertools
import random
class Cards:
    def __init__(self):
        self.suits = ['Spades','Hearts','Diamonds','Clubs']
        self.values = range(1,14)
        self.ActualCards = [] #Empty List to Append
        for Card in itertools.product(self.suits,self.values):
            self.ActualCards.append(Card) #Cartesian Product to Create Deck
    def GetRandomCard(self):
        RandomNumber = random.randint(0,51)
        CardToBeReturned = self.ActualCards[RandomNumber] #Generates Random Card
    return(CardToBeReturned)

class Player:
    def __init__(self,ID,Card):
        self.PlayerID = ID
        self.CardForPlayer = Card

class Game:
    def __init__(self,NameOfGame):
        self.name = NameOfGame

class SimpleWar(Game):
    def __init__(self,NumberOfPlayers):
        self.NumberOfPlayers = NumberOfPlayers
        self.PlayerList = []
    def StartGame(self):
        DeckOfCards = Cards()
        for playerID in range(0,self.NumberOfPlayers):
            CardForPlayer = DeckOfCards.GetRandomCard() #Deal Card to Player
            NewPlayer = Player(playerID,CardForPlayer) #Save Player ID and Card
            self.PlayerList.append(NewPlayer)
        self.DecideWinner()    
    def DecideWinner(self):
        WinningID = self.PlayerList[0] #Choose Player 0 as Potential Winner
        for playerID in self.PlayerList:
            if(playerID.CardForPlayer[1]>WinningID.CardForPlayer[1]):
                WinningID = playerID #Find the Player with Highest Card
        print "Winner is Player "+str(WinningID.PlayerID)
        print "Her Card was "+ str(WinningID.CardForPlayer[1]) + " of " + str(WinningID.CardForPlayer[0]) 


if __name__=="__main__":
    NewGame = SimpleWar(2)
    NewGame.StartGame()
share|improve this question
add comment

4 Answers

up vote 16 down vote accepted

Syntax Error

In Cards.GetRandomCard(), the return statement is incorrectly indented. (Also, I would recommend writing the return without parentheses.)

Style Conventions

Use lower_case_names for variables and methods.

For readability, please add one space after each comma, and one newline between functions definitions.

Modeling

Your Game class isn't useful. You never set or use NameOfGame. I recommend getting rid of the Game class altogether.

In a real-life card game, you would deal a card from the deck to each player, without replacement. In your code, you deal with replacement (i.e., there is a chance of dealing the same card to both players). A more realistic simulation would do a random.shuffle() on the array. When dealing, you would pop() a card from the list to remove it from the deck. 51 is a "magic" number; you should use len(self.ActualCards) - 1.

Cards is really a Deck. Rather than just a tuple of strings, you should have a Card class representing a single card. The Card class should have a __str__() method that returns a string such as "Ace of diamonds". If you also define comparison operators, then you can determine the winner using max(self.PlayerList, key=lambda player: player.CardForPlayer).

Expressiveness

In Python, you can usually avoid creating an empty list and appending to it:

self.ActualCards = [] #Empty List to Append
for Card in itertools.product(self.suits,self.values):
    self.ActualCards.append(Card) #Cartesian Product to Create Deck

Instead, you should be able to build it all at once:

self.actual_cards = list(itertools.product(self.suits, self.values))
share|improve this answer
    
PEP 8 suggests two newlines between function and class definitions. However, one is better than nothing. –  nyuszika7h Feb 19 at 14:44
    
@nyuszika7h PEP 8 only calls for one blank line between method definitions within a class, which is what matters here. –  200_success Feb 19 at 17:12
1  
@200_success: It would be hard to define a comparison for the Cards, as the order only has meaning in the context of a particular game. So it's a Game responsibility (War, Solitaire, etc) to compare two cards. –  mgarciaisaia Feb 19 at 18:02
    
@200_success You're right, I didn't bother to check if it's within a class. –  nyuszika7h Feb 20 at 17:30
add comment

From a testing perspective, it would be good, to inject the deck (Cards) and the players into the game. Also printing the results in the game object is not a very good idea, I think. Maybe it would be better, to return the round, that contains the winner. This could then also be used for logging or a mastership :)

Deck = Deck()
Game = SimpleWar(Deck)
Game.addPlayer(Player('Andrea'))
Game.addPlayer(Player('Bert'))
Round = Game.play()
Winner = Round.get_winner()
print('The winner is: ' + str(Winner))
print('The winning card is: ' + str(Winner.get_last_card()))

Deck.shuffle()
Round = Game.play()

As said before, the deck should contain card objects (or an array and build the card objects, when requested, if those card objects would be expensive) and the cards should have to be put back into the deck after each game round (or a reset() method of the deck could be called by the game). The question then would be, who remembers the winning card, after all cards have been returned into the deck. In the example above, this is the player (get_last_card()), but it could be stored in the Round, too.

This way you don't have any object instantiation in your classes, except for the deck, that builds card objects. This would be very good testable. For example, you can pass a deck mock into the game and define, which cards it should return in the single requests to test the detection of the winner.

Search the web for "dependency injection" for more information.

share|improve this answer
1  
what I like about your design is that you've thought through the interactions between objects, and that defines their methods. Also big plus for dependency injection. –  Rob Y Feb 19 at 19:11
add comment

Cool app. My comments aren't intended to be critical or haughty, just me thinking out loud.

  • Python 3? If not, make sure your classes extend object.

  • I probably wouldn't define and extend Game. It doesn't add anything. You don't need to generalize or abstract the idea of a Game, especially in this context.

  • More broadly, it's a good practice to avoid inheritance. Do a search on "favor composition over inheritance" for more on that.

  • The state of Cards doesn't change... the point of having a class is managing state. So I'd have something like pick_card that actually removes the selected card from the deck. Otherwise you'd probably be better off just using a function—at least in Python. Classes are all about bundling data and functions so you can manage state safely. Absent that, they kind of lose their luster (unless you're writing Java ;) ).

  • This is just me, but I wouldn't have StartGame run the loop or decide the winner. Again, I'd have __init__ start the game, and then have something like run_game. Again, rely on the class to manage the state. Each method is supposed to move it farther along.

  • Knowing me, I'd break out __init__, play_a_turn, decide_game, and show_game as methods and loop outside the class to keep the calling structure flat.

  • IMHO your logic for deciding the winner, and the logic for displaying the winner could go in separate methods. I like keeping methods small, light, and targeted.

  • Again, I'd make SimpleWar.__init__ do more, and StartGame do less. If I had a StartGame at all.

EDIT: I like to define my classes closely to the things they model. So instead of Cards, I'd probably have Deck, with methods shuffle and pick. I also like the comment above about spinning off a separate method to compare cards—I'd definitely do that.

share|improve this answer
    
It's not Python 3, because it uses Python 2's print syntax. –  nyuszika7h Feb 19 at 14:43
    
ah, good call. I'm still self-conscious about not having migrated. In that case, make sure that your init methods invoke the base class's init, either directly or with super() –  Rob Y Feb 19 at 16:00
    
For future reference, use `__init__` to make it display properly. Or if you want to bold it, **\_\_init\_\_**. –  nyuszika7h Feb 20 at 17:36
add comment

At a glance, I would recommend putting a CompareTo(OtherCard) function in the Card class, and using that for DecideWinner.

share|improve this answer
1  
The value of a card is game dependent. In a world, where those cards could be used for many different games, the cards wouldn't know their own values. So, I think, the WarGame object is a good place, to put this decision in. –  stofl Feb 19 at 15:05
    
@stofl The only thing that would change for numerical comparison is whether aces are high or low, and it actually doesn't matter at all unless aces could be either within the same game, or if cards are passed between games for some reason. –  Saposhiente Feb 19 at 18:50
1  
If the value of a card is an attribute of the card, then a compare method makes sense in the card class. If the value of a card is defined in the game rules, then the card class would be not the right place for a compare method. In the concrete code, we have the value in the cards - in real world we most often don't: Cards have many different values in different games. –  stofl Feb 19 at 19:05
    
@stofl I think that what's in the code is more relevant than what's in real life; and in the code, there is only one game, and only one definition of card comparison. –  Saposhiente Feb 19 at 19:30
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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