I am a beginner level programmer and recently I started learning OOP, so I made a simple war game

import random
cardsstart = 4 * range(2, 15)
cardsstart.extend([15, 15]);random.shuffle(cardsstart)
cardslen = len(cardsstart)/2
class cards:
    def __init__(self):
        """intializes a set of cards(deck) to the object
        global cards
        self.deck = [cardsstart.pop() for i in range(cardslen)]
    def take(self, crd):
        """appends the card given to the object's list deck"""
        crd = [crd] if type(crd) is not list else crd
    def give(self, crd):
        """removes the card given from the object's list deck"""
        crd = [crd] if type(crd) is not list else crd
        for i in crd:

player = cards()
enemy = cards()
while True:
    if len(player.deck) == 0:
        print 'rival won'
    elif len(enemy.deck) == 0:
        print 'you won'
    print 'player shown his', player.deck[0]
    print 'rival shown his', enemy.deck[0]
    if player.deck[0] > enemy.deck[0]:
        print 'player took rival\'s card'
    elif player.deck[0] == enemy.deck[0]:
        print 'WAR'
        n = 3
        while True:
            print 'the war is on. player ruling card is {} and rival ruling card is {}'.format(player.deck[n], enemy.deck[n])
            if player.deck[n] > enemy.deck[n]:
                print 'player won the war'
            elif player.deck[n] == enemy.deck[n]:
                print 'cards are equal!another war round starting'
                n += 3
                print 'rival won the war'
        print 'rival took player\'s card'

Looking for advice on how to make it better? Any other tips?


1 Answer 1



That merits being capitalized. And underlined. And lots of exclamation points. Nothing can kill your ability to write good, modular quite quite like global variables. You have several of them, and it took me several reads to understand what exactly is going on here.

Let's simplify. What do we need for WAR? We need a Deck:

class Deck(object):

We can start with the default deck, this will be all of our cards:

class Deck(object):
    def __init__(self, cards=None):
        if cards is not None:
            self.cards = cards
            self.cards = range(13) * 4

Never write multiple lines of code on one line (your line with the ;) It'll be hard to even see that the second line exists! I also don't understand your extend([15,15]) line. Why are there 15s?

So anyway, with this deck, we have:

full_deck = Deck() # all the cards

Which we probably want to be able to shuffle:

def shuffle(self):

Now we need to split this in half for our two players. Let's just add a split() method:

def split(self):
    half = len(self.cards) // 2
    return Deck(self.cards[:half]), Deck(self.cards[half:])

With which we can do:

player, enemy = deck.split()

Note that we didn't need any global variables here! It's easy to see where this is coming from. We had a full deck of cards, we shuffled it, and split it in half.


In Python, everything is public. But that doesn't mean that you should rely on this. You are relying very explicitly on the members and their types. There had better be a .deck that's a list. Much better to hide the implementation behind an interface that just has a few key operations.

Which operations? Well, we need the pop off the top card:

def pop(self):
    return self.cards.pop(0)

And we need to push cards onto it:

def push(self, vals):

And we need to check for emptiness:

def empty(self):
    return not self.cards

And that's about it. Now our main loop just has to look like:

while True:
    if player.empty():
        # enemy won
    elif enemy.empty():
        # player won

    player_card = player.pop()
    enemy_card = enemy.pop()

    if player_card > enemy_card:
        player.push([player_card, enemy_card])
    elif enemy_card > player_card:
        enemy.push([player_card, enemy_card])

Avoiding the globals and the internal memory structure would let you do later improvements to the Deck internals. Right now we're popping off the top and pushing onto the back, that's not very efficient for list, but it's much better for deque. Maybe we change to that? Then we'd just have to change pop(0) to be popleft(), but none of the callers of our code will have to know that.

  • \$\begingroup\$ Good answer! But I'd suggest using // to find half in the deck split method, as len(self.cards) / 2 could produce a float if the length of the cards value ever changed. Or at least raise it as a clear error. \$\endgroup\$ Sep 16, 2015 at 15:27
  • \$\begingroup\$ Might have something to do with "explicit better than implicit " perhaps ;-) \$\endgroup\$
    – janos
    Sep 16, 2015 at 15:34

Your Answer

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

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