Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.python > #20488 > unrolled thread

Wanted: Criticism of code for a Python module, plus a Mac tester

Back to article view | Back to comp.lang.python

Contents

  Wanted: Criticism of code for a Python module, plus a Mac tester HoneyMonster <someone@someplace.invalid> - 2012-02-15 23:33 +0000
    Re: Wanted: Criticism of code for a Python module, plus a Mac tester Ian Kelly <ian.g.kelly@gmail.com> - 2012-02-15 17:07 -0700
      Re: Wanted: Criticism of code for a Python module, plus a Mac tester HoneyMonster <someone@someplace.invalid> - 2012-02-16 01:11 +0000
        Re: Wanted: Criticism of code for a Python module, plus a Mac tester Ian Kelly <ian.g.kelly@gmail.com> - 2012-02-15 22:03 -0700
        Re: Wanted: Criticism of code for a Python module, plus a Mac tester Arnaud Delobelle <arnodel@gmail.com> - 2012-02-16 07:59 +0000
    Re: Wanted: Criticism of code for a Python module, plus a Mac tester Tim Chase <python.list@tim.thechases.com> - 2012-02-15 18:43 -0600

#20488 — Wanted: Criticism of code for a Python module, plus a Mac tester

I am quite new to Python (running Python 2.7 on Linux).

I have written a very small and simple dealing module for the game of 
Bridge. For those unfamiliar with the game, the idea is to deal each of 4 
players a hand of 13 cards from a pack of 52, and to display it thus (use 
a fixed pitch font):

Board 1
Neither vulnerable
Dealer North

        K98432       
        T7           
        2            
        AQT2         

T               AQJ          
KJ854           9632         
KQ973           T854         
85              74           

        765          
        AQ           
        AJ6          
        KJ963        


Firstly, is there anyone here who uses Python on a Mac and would be 
prepared to test it? I have tested it on Linux and Windows, but don't 
have access to a Mac.

Secondly, as a more general point I would welcome comments on code 
quality, adherence to standards and so forth. The code is at:

http://dl.dropbox.com/u/6106778/bridgedeal.py

Thanks.

[toc] | [next] | [standalone]

#20492

On Wed, Feb 15, 2012 at 4:33 PM, HoneyMonster <someone@someplace.invalid> wrote:
> Secondly, as a more general point I would welcome comments on code
> quality, adherence to standards and so forth. The code is at:

Looks pretty nice overall.  To reduce repetition, I would have
constructed the CONDITIONS list by iteration like you did the DECK
list, something like:

DEALERS = ["Dealer North", "Dealer East", "Dealer South", "Dealer West"]
VULNERABILITIES = [
    "Neither vulnerable", "North-South vulnerable",
    "East-West vulnerable", "Both vulnerable",
]
CONDITIONS = [(d, v) for d in DEALERS for v in VULNERABILITIES]

You could also eliminate some repetition in the deal method by putting
the suit lists in a local dict:

        suits = {'S': self.spades, 'H': self.hearts,
                 'D': self.diamonds, 'C': self.clubs}
        for card in cards:
            suits[DECK[card][SUIT]].append(DECK[card][RANK])

Another comment I had at first was that instead of creating the pack
list, which is just a list of indexes into DECK, you could shuffle and
deal the DECK list directly.  But then I realized that the indirection
allows you to easily sort the cards in the desired order, so that
should be left alone.

Cheers,
Ian

[toc] | [prev] | [next] | [standalone]

#20495

On Wed, 15 Feb 2012 17:07:48 -0700, Ian Kelly wrote:

> On Wed, Feb 15, 2012 at 4:33 PM, HoneyMonster
> <someone@someplace.invalid> wrote:
>> Secondly, as a more general point I would welcome comments on code
>> quality, adherence to standards and so forth. The code is at:
> 
> Looks pretty nice overall.  To reduce repetition, I would have
> constructed the CONDITIONS list by iteration like you did the DECK list,
> something like:
> 
> DEALERS = ["Dealer North", "Dealer East", "Dealer South", "Dealer West"]
> VULNERABILITIES = [
>     "Neither vulnerable", "North-South vulnerable",
>     "East-West vulnerable", "Both vulnerable",
> ]
> CONDITIONS = [(d, v) for d in DEALERS for v in VULNERABILITIES]
> 
> You could also eliminate some repetition in the deal method by putting
> the suit lists in a local dict:
> 
>         suits = {'S': self.spades, 'H': self.hearts,
>                  'D': self.diamonds, 'C': self.clubs}
>         for card in cards:
>             suits[DECK[card][SUIT]].append(DECK[card][RANK])
> 
> Another comment I had at first was that instead of creating the pack
> list, which is just a list of indexes into DECK, you could shuffle and
> deal the DECK list directly.  But then I realized that the indirection
> allows you to easily sort the cards in the desired order, so that should
> be left alone.

Thank you very much indeed, Ian.

Your second suggestion has opened my eyes; it had not even occurred to me 
that the "value" element of a dictionary entry could be a list. Just 
shows me how much I have to learn! Isn't Python great?

As to your first suggestion though, I am having some difficulty. Note 
that the vulnerability rotates; i.e. CONDITIONS[4] is not the same as 
CONDITIONS[0].
Is there a better way of doing it than a simple list.append()?

Thanks again.

[toc] | [prev] | [next] | [standalone]

#20502

On Wed, Feb 15, 2012 at 6:11 PM, HoneyMonster <someone@someplace.invalid> wrote:
> As to your first suggestion though, I am having some difficulty. Note
> that the vulnerability rotates; i.e. CONDITIONS[4] is not the same as
> CONDITIONS[0].
> Is there a better way of doing it than a simple list.append()?

Ah, it's more complicated than I realized.  I believe this generates
the correct result, although adding None to the dealers list is
somewhat unsatisfactory:

DEALERS = ["Dealer North", "Dealer East", "Dealer South", "Dealer West", None]
VULNERABILITIES = [
   "Neither vulnerable", "North-South vulnerable",
   "East-West vulnerable", "Both vulnerable",
]
CONDITIONS = [(d, v) for (d, v) in zip(DEALERS * 4, VULNERABILITIES * 5) if d]

I might be tempted to write a custom iterator for it, something like this:

def rotating(iterable):
    cache = collections.deque()
    for value in iterable:
        yield value
        cache.append(value)
    while True:
        cache.rotate(-1)
        for value in cache:
            yield value

# Using the original 4-length DEALERS list
CONDITIONS = list(itertools.islice(itertools.izip(itertools.cycle(DEALERS),
                                rotating(VULNERABILITIES)), 16))

But I don't know that it's really worth the added complexity.

Cheers,
Ian

[toc] | [prev] | [next] | [standalone]

#20504

On 16 February 2012 05:03, Ian Kelly <ian.g.kelly@gmail.com> wrote:
> On Wed, Feb 15, 2012 at 6:11 PM, HoneyMonster <someone@someplace.invalid> wrote:
>> As to your first suggestion though, I am having some difficulty. Note
>> that the vulnerability rotates; i.e. CONDITIONS[4] is not the same as
>> CONDITIONS[0].
>> Is there a better way of doing it than a simple list.append()?
>
> Ah, it's more complicated than I realized.  I believe this generates
> the correct result, although adding None to the dealers list is
> somewhat unsatisfactory:
>
> DEALERS = ["Dealer North", "Dealer East", "Dealer South", "Dealer West", None]
> VULNERABILITIES = [
>   "Neither vulnerable", "North-South vulnerable",
>   "East-West vulnerable", "Both vulnerable",
> ]
> CONDITIONS = [(d, v) for (d, v) in zip(DEALERS * 4, VULNERABILITIES * 5) if d]

You could also do:

DEALERS = ["Dealer North", "Dealer East", "Dealer South", "Dealer West"]
VULNERABILITIES = [
  "Neither vulnerable", "North-South vulnerable",
  "East-West vulnerable", "Both vulnerable",
]
CONDITIONS = [
    (DEALERS[j], VULNERABILITIES[(i + j)%4])
    for i in range(4) for j in range(4)
]

If you don't care about the order in which the conditions are listed,
you could use

CONDITIONS = itertools.product(DEALERS, VULNERABILITIES)

(But maybe you do, I haven't looked at the code)

-- 
Arnaud

[toc] | [prev] | [next] | [standalone]

#20496

On 02/15/12 17:33, HoneyMonster wrote:
> Firstly, is there anyone here who uses Python on a Mac and
> would be prepared to test it? I have tested it on Linux and
> Windows, but don't have access to a Mac.

It works from my quick test of it on my Mac.  The "class 
Player():" and the .format() calls choke on 2.4 (if perhaps for 
some reason you're running it on earlier versions), but 
otherwise, it should be good if you're running 2.7 everywhere.

> Secondly, as a more general point I would welcome comments on
> code quality, adherence to standards and so forth. The code is
> at:


All the .append()s seem a little unpythonic to me.  I'd just set 
it up with

   CONDITIONS = [
     [...],
     ...
     ]

And since you're not modifying it, I'd even use tuples (or named 
tuples):

   CONDITIONS = (
     ("Dealer North", "Neither vulnerable"),
     ...
     )

I'd also take advantage of iterable-unpacking to do something 
like the following in your Player.deal() method:

   for card in cards:
     suit, rank = DECK[card]
     {
      'S': self.spades,
      'D': self.diamonds,
      'C': self.clubs,
      'H': self.hearts,
     }[suit].append(rank)

(that fixed dictionary might even be hoisted out for reuse 
elsewhere).

Additionally, if you import this as a module rather than running 
it directly, there's no "north", "south", ... in your module 
namespace (they're only defined in the "if __name__ ..." section) 
so it will fail.

There are some other structural decisions that I might reconsider 
too:

- just shuffling the deck, rather than shuffling indexes into 
that deck

- there seems to be a lot of redundancy in the drawing code, but 
I'm not sure how I'd reduce that

- assigning to internal rank-named lists seems a little redundant 
to me.  In my (non-bridge) card-playing history, and tinkering 
with small programs like this to simulate card-games, I usually 
just give a player the cards and then leave the sifting/sorting 
to the display algorithm

- I see Ian's email came in as I was typing this and agree with 
his method of defining CONDITIONS with the caveat that it doesn't 
keep the same order as yours (I seem to recall bridge had some 
odd rules about that order)


-tkc

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.python

csiph-web