Darth Kaboda wrote:
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
You're certainly welcome. Sorry I blew it on extend(). I should have
known better; not just remembered. Python is pretty consistent - when a
method modifies its self-data, it generally doesn't return it as well.
For example sort() returns None, while sorted() returns the new list.
As for pop(-1): it'll always take from the end of the list, rather than
the beginning for pop(0). That's faster, but the result ends up
backwards. So you call reverse() on the result to get it back to where
you began. I hope I pointed out that it's a minor optimization for
small lists, and that it might obscure clarity.
However, the notion that -1 refers to the end of a list or string is a
useful one to learn, for both indexing and slicing.
From some perspectives, a module is analogous to a class for which all
methods are class methods (java static). And that in turn is very
similar to a class for which only one instance will ever be created. As
your class currently stands, the only reason I can imagine needing a
second instance is if you were operating on more than one deck
simultaneously, and wanted random() to be repeatable between runs, so
you would be separately maintaining the random seed for each deck.
However, you're quite right that other methods and instance attributes
might make it necessary to be a class.
DaveA
_______________________________________________
Tutor maillist - Tutor@python.org
http://mail.python.org/mailman/listinfo/tutor
