Dice gen and analyser script for RPGs: comments sought

[George Sakkis]

Richard Buckle wrote:

> Comments, insights and overall evaluations are especially welcomed re:
> * Cleanliness of design
> * Pythonicity of design
> * Pythonicity of code
> * Efficiency of code
> * Quality of docstrings
> * Conformance with modern docstring standards
> * Conformance with coding standards e.g. PEP 8
>
> I look forward to receiving your comments and criticisms.

Here are some random (and less random) comments in no particular order,
with minimal or no justification; if you're not sure why some point is
good advice, simply ask :) I did just a "shallow parsing", meaning I
didn't attempt to understand what it's actually doing, check for
correctness, algorithm optimizations or major refactorings.

* Instead of importing sets, have the following snippet to be able to
use the builtin set (available since 2.4):
try: set
except NameError: from sets import Set as set

* Prefer new-style classes unless you have a good reason not to. The
change is minimal; just replace "class Dice" with "class Dice(object)".

* Replace most occurences of dict.keys, dict.values, dict.items with
dict.iterkeys, dict.itervalues, dict.iteritems respectively (unless you
write code with Py3K in mind ;-)).

* Avoid mutable default function arguments unless necessary. Instead of
a default empty list, use either an empty tuple or None (in which case
you may assign an empty list in the function's body when the argument
is None).

* Raising LookupError when a sanity check of function arguments fails
looks strange. ValueError (or a a specialised subclass of it) would be
more appropriate. 'assert' statements should also not be used for
arguments checking because they may be turned off when running the
program with '-O'.

* reduce() is discouraged; it's almost always more readable to unroll
it in a for loop.

* Are all the methods public, i.e. useful to a client of the classes ?
If not, the convention is to start the non-public methods' name with a
single underscore (or double underscore sometimes, but name mangling
often causes more problems than it solves in my experience, especially
when inheritance is involved).

* Several snippets can be condensed with list comprehensions, though
one-liners are not to everyone's taste. E.g. I'd write
	histograms = []
	maxDice = 10
	for i in xrange(maxDice):
		dice = StoryTellerDice(i)
		histogram = dice.bruteforce()
		histograms.append(histogram)
as histograms = [StoryTellerDice(i).bruteforce() for i in xrange(10)]

* [personal preference]: Don't leave space between *every* operator in
expressions, group them based on precedence. E.g. instead of "(n *
sigmaSq - sigma * sigma) / (n * n)", I read it easier as "(n*sigmaSq -
sigma*sigma) / (n*n).


HTH,
George