Skip to content

Conversation

@guokevin
Copy link

No description provided.

@srli
Copy link

srli commented Apr 2, 2016

Review status: 0 of 30 files reviewed at latest revision, 12 unresolved discussions.


a discussion (no related file):
This is a super fun game, and I'm really impressed at how well it's been implemented. I've made a couple comments at places where things could be improved, please take a look below!

In the future, please remember to organize the files in your Git repo according to their datatype and function. i.e: all the .ogg files should go in a "media" folder or something of the like. Have a .gitignore so you aren't committing the .pyc files.

I've noticed that your conditional statements tend to get really long since you check against multiple statements at once. In the future, I'd like to challenge you guys to figure out how to structure your code so that those super long conditionals aren't necessary. They're fine as they are, but detract from the readability of your code.

All in all, nicely done. This was a great project, I'm looking forward to your future work!


Escape_The_Maze.py, line 12 [r1] (raw file):
You should put all this stuff under the if _ name _ = " _ main _ " statement


Escape_The_Maze.py, line 43 [r1] (raw file):
self.ticker isn't too self-explanatory. If "magic numbers" like 30 or 200 appear in your code, it's nice to have it documented


Escape_The_Maze.py, line 67 [r1] (raw file):
I'm loving all your in-line comments though!


Escape_The_Maze.py, line 223 [r1] (raw file):
Honestly, this game is so great


Escape_The_Maze.py, line 331 [r1] (raw file):
Another way to do this would be to directly update self.center inside the update_center method


Escape_The_Maze.py, line 352 [r1] (raw file):
Hmm, this code is repeated directly above. Perhaps it'd make more sense to move this into the model?


Escape_The_Maze.py, line 354 [r1] (raw file):
Since this is never used, remember to delete it


Escape_The_Maze.py, line 359 [r1] (raw file):
Hmm, I would hesitate to use such a generic name for a class, since "list" is a datatype as well. GameLists or something would be more descriptive


Escape_The_Maze.py, line 534 [r1] (raw file):
Try to avoid super long conditionals like this. While it does make it easier to implement, it's hard to read and easy to result in semantic mistakes down the road


Escape_The_Maze.py, line 721 [r1] (raw file):
Avoid capitalizing the first letter of methods/functions since those are usually reserved for Class names


Escape_The_Maze.py, line 944 [r1] (raw file):
You already did this :P


Comments from Reviewable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants