[Tutor] First code snipet

[Dave Angel]

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