[Tutor] Review and criticism of python project

[Kent Johnson]

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.


>>                uh, tf = line.split(',')
>>                if uh in self.uhdata:
>>                    f=self.uhdata[uh]
>>                    if tf not in f:
>>                        f.append(tf)
>>                else:
>>                    self.uhdata[uh]=[tf]
> 
> This can read
> 
> try:
>   f = self.uhdata[uh]
> except KeyError:
>   self.uhdata[uh] = []
> finally:
>   self.uhdata[uh].append(tf)

These are not equivalent - the original code avoids duplicates in 
self.uhdata[uh].

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)
   ...
   self.uhdata[uh].add(tf)

>>            for uh, Sections in self.uhdata.items():
>>                Sections.sort()
> 
> 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.


>>            missingpgcounts={}
>>            fmissingpgcounts=[]
>>            for x in self.uhdata:
>>                for f in self.uhdata[x]:
>>                    if f not in fmissingpgcounts:
>>                        fmissingpgcounts.append(f)
> 
> fmissingpgcounts = [f for f in self.uhdata.itervalues() if f not in 
> fmissingpgcounts]
> 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():
   fmissingpgcounts.update(v)

> self.pgcounts = dict((x,0) for x in fmissingpgcounts)

or, self.pgcounts = dict.fromkeys(fmissingpgcounts, 0)

That's all I have energy for...
Kent