[Tutor] Review and criticism of python project
kent37 at tds.net
Sat Jan 5 03:29:37 CET 2008
> 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.
>> 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.
>> 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...
More information about the Tutor