3 ms·

Could you elaborate a little about his misuse of OO and functions? I ask as a CS student whose code doesn't get read very often. I want to be able to spot this

Could you elaborate a little about his misuse of OO and functions? I ask as a CS student whose code doesn't get read very often. I want to be able to spot this sort of thing but don't see much wrong with his method of enqueuing function calls. However, two things do jump out at me as improvements: * Enqueue the function object and arguments rather than a lambda of the function (his version not actually being a "great hack as the comments claim"). Or perhaps use functools.partial(). * CONFORM TO PEP8 FOR THE LOVE OF JOHN CLEESE

anthonyb 16y ago
In __update_pos, he has a series of if statements which look at the image filename to work out how far it's supposed to move[1]. This is the canonical warning sign that you should be using classes. The worst part is that he has created Ball classes - they just don't do very much: http://code.google.com/p/pygaf/source/browse/trunk/ball_object.py http://code.google.com/p/pygaf/source/browse/trunk/ball_obje... [1] Update: actually not - it's just working out the image rectangle. Which means that it's not updating 'pos' and that there's another 'update_pos' hiding somewhere non-obvious. A much cleaner way is to have something like, say, a Ball or Bullet class which knows how far it's supposed to go. Ditto for all of the canvas.create_foo classes in the second half - you should have objects which set themselves up, have update, draw and think methods and so on, inheriting from a common class. In terms of functions, you're pretty much on the money - there's a lot of function creation for very little gain in terms of readability or (at a guess) speed. You'd be better off either queuing dictionaries or strings as arguments for function generation, or else just using threads for your AI and having one method which handles all the drawing and updating within your environment. In addition to this, there are lots of places (__update_pos in particular) where he should be using functions (or methods) and doesn't - anywhere where there's repetition and/or common functionality, in this case updating the position of an image, really should have a function. Doubly so within large function or method bodies, like __new_population in http://code.google.com/p/pygaf/source/browse/trunk/optimization_ga.py http://code.google.com/p/pygaf/source/browse/trunk/optimizat... - "if blahblah:" followed by a huge chunk of code is just crying out to be a function.
- stevejohnson 16y ago
  Half of the problem was that I was looking at the wrong function (update_pos() vs __update_pos()), so all I saw was one line of code... Anyway, thanks for elaborating!

anthonyb 16y ago

In __update_pos, he has a series of if statements which look at the image filename to work out how far it's supposed to move[1]. This is the canonical warning sign that you should be using classes. The worst part is that he has created Ball classes - they just don't do very much: http://code.google.com/p/pygaf/source/browse/trunk/ball_object.py http://code.google.com/p/pygaf/source/browse/trunk/ball_obje... [1] Update: actually not - it's just working out the image rectangle. Which means that it's not updating 'pos' and that there's another 'update_pos' hiding somewhere non-obvious. A much cleaner way is to have something like, say, a Ball or Bullet class which knows how far it's supposed to go. Ditto for all of the canvas.create_foo classes in the second half - you should have objects which set themselves up, have update, draw and think methods and so on, inheriting from a common class. In terms of functions, you're pretty much on the money - there's a lot of function creation for very little gain in terms of readability or (at a guess) speed. You'd be better off either queuing dictionaries or strings as arguments for function generation, or else just using threads for your AI and having one method which handles all the drawing and updating within your environment. In addition to this, there are lots of places (__update_pos in particular) where he should be using functions (or methods) and doesn't - anywhere where there's repetition and/or common functionality, in this case updating the position of an image, really should have a function. Doubly so within large function or method bodies, like __new_population in http://code.google.com/p/pygaf/source/browse/trunk/optimization_ga.py http://code.google.com/p/pygaf/source/browse/trunk/optimizat... - "if blahblah:" followed by a huge chunk of code is just crying out to be a function.

stevejohnson 16y ago
Half of the problem was that I was looking at the wrong function (update_pos() vs __update_pos()), so all I saw was one line of code... Anyway, thanks for elaborating!