Code review

[Jean-Michel Pichavant]

----- Original Message -----
> From: "C Smith" <illusiontechniques at gmail.com>
> To: python-list at python.org
> Sent: Tuesday, 4 November, 2014 4:28:33 PM
> Subject: Code review
> 
> I was wondering if I could get some feedback on the biggest thing I
> have done as an amateur Python coder. The sidepots algorithm isn't
> correct yet, but I haven't worked on it in a while and thought I
> would
> get some advice before diving back in.
> 
> import random, os
> ##################################################################
> class Table(object):
>     def __init__(self,bigblind=20,PLAYERNUM=0,pot=0,PLAYERORDER=None,
> hand_done=0,\
>     left_to_act=None,cost_to_play=0,in_hand=None,last_raise=0,cards_in_play=None,round='preflop'):
>         if cards_in_play is None:
>             cards_in_play = []
>         if in_hand is None:
>             in_hand = []
>         if PLAYERORDER is None:
>             PLAYERORDER = []
>         if left_to_act is None:
>             left_to_act = []

[snip hundreds of code lines]

  - Most methods have too much code, split methods into more unitary logical functions.
     - For instance, add classes 'Hand' and 'Card'
  - not enough comments
  - have you written some tests? The code reached a reasonable size where tests would be very helpful (module unittest)
  - why do you use uppercase attributes ? like PLAYERORDER. These are not 'constants', you change them


Example of (incomplete) Card  and Hand implementation (python 2.7):

class Card(object):
  """Card implementation."""
  def __init__(self, suit, rank):
    self._suit = suit
    self._rank = rank

  # write test card1 == card2
  def __eq__(self, other):
    return (self.suit, self.rank) == (other.suit, other.rank)

  # use cards as dict keys
  def __hash__(self):
    return hash(self.suit, self.rank)

  # will allow python to sort a list of cards
  def __lt__(self, other):
    return int(self) < int(other)

  def __int__(self):
    """Return the numerical value of a card"""
    # the dictionary is incomplete
    return {'1':1,'2':2,'J':11,'Q':12}[self.rank]

  def __str__(self):
    return '<Card(%s of %s)>' % (self.rank, self.suit)

  # alias in case card.rank has more meaning than int(card)
  @property
  def rank(self):
    return int(self)

  # read only access to suit
  @property
  def suit(self):
    return self._suit


class Hand(object):
  """Short incomplete example of Hand class"""
   def __init__(self):
      self._cards = []

   def add(self, card):
     if not self.full:
        self._cards.append(card)
        if card.rank == 14: # it's an Ace
           # trick to get Ace considered as 1 as well
           self._cards.append(Card(card.suite, '1'))

   # generate the ordered sequence of cards, hiding the '1' cards
   @property
   def cards(self):
     return (card for card in sorted(self._cards) if card.rank > 1)

   # allow to write len(hand)
   def __len__(self):
     return len(list(self.cards))
  
  #property
  def full(self):
    return len(self) == 5

  # allow to write 'for card in hand:'
  def __iter__(self):
     return iter(self.cards)
          
  def remove(self, card):
     # and so on...


Otherwise:

replace 
  if left_to_act is None:
    left_to_act = []
  self.left_to_act = left_to_act
by
  self.left_to_act = left_to_act or []

replace
  if ranky == 9 or ranky == 5
by
  if ranky in [9,5]

replace 
  if foo == True
by
  if foo

replace
  if len(self.left_to_act) == 0
by 
  if self.left_to_act

And much more... but honestly, there's too much code :)
I'll let you chew on this one.

JM


-- IMPORTANT NOTICE: 

The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.