[Tutor] Criticism / Suggestions

[Kent Johnson]

Bill Kranec wrote:
> Hello,
> 
> So I think that I've 'completed' my first real Python program, and I 
> would appreciate any constructive criticism you all could offer.  The 
> program deals with a question that my Dad asked me awhile ago, which was 
> "If twelve people want to divide into teams of two and play (golf) 
> against each other, can a series of rounds be constructed such that each 
> player is teammates with each other player only once, 

do you mean, 'each  player is teammates with each other player exactly once', or, 'each  player is 
teammates with each other player at most once'? Is there a certain number of rounds that must be played?


Comments in no particular order
- Why do you have the initialization code in a comment? Wouldn't it be helpful to put that in a 
function or maybe Tournament.__init__()?

- I have a hard time figuring out how this is supposed to work. Maybe some comments about the 
algorithm you are using would help? I have no idea what self.key is doing.

- What is the argument to Tournament.__init__() for?

- checkTeammates() and checkOpponents() should probably be part of Tournament unless they are useful 
elsewhere.

- You might want to use sets to represent your teams and lists of teams. Sets have a built-in 
intersect method that should be faster than your hand coded one. In Python 2.4 you could create 
pairings as
pairings = [set(number1, number2) for number1 in range(1,13) for number2 in range(1,13) if number1 < 
number2]

or you could eliminate the conditional by tweaking the ranges:
pairings = [(number1, number2) for number1 in range(1,12) for number2 in range(number1+1,13)]

- This code repeats twice, maybe it should be in a method with a name that explains what it does:
         self.rounds = [ roundlist[ entry ] for entry in args ]
         self.flattened = [ entry for list in self.rounds for entry in list ]

- The above code depends on roundlist which is not even defined anywhere in the module (just shown 
in the comment). This suggests again that the init code from the comment should be part of 
Tournament, and roundlist should be an attribute of Tournament. For a class to depend on some 
external variable is bad design and will break if the client code is in a different module from the 
class code (roundlist will be in a different namespace than Tournament).

- Tournament.next() doesn't return a value, it prints it directly. It should return self.key or str( 
self.key )

- Your loop
for key in tment:
     key

relies on the interactive shell to print out key, it won't print anything if run from a program.
for key in tment:
     print key

would be better.

OK, enough for now, I hope this is helpful and doesn't come across as too critical :-)
Kent