In <[EMAIL PROTECTED]>, 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 -- http://mail.python.org/mailman/listinfo/python-list
