War Games whOOP

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
self.deck.extend(crd)
random.shuffle(self.deck)
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:
self.deck.remove(i)

player = cards()
enemy = cards()
while True:
raw_input()
if len(player.deck) == 0:
print 'rival won'
break
elif len(enemy.deck) == 0:
print 'you won'
break
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'
player.take(enemy.deck[0])
enemy.give(enemy.deck[0])
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'
player.take(enemy.deck[0:n])
enemy.give(enemy.deck[0:n])
break
elif player.deck[n] == enemy.deck[n]:
print 'cards are equal!another war round starting'
n += 3
else:
print 'rival won the war'
enemy.take(player.deck[0:n])
player.give(player.deck[0:n])
break

!!NO GLOBAL VARIABLES!!

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
        else:
            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):
    random.shuffle(self.cards)

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:

deck.shuffle()
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.

NO RELIANCE ON INTERNAL MEMBERS

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):
    self.cards.extend(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])
    else:
        repeat

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.

StackExchange Code Review Q#104816, answer score: 5