> Good point. It is important to state your goal for the code, preferably
> in comments in your code, as well as in the message asking us the
> question. Suggest you include a doc-string for each class and method
> declaration.
>
> But it's important to tell us how you expect to use it, as well. If
> it's for your own learning, then it's pretty good as it is. But if you
> intend to show it to prospective employers, there are a lot of ways to
> clean it up.
>
>
> So...
>
> Unlike Java, not everything needs to be in a class. This shuffler class
> accomplishes nothing that the module doesn't already do. The only
> instance attribute you use is rgen, and that could just as readily have
> been a global (visible within the entire module).
>
> Naming is important. I think I would have factored it into three
> functions cut() and ripple(). shuffle() simply calls cut(), then ripple().
>
> You need to know the library better. Andreas already mentioned the std
> library random.shuffle(), and you correctly explained why you didn't use
> it. But there are other library functions that would make your code
> more concise, more readable, and maybe faster.
>
> Starting with shuffle() (which I'd call ripple())
>
> Because Python makes it easy to swap two variables in-place, it's common
> to swap arguments directly, rather than copy them to local attributes.
> So I'd replace the first 4 lines with:
> if self.rgen.randint(0,1): #randomly decide which pile is shuffled
> down first
> inpile2, inpile1 = inpile1, inpile2
>
> len() is very fast, so there's no need for a separate variable to keep
> track of the lengths of inpile1 and inpile2. So eliminate all refs to
> pile1c and pile2. In the while, just use
> while len(inpile1) and len(inpile2):
>
> Lists have no type associated with them. Each object in a list has its
> own type. So there's no meaning to constructing an empty list out of
> another one.
> rpile = []
>
> pop(0) is slow, compared with pop(-1). If you used the latter, you
> would have to do a reverse on the list when you were done to keep the
> realism. And that would make the code less readable. So I think this
> is an unnecessary optimization. But keep it in mind if you're ever
> dealing with thousands of items in a list. Peeling them off from the
> front one at a time will be slow.
>
> The logic of popping a random number of cards from one list, and
> appending them to the result could be done the same way you do the
> cut(), using slices. I think it would be worth it if you made a
> separate function out of it, to call from whichever deck was being
> operated on.
>
> Where you do the if i < pile1c you could instead use the min()
> function.
> i = min(len(inpile1), self.rgen.randint(1,4))
>
> At the end of the function, the if pile1c: elif ... logic could be
> replaced by
> rpile.extend(inpile1).extend(inpile2)
>
> as there's no harm in extending with an empty list.
>
> HTH, DaveA
>
Dave and Alan,
Thank you for the advice and showing me some of the small nuances I haven't
quite figured out yet. I used both of your suggestions to change the class the
updated code is attached if you'd like to see.
I tried putting the two extends in the same line and that didn't work as extend
doesn't return anything so ended up putting them on consecutive lines without
the if statement.
Instead of doing the pop method I took your advice and tried it with slices
instead along with extends. Shortened the code a bit. One note on pop method is
pop() and pop(-1) the same thing? Looks like it based on what I've read but not
100% confident in that answer.
On the class versus module I wasn't sure where I'd end up taking this idea so
went ahead and coded it as a class. If I actually do much more with this, other
then just as a coding exercise to help learn Python, I might be adding things
to this or using it as a superclass. Examples of expansion ability to queue up
decks, have multiple shufflers going at the same time in a program etc... So
without knowing all the options in Python figured the class gave me the
greatest flexibility. Yeah otherwise I'd have just made a module and still
might switch it if I expand on this depending on what the needs turn out to be.
Again thanks to everyone for looking at my code and the quick and helpful
feedback. Hoping to do start coding a larger project soon so you'll probably
get some questions from me.
Thanks,
Brian
import random
class shuffler(object):
def __init__(self):
self.rgen = random.Random()
def shuffle(self, inpile1, inpile2):
"""Shuffles two lists of items in the same manner as a real deck of
cards is shuffled. This function will not change the passed in variables of
piles."""
if self.rgen.randint(0,1): #randomly decide which pile is shuffled
down first
(pile1,pile2)=(inpile1,inpile2)
else:
(pile1,pile2)=(inpile2,inpile1)
rpile = pile1[0:0] #get blank copy of the passed in object types.
Should allow user defined list type objects to work as well
while pile1 and pile2: #while there are still items in both piles
execute following logic
i = min(len(pile1)+1,self.rgen.randint(1,4)) #to allow for human
error get random offset of items to shuffle and ensure we don't go out of
bounds for the pile
rpile.extend(pile1[:i])
pile1[:i]=[]
i = min(len(pile2)+1,self.rgen.randint(1,4)) #to allow for human
error get random offset of items to shuffle and ensure we don't go out of
bounds for the pile
rpile.extend(pile2[:i])
pile2[:i]=[]
#append any items left over
rpile.extend(pile1)
rpile.extend(pile2)
return rpile
def shuffledeck(self, deck):
"""Shuffle a list of items in the same manner as a real deck of cards
is shuffled. The function will not change the passed in variable of deck."""
i = len(deck) #get length of the deck object to shuffle
cut = (i // 2) + self.rgen.choice(range(-4,5)) #find half way point of
deck +/- human error factor
pile1 = deck[0:cut]
pile2 = deck[cut:]
return self.shuffle(pile1,pile2)
if __name__ == '__main__':
myshuffle = shuffler()
deck = range(1,53)
print deck
for i in range(500):
deck = myshuffle.shuffledeck(deck)
print deck
_______________________________________________
Tutor maillist - Tutor@python.org
http://mail.python.org/mailman/listinfo/tutor
