4 ms·

Source Code Makeover: Square Shooter, Part 1

52 points by yen223 14y ago

Eduardo3rd 14y ago
I'm just a non-computer scientist who mainly codes in Python and Matlab, but a lot of these suggestions (making a number a vairable if it is used more than once, etc) seem like normal coding practices to me. Do very many coding tutorials leave out this sort of thing?
- yen223 14y ago
  The tips written there should (hopefully) be fairly obvious to seasoned programmers, I agree. However, I find that this is particularly helpful in that it based on an actual working code base, instead of contrived examples. It demonstrates that writing clean code isn't just a pointless academic exercise, there is a purpose to it.
- timwiseman 14y ago
  Yes, most of them are normal coding practices. But ways to write better code generally are or at least should be normal coding practices. But many people do not know what the normal practices should be.
- stonemetal 14y ago
  The lowest quality of code I have ever seen has been tutorial code. In my experience(of reading various tutorials) it is uniformly badly written. There is never any error detection, basic things like setting up constants are relatively rare, Logging and debugability is never considered.
jsprink_banned 14y ago
Why the author made the code worse, commit by commit: 1. Harmful and degrades the readability of the code. Inline constants like that are just as bad as global variables, and there's now a giant refactor danger in the code from people (a) reimplementing this constant and (b) not updating the original one, if it becomes used in more than one place. Neither solution is great, here, but this is game data that should not be expressed in code; it should, ideally, be in some kind of asset. Additionally, this is a judgment call, but I feel that adding whitespace to line up the color constants violates the spirit of PEP 8, if not the letter: http://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements http://www.python.org/dev/peps/pep-0008/#whitespace-in-expre... 2. Magic numbers are bad. Littering the file with SHOUTING constants is worse. As with the powerups from item 1, move this to a configuration of some kind rather than expressing it in code. 3. Docstring immediately violates PEP 8 by being 83 characters long (for a total of 89 including quotes). http://www.python.org/dev/peps/pep-0008/#maximum-line-length http://www.python.org/dev/peps/pep-0008/#maximum-line-length Additionally, the docstring regurgitates what the code does without explanation, which is entirely pointless. Docstrings must explain what I cannot infer from reading the declaration and implementation. At least it mentioned the return type, which is good. Do not refactor someone else's code and change their indentation style because it fits your preference. That's just being argumentative, and unproductive. There's a reason that PEP 8 says for new projects in this guidance: http://www.python.org/dev/peps/pep-0008/#tabs-or-spaces http://www.python.org/dev/peps/pep-0008/#tabs-or-spaces 4. Multiple PEP 8 violations as in item 3. http://www.python.org/dev/peps/pep-0008/#maximum-line-length http://www.python.org/dev/peps/pep-0008/#maximum-line-length 5. Direct assault on readability that has no purpose. The original mistake was shorthanding 'pos'. Your code should tell me exactly what is going on, and the author admitting "it means either position or positive" is a clue that this is the wrong tactic. 6. Fine. 7. Fine. 8. Fine. 9. The addition of the conditional does not make the call simpler, and the author has now introduced two calls to random() instead of one. Although the original implementation of random_position() can certainly be improved, this change makes it worse in potentially performance-important code. (Clearly it isn't, but better to make things more efficient than less. A conditional and two random() calls is far less efficient than some math.) 10. Fine. 11. Now we're into blatant violations of the whitespacing from PEP 8 I alluded to earlier: http://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements http://www.python.org/dev/peps/pep-0008/#whitespace-in-expre... We're also expressing configuration as code again, and instantiating a dictionary to express that configuration every time the factory is called. 12. --amend your commits. There's absolutely no excuse for pushing a busted Git commit message aside from not knowing how to use your tools. Line 130 through 135 contain newlines for absolutely no reason; I don't understand why these were necessary. There are string comparisons in this commit for the kind of an object; string comparisons to describe something programmatic are the least-efficient way to reflect program state. The author is going back on something he chastised the original author for earlier, and using 0.03 directly in code AS A MAGIC CONSTANT which he said never to do (line 162)! 13. More PEP 8 violations. The author then documented his magic constant on line 164 because he realized it's unreadable, but didn't bother to fix it. I'm giving up at this point.
jarcoal 14y ago
Nice post, thanks. However, as long as we're in best practices mode, you may want to write those commit messages in present tense.
zalew 14y ago
> When there is nearly identical code, I like to make the spacing match up so it easier to compare them with each other. When you make a tutorial how to write better code in Python, you should follow, you know, Python conventions how to write code. http://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements http://www.python.org/dev/peps/pep-0008/#whitespace-in-expre... The same with those oneline conditionals, lack of spacing between functions, etc. Following PEP8 (with --ignore=E5,W602) is useful (if not for you, then for others who will commit to your code) and easy (configure your ide).
- eieio 14y ago
  I try very hard to follow PEP 8, but I have noticed some very nice python code(such as the setup portion of Peter Norvig's beautiful sudoku code http://norvig.com/sudoku.html http://norvig.com/sudoku.html) that alligns equals signs using more than one space. I try to use allignment when it makes the code easier to read, but if I'm unsure I will defer to PEP 8 and not allign.
  - zalew 14y ago
    if you care enough make a tutorial, you should care enough promote best practices, just sayin'. I ignored pep8 for a long time, until I noticed more and more whitespace mess in diffs. Following pep just makes work easier.
    - famousactress 14y ago
      Saying "best practices" makes my brain turn off. Call it 'dogma', it's more accurate because it lacks the subjective boolean people like to slip behind 'practices'. I get it. PEP-8 is rad, and pretty well accepted... but: 1. It's a blog post. It's subjective opinion. 2. Whitespace alignment is rad, and some of the PEP-8 whitespace rules are pretty unfortunate. I guess if it were me I'd at least have called out that the alignment violates PEP-8 as a warning to folks.. and I wouldn't have led the tutorial with it, so that I'd at least be dealing with trolls that make it through the entire post.
    - 14y ago
tomku 14y ago
I can't help but feel that the powerup_type change is short-sighted. In the new code, the enumeration of powerup types happens three times, in three different places. The first is in Powerup's constructor as a tuple of strings, the second is in Powerup.render as three paths of an if/else, and the third is in GameWorld.apply_powerup as the keys to a hash of functions. It feels wrong that GameWorld should keep track of a list of powerup types independently from instances of the Powerup class, and that Powerup instances have no knowledge of what they do. It smells suspiciously like using strings as a substitute for object types. To me, that hints that there are stronger refactorings to be found that could take advantage of polymorphism or a strategy pattern, depending on your taste for inheritance hierarchies.
ralph 14y ago
def wrap_around(self): """Change the position of the bubble to toroidally "wrap around" if it goes off one edge of the map.""" if self.pos.x < 0: self.pos.x += 1 if self.pos.y < 0: self.pos.y += 1 if self.pos.x > 1: self.pos.x -= 1 if self.pos.y > 1: self.pos.y -= 1 Why isn't else used? In addition to efficiency it tells the reader that the author expects at most one of x++ or x-- to happen.

52 points by yen223 14y ago

I'm just a non-computer scientist who mainly codes in Python and Matlab, but a lot of these suggestions (making a number a vairable if it is used more than once, etc) seem like normal coding practices to me. Do very many coding tutorials leave out this sort of thing?

yen223 14y ago
The tips written there should (hopefully) be fairly obvious to seasoned programmers, I agree. However, I find that this is particularly helpful in that it based on an actual working code base, instead of contrived examples. It demonstrates that writing clean code isn't just a pointless academic exercise, there is a purpose to it.
timwiseman 14y ago
Yes, most of them are normal coding practices. But ways to write better code generally are or at least should be normal coding practices. But many people do not know what the normal practices should be.
stonemetal 14y ago
The lowest quality of code I have ever seen has been tutorial code. In my experience(of reading various tutorials) it is uniformly badly written. There is never any error detection, basic things like setting up constants are relatively rare, Logging and debugability is never considered.

jsprink_banned 14y ago

Why the author made the code worse, commit by commit: 1. Harmful and degrades the readability of the code. Inline constants like that are just as bad as global variables, and there's now a giant refactor danger in the code from people (a) reimplementing this constant and (b) not updating the original one, if it becomes used in more than one place. Neither solution is great, here, but this is game data that should not be expressed in code; it should, ideally, be in some kind of asset. Additionally, this is a judgment call, but I feel that adding whitespace to line up the color constants violates the spirit of PEP 8, if not the letter: http://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements http://www.python.org/dev/peps/pep-0008/#whitespace-in-expre... 2. Magic numbers are bad. Littering the file with SHOUTING constants is worse. As with the powerups from item 1, move this to a configuration of some kind rather than expressing it in code. 3. Docstring immediately violates PEP 8 by being 83 characters long (for a total of 89 including quotes). http://www.python.org/dev/peps/pep-0008/#maximum-line-length http://www.python.org/dev/peps/pep-0008/#maximum-line-length Additionally, the docstring regurgitates what the code does without explanation, which is entirely pointless. Docstrings must explain what I cannot infer from reading the declaration and implementation. At least it mentioned the return type, which is good. Do not refactor someone else's code and change their indentation style because it fits your preference. That's just being argumentative, and unproductive. There's a reason that PEP 8 says for new projects in this guidance: http://www.python.org/dev/peps/pep-0008/#tabs-or-spaces http://www.python.org/dev/peps/pep-0008/#tabs-or-spaces 4. Multiple PEP 8 violations as in item 3. http://www.python.org/dev/peps/pep-0008/#maximum-line-length http://www.python.org/dev/peps/pep-0008/#maximum-line-length 5. Direct assault on readability that has no purpose. The original mistake was shorthanding 'pos'. Your code should tell me exactly what is going on, and the author admitting "it means either position or positive" is a clue that this is the wrong tactic. 6. Fine. 7. Fine. 8. Fine. 9. The addition of the conditional does not make the call simpler, and the author has now introduced two calls to random() instead of one. Although the original implementation of random_position() can certainly be improved, this change makes it worse in potentially performance-important code. (Clearly it isn't, but better to make things more efficient than less. A conditional and two random() calls is far less efficient than some math.) 10. Fine. 11. Now we're into blatant violations of the whitespacing from PEP 8 I alluded to earlier: http://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements http://www.python.org/dev/peps/pep-0008/#whitespace-in-expre... We're also expressing configuration as code again, and instantiating a dictionary to express that configuration every time the factory is called. 12. --amend your commits. There's absolutely no excuse for pushing a busted Git commit message aside from not knowing how to use your tools. Line 130 through 135 contain newlines for absolutely no reason; I don't understand why these were necessary. There are string comparisons in this commit for the kind of an object; string comparisons to describe something programmatic are the least-efficient way to reflect program state. The author is going back on something he chastised the original author for earlier, and using 0.03 directly in code AS A MAGIC CONSTANT which he said never to do (line 162)! 13. More PEP 8 violations. The author then documented his magic constant on line 164 because he realized it's unreadable, but didn't bother to fix it. I'm giving up at this point.

jarcoal 14y ago

Nice post, thanks. However, as long as we're in best practices mode, you may want to write those commit messages in present tense.

zalew 14y ago

> When there is nearly identical code, I like to make the spacing match up so it easier to compare them with each other. When you make a tutorial how to write better code in Python, you should follow, you know, Python conventions how to write code. http://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements http://www.python.org/dev/peps/pep-0008/#whitespace-in-expre... The same with those oneline conditionals, lack of spacing between functions, etc. Following PEP8 (with --ignore=E5,W602) is useful (if not for you, then for others who will commit to your code) and easy (configure your ide).

eieio 14y ago
I try very hard to follow PEP 8, but I have noticed some very nice python code(such as the setup portion of Peter Norvig's beautiful sudoku code http://norvig.com/sudoku.html http://norvig.com/sudoku.html) that alligns equals signs using more than one space. I try to use allignment when it makes the code easier to read, but if I'm unsure I will defer to PEP 8 and not allign.
- zalew 14y ago
  if you care enough make a tutorial, you should care enough promote best practices, just sayin'. I ignored pep8 for a long time, until I noticed more and more whitespace mess in diffs. Following pep just makes work easier.
  - famousactress 14y ago
    Saying "best practices" makes my brain turn off. Call it 'dogma', it's more accurate because it lacks the subjective boolean people like to slip behind 'practices'. I get it. PEP-8 is rad, and pretty well accepted... but: 1. It's a blog post. It's subjective opinion. 2. Whitespace alignment is rad, and some of the PEP-8 whitespace rules are pretty unfortunate. I guess if it were me I'd at least have called out that the alignment violates PEP-8 as a warning to folks.. and I wouldn't have led the tutorial with it, so that I'd at least be dealing with trolls that make it through the entire post.
  - 14y ago

tomku 14y ago

I can't help but feel that the powerup_type change is short-sighted. In the new code, the enumeration of powerup types happens three times, in three different places. The first is in Powerup's constructor as a tuple of strings, the second is in Powerup.render as three paths of an if/else, and the third is in GameWorld.apply_powerup as the keys to a hash of functions. It feels wrong that GameWorld should keep track of a list of powerup types independently from instances of the Powerup class, and that Powerup instances have no knowledge of what they do. It smells suspiciously like using strings as a substitute for object types. To me, that hints that there are stronger refactorings to be found that could take advantage of polymorphism or a strategy pattern, depending on your taste for inheritance hierarchies.

ralph 14y ago

def wrap_around(self): """Change the position of the bubble to toroidally "wrap around" if it goes off one edge of the map.""" if self.pos.x < 0: self.pos.x += 1 if self.pos.y < 0: self.pos.y += 1 if self.pos.x > 1: self.pos.x -= 1 if self.pos.y > 1: self.pos.y -= 1 Why isn't else used? In addition to efficiency it tells the reader that the author expects at most one of x++ or x-- to happen.