On Thu, Mar 26, 2015 at 6:12 AM, <humbed...@apache.org> wrote: >...
> +++ steve/trunk/pysteve/lib/plugins/ap.py Thu Mar 26 11:12:52 2015 > >... > + y = n = a = 0 > + by = bn = 0 > + for vote in votes.values(): > + if vote == 'y': > + y += 1 > + if vote == 'n': > + n += 1 > + if vote == 'a': > + a += 1 > + if vote == 'by': > + by += 1 > + if vote == 'bn': > + bn += 1 > "elif", please ... no need to run all tests for every vote. Yah yah, speed isn't an issue, but it also improves clarity, that these are disjoint. Could also throw in an 'else' and raise an exception if a vote doesn't match one of the legal values. >... > + > +def validateAP(vote, issue): > + "Tries to invalidate a vote, returns why if succeeded, None otherwise" > The name of the function is "validateAP" and the docstring says it tries to *invalidate* ?? These should match. >... > +constants.VOTE_TYPES += ( > Modifying a global in *another module* is all kinds of ugly. Totally breaks any kind of encapsulation of concerns. And what happens if this gets run twice? What if not all are imported? Does something force them all to be loaded? etc etc. This can also introduce big problems with references to constants.VOTE_TYPES that may exist at different times. Not only are you creating a system with a seeming global constant that is NOT, but it also takes on multiple values. If I do: k1 = constants.VOTE_TYPES import ap k2 = constants.VOTE_TYPES k1 and k2 will be *different*. You're rebinding the VOTE_TYPES name, rather than modifying it in place. So not only breaking encapsulation, but also creating a potential hazard of modules thinking they have "the latest" VOTE_TYPES, but merely have a snapshot of how it existed at a single point in time. >... Cheers, -g
