[Tutor] Review and criticism of python project
keridee at jayco.net
Mon Jan 7 23:33:53 CET 2008
> Tiger12506 wrote:
>> Ouch. Usually in OOP, one never puts any user interaction into a class.
> That seems a bit strongly put to me. Generally user interaction should be
> separated from functional classes but you might have a class to help with
> command line interaction (e.g. the cmd module in the std lib) and GUI
> frameworks are usually built with classes.
Sorry... made that a little too general. I just have personal experiences
with putting a whole bunch of raw_inputs in a class once and learned never
to make that mistake again when I wanted to use the code for something else.
>>> uh, tf = line.split(',')
>>> if uh in self.uhdata:
>>> if tf not in f:
>> This can read
>> f = self.uhdata[uh]
>> except KeyError:
>> self.uhdata[uh] = 
> These are not equivalent - the original code avoids duplicates in
> Possibly self.uhdata[uh] could be a set instead of a list. Then this could
> be written very nicely using defaultdict:
> from collections import defaultdict
> self.uhdata = defaultdict(set)
>>> for uh, Sections in self.uhdata.items():
>> This will not work. In the documentation, it says that dictionary object
>> return a *copy* of the (key,value) pairs.
>> You are sorting those *copies*.
> No, the original code is fine. The docs say that dict.items() returns "a
> copy of a's list of (key, value) pairs". This is a little misleading,
> perhaps. A dict does not actually contain a list of key, value pairs, it
> is implemented with a hash table. dict.items() returns a new list
> containing the key, value pairs.
> But the keys and values in the new list are references to the same keys
> and values in the dict. So mutating the values in the returned list, via
> sort(), will also mutate the values in the dict.
Ah hah! Yes knew about hashes... It was the references that got me. I need
to learn to use id() at the python prompt a little more often. ;-)
>>> for x in self.uhdata:
>>> for f in self.uhdata[x]:
>>> if f not in fmissingpgcounts:
>> fmissingpgcounts = [f for f in self.uhdata.itervalues() if f not in
>> Ahhh... This looks like a set.
>> fmissingpgcounts = set(self.uhdata.itervalues())
> Again, this is not quite the same thing. The original code builds a set of
> the contents of the values. You are building a set from the values (which
> are already lists). You still need one loop:
> fmissingpgcounts = set()
> for v in self.uhdate.itervalues():
>> self.pgcounts = dict((x,0) for x in fmissingpgcounts)
> or, self.pgcounts = dict.fromkeys(fmissingpgcounts, 0)
> That's all I have energy for...
Hmm... Apparently I didn't have enough energy. I must remember to eat
breakfast more often, it's affecting how I think... You're completely right
of course, Kent! What would all of us do without you?
More information about the Tutor