answers.py v0.0.1 - source

[Marc 'BlackJack' Rintsch]

In <87bqloz3fy.fsf at pyenos.pyenos.org>, Pyenos wrote:

> global ANSWER_PATH

``global`` at module level has no effect.  That keyword is used to declare
names in functions or methods global that would be local to the function
otherwise and it's only needed if you want to *assign* to the global name
from within a function.  As this seems to be a constant it might be even
an "error" to reassign the name.

> global VERSION
> VERSION=0.01

Version numbers aren't really numbers, so it's better to use strings here.

> class Problem:
>     def __init__(self):self.problem=""
>     def set(self,problem):self.problem=problem
>     def get(self):return self.problem

You like it compact right?  :-)

Take a look at the style guide: http://www.python.org/dev/peps/pep-0008/

Use more spaces.  Around binary operators and the ``=`` in assignments. 
And newlines after the colon that introduces a body of a method or branch
in an ``if``/``else`` statement and so on.

Next thing are getters and setters:  Don't use them it's just unnecessary
typing.  You don't loose the flexibility to decide to replace the simple
attribute lookup by a calculated access later because Python has
"properties".  Look at the docs of the `property()` function.

So the `Problem` can be simplified to:

class Problem(object):
    def __init__(self, problem):
        self.problem = problem

Maybe add docstrings to the classes and methods.

> class Storage:
>     def add(self,problem,solution):
>         self.answers[problem]=solution
>     def save(self):
>         import pickle
>         try: pickle.dump(self.answers,file(ANSWER_PATH+".answers","w"));print "Answers saved"
>         except: print "Couldn't save answers. Something went wrong"

Many people put ``import`` statements at the top of the module so it's
very easy to find the dependencies of the module.

Paths can be concatenated with `os.path.join()` in a platform independent
way and pickles should always be opened in binary mode.  Otherwise they
won't survive transfers between operating systems in all cases.

Using the ``;`` to put more than one statement on one line is even more
uncommon than putting something after the ``:``.  And the style guide
suggests not to use lines longer than 80 characters.

Don't use a bare ``except``!  This catches every exception.  If you
mistyped a name you get a `NameError` or `AttributeError` which will be
"replaced" by this rather meaningless text that's printed.  In more
complex programs such ``excepts`` can mask really hard to find bugs.

You don't close the file which is at least a bit unclean.

>     def load(self):
>         import pickle
>         try: self.answers=pickle.load(file(ANSWER_PATH+".answers"));print "Answers loaded"
>         except: print "Couldn't load answers. Something went wrong or this may be your first time running"

You might want to make load a `staticmethod` or a `classmethod` so you
don't need an instance to load the answers.  But this is a bit more
advanced stuff.

>     def __init__(self,answers={}):
>         self.answers=answers

Default arguments are only evaluated *once* when the ``def`` is executed,
so all your `Storage` classes share the same dictionary.  One idiom to
solve this is:

    def __init__(self, answers=None):
        self.answers = answers or dict()

I would expect the `__init__()` method to be the first method in a class
definition followed by other special double underscore methods.

Now comes a very strange and convoluted piece of code.  You are "misusing"
a class as namespace for other classes and nested classes with just one
method that doesn't even use the `self` argument.  That's an overly
complex way to write a function.

> class Console:
>     class Menu:
>         global storage
>         storage=Storage()

If you want `storage` to be at module level why do you hide the binding in
that nested class?

>         class Level0:
>             def do(self):
>                 import sys
>                 action=sys.exit()
>             
>         class Level1:
> 
>             def do(self):
>                 problem=Problem();solution=Solution()
>                 text=("Do you have a problem? $ ","Do you have the solution? $ ","no idea","Try again with your common sense")

Why do you put all the texts into tuples so later there's a not very
descriptive `text[x]` reference and one has to search the definition at
another place?

>                 commands=("answer","load(disabled, already
>                 loaded)","quit","help") problem.set(raw_input(text[0]))
> 
>                 if problem.get()==commands[0]:return 4 #elif
>                 problem.get()==commands[1]:storage.load();return 1 elif
>                 problem.get()==commands[2]:return 0 elif
>                 problem.get()==commands[3]:print commands;return 1

Here you are doing something that doesn't belong here IMHO.  You put
potential commands into a `Problem` object.  You are mixing the main menu
and the input of a problem/solution in one function.  The menu code would
fit better in the main loop.

Damned, I trimmed to much and my newsreader doesn't let me insert that
overlong lines without reformatting.  So without your code:  You are
storing the *text* of a `Solution` and a `Problem`, not the objects, so
why do you have those classes at all?

>         def start(self):
> 	    storage.load()
>             level=1
>             while 1:
>                 if level==0:
>                     Console.Menu.Level0().do()
>                 if level==1:
>                     level=Console.Menu.Level1().do()
>                 if level==2:
>                     level=Console.Menu.Level2().do()
>                 if level==3:
>                     level=Console.Menu.Level3().do()
>                 if level==4:
>                     level=Console.Menu.Level4().do()

Level numbers and the function names are quite dull.  It's really hard to
get an idea what happens here without reading all the code and taking
notes.

Such constructs can be written with a dictionary which maps level numbers
to functions.  In this case even a list would do.  Assuming the code is in
ordinary functions named `level0` to `level4`:

def dispatch():
   levels = [level0, level1, level2, level3, level4]
   next_level = 1
   while True:
       next_level = levels[next_level]()

Giving the numbers meaningful names helps readability.  Or even better
give the functions better names and get rid of the numbers by returning
the next to call function object directly.

Then the `dispatch()` is just:

def start_func():
    ...
    if some_condition:
        return spam_func
    else:
        return eggs_func

def spam_func():
    ...

...

def dispatch():
    func = start_func
    while True:
        func = func()

Ciao,
	Marc 'BlackJack' Rintsch