Python: 'object in list' checks and '__cmp__' overflow

Question

this is my first time at stack overflow so I'm sorry if the format doesn't fit quite right with the site. I just recently started learning programming, almost 2 weeks have passed since. I'm learning python from http://openbookproject.net/thinkcs/python/english3e/index.html and everything had been quite nice until now, where I just got stuck for hours. I googled a lot but couldn't find a proper solution to my problem so here I am.

I'm trying to get the OldMaidGame() run without problems as explained on CH17. http://openbookproject.net/thinkcs/python/english3e/ch17.html - Most of the code also comes from the previous chapter.

What I've found out is I can't get the Deck.remove, Hand.remove_matches, or any other kind of remove function to work. After some debugging I found out that the problem occurs when the program checks if the given card is present in the deck/hand/etc. It can't ever make a match. Then after some looking back on the chapter, (in ch16), I found out that 'if card in deck/hand/etc: remove(card)' etc looks up the .cmp() of the object to determine if the card actually exists in the deck/hand/etc. This is my version of the cmp after doing the additions for 'ace's on the given code from the e-book.

def __cmp__(self, other):
    """ Compares cards, returns 1 if greater, -1 if lesser, 0 if equal """
    # check the suits
    if self.suit > other.suit: return 1
    if self.suit < other.suit: return -1
    # suits are the same... check ranks
    # check for aces first.
    if self.rank == 1 and other.rank == 1: return 0
    if self.rank == 1 and other.rank != 1: return 1
    if self.rank != 1 and other.rank == 1: return -1
    # check for non-aces.
    if self.rank > other.rank: return 1
    if self.rank < other.rank: return -1
    # ranks are the same... it's a tie
    return 0

The cmp itself seems fine afaik, ofc I could use some tips on how to make it better (like with ace checks). So I have no idea why the card in deck/hand checks always return false. This was the given remove function:

class Deck:
    ...
    def remove(self, card):
        if card in self.cards:
            self.cards.remove(card)
            return True
        else:
            return False

Desperately trying to get it to work, I came up with this:

class Deck:
    ...
    def remove(self, card):
        """ Removes the card from the deck, returns true if successful """
        for lol in self.cards:
            if lol.__cmp__(card) == 0:
                self.cards.remove(lol)
                return True
        return False

Seemed to work fine, until I moved on to the other non-working remove functions:

class OldMaidHand(Hand):
    def remove_matches(self):
        count = 0
        original_cards = self.cards[:]
        for card in original_cards:
            match = Card(3 - card.suit, card.rank)
            if match in self.cards:
                self.cards.remove(card)
                self.cards.remove(match)
                print("Hand {0}: {1} matches {2}".format(self.name, card, match))
                count = count + 1
        return count

I again made some adjustments:

class OldMaidHand(Hand):
    def remove_matches(self):
        count = 0
        original_cards = self.cards[:]
        for card in original_cards:
            match = Card(3 - card.suit, card.rank)
            for lol in self.cards:
                if lol.__cmp__(match) == 0:
                    self.cards.remove(card)
                    self.cards.remove(match)
                    print("Hand {0}: {1} matches {2}".format(self.name, card, match))
                    count = count + 1
        return count

The removing worked fine for the card, but it would give an error (x not in list) when I tried to remove match. Another our or so, I might've been able to make that work too, but since it already feels like I'm on the wrong road since I can't fix the original 'card in deck/hand/etc' etc, I came here looking for some answers/tips.

Thanks for reading and I greatly appreciate any help you can give :)

--------------------- EDIT 1 *>

This is my current code: http://pastebin.com/g77Y4Tjr

--------------------- EDIT 2 *>

I've tried every single cmp advised here, and I still can't get it to find a card with 'in'.

>>> a = Card(0, 5)
>>> b = Card(0, 1)
>>> c = Card(3, 1)
>>> hand = Hand('Baris')
>>> hand.add(a)
>>> hand.add(b)
>>> hand.add(c)
>>> d = Card(3, 1)
>>> print(hand)
Hand Baris contains
5 of Clubs
 Ace of Clubs
  Ace of Spades
>>> d in hand.cards
False
>>> 

I've also tried the card.py @DSM has used successfully, and I get errors there too, like at the sort function it says it cant compare the two card objects.
So I was wondering, maybe it is a problem with Python 3.2, or maybe the syntax has changed somewhere?

Answer 1

"So I was wondering, maybe it is a problem with Python 3.2, or maybe the syntax has changed somewhere?"

Oh, you're running Python 3.2? This'll never work in Python 3: python 3 doesn't use __cmp__!

See the data model (look for __eq__). Also read the what's new in Python 3 for some other things it's way too easy to miss.

Sorry, this is on us Python programmers here; we should have caught this far earlier. Most of probably looked at all the code, realized without even thinking about it that the source was obviously python 2 code, and assumed that's what we were working with. The cmp function doesn't even exist in Python 3.2, but the reason that it doesn't blow up with a NameError is because __cmp__ is never called.

If I run the code in Python 3.2, I reproduce your problem exactly:

>>> c = Card(0,2)
>>> str(c)
'2 of Clubs'
>>> c in [c]
True
>>> c in Deck().cards
False

In Python 3, you either implement all the rich cmps or __eq__ and one of them and use a total_ordering decorator.

from functools import total_ordering

@total_ordering
class Card(object):
    """Represents a standard playing card."""
    suit_names = ["Clubs", "Diamonds", "Hearts", "Spades"]
    rank_names = [None, "Ace", "2", "3", "4", "5", "6", "7", 
              "8", "9", "10", "Jack", "Queen", "King"]
    def __init__(self, suit=0, rank=2):
        self.suit = suit
        self.rank = rank
    def __str__(self):
        return '%s of %s' % (Card.rank_names[self.rank],
                             Card.suit_names[self.suit])
    def __repr__(self): return str(self)
    def __lt__(self, other):
        t1 = self.suit, self.rank
        t2 = other.suit, other.rank
        return t1 < t2
    def __eq__(self, other):
        t1 = self.suit, self.rank
        t2 = other.suit, other.rank
        return t1 == t2


>>> c = Card(2,3)
>>> c
3 of Hearts
>>> c in Deck().cards
True

Answer 2

I also can't reproduce the error. It works fine for me. My only suggestion would be that you probably shouldn't be modifying a list while you're iterating over it (ie, calling self.cards.remove within a loop over self.cards). That can't explain why the versions using "in" wouldn't work for you.

Your cmp function can be written somewhat more succinctly (and IMHO more simply) as:

def __cmp__(self, other):
    """ Compares cards, returns 1 if greater, -1 if lesser, 0 if equal """
    return cmp((self.suit,  self.rank == 1,  self.rank),
              (other.suit, other.rank == 1, other.rank))

or if you prefer:

    return (cmp(self.suit, other.suit) or
            cmp(self.rank == 1, other.rank == 1) or
            cmp(self.rank, other.rank))

Answer 3

I can't seem to reproduce the problem with not being able to remove cards via Deck.remove. If I start with the card.py at the thinkpython site and add the remove function you've posted up there, it seems to work:

>>> deck = Deck()
>>> str(deck).split('\n')
['Ace of Clubs', '2 of Clubs', '3 of Clubs', '4 of Clubs', '5 of Clubs', '6 of Clubs', '7 of Clubs', '8 of Clubs', '9 of Clubs', '10 of Clubs', 'Jack of Clubs', 'Queen of Clubs', 'King of Clubs', 'Ace of Diamonds', '2 of Diamonds', '3 of Diamonds', '4 of Diamonds', '5 of Diamonds', '6 of Diamonds', '7 of Diamonds', '8 of Diamonds', '9 of Diamonds', '10 of Diamonds', 'Jack of Diamonds', 'Queen of Diamonds', 'King of Diamonds', 'Ace of Hearts', '2 of Hearts', '3 of Hearts', '4 of Hearts', '5 of Hearts', '6 of Hearts', '7 of Hearts', '8 of Hearts', '9 of Hearts', '10 of Hearts', 'Jack of Hearts', 'Queen of Hearts', 'King of Hearts', 'Ace of Spades', '2 of Spades', '3 of Spades', '4 of Spades', '5 of Spades', '6 of Spades', '7 of Spades', '8 of Spades', '9 of Spades', '10 of Spades', 'Jack of Spades', 'Queen of Spades', 'King of Spades']
>>> len(deck.cards)
52
>>> c = Card(suit=0, rank=8)
>>> str(c)
'8 of Clubs'
>>> c in deck.cards
True
>>> deck.remove(c)
True
>>> len(deck.cards)
51
>>> c in deck.cards
False
>>> str(deck).split('\n')
['Ace of Clubs', '2 of Clubs', '3 of Clubs', '4 of Clubs', '5 of Clubs', '6 of Clubs', '7 of Clubs', '9 of Clubs', '10 of Clubs', 'Jack of Clubs', 'Queen of Clubs', 'King of Clubs', 'Ace of Diamonds', '2 of Diamonds', '3 of Diamonds', '4 of Diamonds', '5 of Diamonds', '6 of Diamonds', '7 of Diamonds', '8 of Diamonds', '9 of Diamonds', '10 of Diamonds', 'Jack of Diamonds', 'Queen of Diamonds', 'King of Diamonds', 'Ace of Hearts', '2 of Hearts', '3 of Hearts', '4 of Hearts', '5 of Hearts', '6 of Hearts', '7 of Hearts', '8 of Hearts', '9 of Hearts', '10 of Hearts', 'Jack of Hearts', 'Queen of Hearts', 'King of Hearts', 'Ace of Spades', '2 of Spades', '3 of Spades', '4 of Spades', '5 of Spades', '6 of Spades', '7 of Spades', '8 of Spades', '9 of Spades', '10 of Spades', 'Jack of Spades', 'Queen of Spades', 'King of Spades']

It seems to work if I replace the __cmp__ by yours, too:

>>> deck = Deck()
>>> c = Card(suit=0,rank=1)
>>> c in deck.cards
True
>>> deck.remove(c)
True
>>> len(deck.cards)
51

So something must be different. Could you dump your entire code -- and some code demonstrating the bug -- somewhere (pastebin, gist, etc.)?

---

(FWIW, my ace-over-king cmp looks like this:

def __cmp__(self, other):
    def aceshigh(r): return 14 if r==1 else r
    t1 = self.suit, aceshigh(self.rank)
    t2 = other.suit, aceshigh(other.rank)
    return cmp(t1, t2)

Exercise: remove the magic numbers.)

Answer 4

It sounds like you've got a problem with your deck variable. Either the remove function is pointing to a different object with a blank deck, or else you have a namespace issue. Is the remove function part of the deck object?

I'd suggest adding a few print deck lines. One just after it is initialized, to see what it has inside it, and one just as you call remove.

Answer 5

Your comparison function should work, as other people have pointed out, need more detail to figure out what was going on there. As for suggestions:

1. Stick Ace at the end of ranks, use 0-12 to map ranks. This seems like the natural approach to me.

2. Take advantage of standard library:

   A. Use random.shuffle to shuffle.

   B. Use cmp to handle comparisons.

   C. collections.defaultdict makes for a cleaner remove_matches method in my opinion.

3. The suggested __str__ method is really, really annoying.

4. Implement __repr__.

Alternative implementation:

from collections import defaultdict
import random

class Card(object):
    suits = ["Clubs", "Diamonds", "Hearts", "Spades"]
    ranks = ["2", "3", "4", "5", "6", "7", "8", "9", "10", "Jack", "Queen", "King", "Ace"]

    def __init__(self, suit=0, rank=0):
        self.suit = suit
        self.rank = rank

    def __str__(self):
        return self.ranks[self.rank] + " of " + self.suits[self.suit]

    def __repr__(self):
        return self.__str__()

    def __cmp__(self, other):
        return cmp((self.suit, self.rank), (other.suit, other.rank))

class Deck(object):
    def __init__(self):
        self.cards = []
        for suit in range(4):
            for rank in range(13):
                self.cards.append(Card(suit=suit, rank=rank))

    def shuffle(self):
        random.shuffle(self.cards)

    def remove(self, card):
        if card in self.cards:
            self.cards.remove(card)

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

    def is_empty(self):
        if len(self.cards) is 0:
            return True
        return False

    def deal(self, hands, num_cards=999):
        num_hands = len(hands)
        for i in range(num_cards):
            if self.is_empty(): break   # break if out of cards
            card = self.pop()           # take the top card
            hand = hands[i % num_hands] # whose turn is next?
            hand.add(card)              # add the card to the hand

class Hand(Deck):
    def __init__(self, name=""):
       self.cards = []
       self.name = name

    def add(self,card):
        self.cards.append(card)

class OldMaidHand(Hand):
    def remove_matches(self):
        matches = defaultdict(list)
        for card in self.cards:
            matches[card.rank].append(card)
        for cards in matches.values():
            if len(cards) == 2:
                print("Hand {0}: {1} matches {2}".format(self.name, *cards))
                for card in cards:
                    self.remove(card)