critique my code, please

Scott David Daniels scott.daniels at acm.org
Mon Feb 6 13:48:06 EST 2006


Brian Blais asked for suggestions and critiques of his code.
For style, look at:
     http://www.python.org/peps/pep-0008.html

This is how I'd rewrite the first class:

     YESNO = ('No', 'Yes')

     class SimulationParams(object):
         '''Simulation Parameters setting up of a simulation'''
         def __init__(self):
             '''Build a new simulation control object'''
             self.epoch_number = 500
             self.iter_per_epoch = 500
             self.epoch_per_display = 1
             self.random_seed = 'clock'
             self.keep_every_epoch = 0
             self.save_input_vectors = 0

         def __repr__(self):
             return '''Epoch Number: %s
     Iter Per Epoch: %s
     Epoch Per Display: %s
     Random Seed: %s
     Keep Every Epoch: %s
     Save Input Vectors: %s
     '''         % (self.epoch_number, self.iter_per_epoch,
                    self.epoch_per_display, self.random_seed,
                    YESNO[self.keep_every_epoch],
                    YESNO[self.save_input_vectors])

Notes:
The docstrings I am using are too generic.  You know your app, so
do something more app-appropriate.

True is a version of 1, and False is a version of 0, so a dictionary
like {0:"No",1:"Yes",True:"Yes",False:"No"} is actually length 2.
I'd just use a constant mapping available anywhere.

'a %s b' % 13 is 'a 13 b', so no need for your seed type test.  If you
do want to make the type visually obvious, use %r rather than %s for
the seed.

Personally I'd choose shorter names, but this is more taste:
     class SimulationParams(object):
         '''Simulation Parameters setting up of a simulation'''
         def __init__(self):
             '''Build a new simulation control object'''
             self.epoch = 500
             self.iter_per_epoch = 500
             self.epoch_per_display = 1
             self.seed = 'clock'
             self.keep_epochs = False  # Since these seem to be booleans
             self.save_input = False  # Since these seem to be booleans
         ...

You also might consider making the __init__ provide defaulted args so
getting a different initial setup only requires your changing the setup:

         ...
         def __init__(self, epoch=500, iter_per_epoch=500,
                      epoch_per_display=1, seed='clock',
                      keep_epochs=False, save_input=False):
             '''Build a new simulation control object'''
             self.epoch = epoch
             self.iter_per_epoch = iter_per_epoch
             self.epoch_per_display = epoch_per_display
             self.seed = seed
             self.keep_epochs = keep_epochs
             self.save_input = save_input
         ...


More some other time.

--Scott David Daniels
scott.daniels at acm.org



More information about the Python-list mailing list