5
\$\begingroup\$

I am programming a graphical text adventure using python. The game is playable, but I am not sure whether the code is well-suited for newcomers (code smells / quirks / general readability).

Here's the main script:

import time
import sys

import printer
from rooms import *
from _player import Player

VER = 0.01

SCENES = {0 : Lobby(),
          1 : LivingRoom(),
          2 : Bathroom()}

player = Player()

def run(start_at=None):
    """Starts the game"""

    if not start_at:
        scene = SCENES[0]
    else:
        scene = SCENES[start_at]
    scene.init_player(player)
    while 1:
        new_scene = scene.run()
        if new_scene != None:
            scene = SCENES[new_scene]
            scene.init_player(player)

if __name__ == "__main__":
    run()
| improve this question | |
\$\endgroup\$
  • 5
    \$\begingroup\$ Consider breaking your code into smaller, digestible posts. Nobody is likely to review your entire project for free. Try with a single module, and include as much context/explanation as possible. I have an entire VBE add-in being peer reviewed on this site, spanning over 30 questions. Ask one, wait for feedback, improve your code base, then ask another. \$\endgroup\$ – Mathieu Guindon♦ Jun 9 '15 at 21:38
  • 1
    \$\begingroup\$ Relevant meta \$\endgroup\$ – Mathieu Guindon♦ Jun 9 '15 at 21:48
  • 1
    \$\begingroup\$ What you may and may not do after receiving answers \$\endgroup\$ – janos Jun 10 '15 at 17:33
  • \$\begingroup\$ phew, sorry... still learning how this works here. thx \$\endgroup\$ – Don Polettone Jun 10 '15 at 18:24
7
\$\begingroup\$

Avoid wildcard imports. Import from rooms just what you need, named explicitly.

while 1 is an unnatural way of writing while True

Don't use != or == with None. Use is not None and is None.

The scene.run call that returns another scene object is not very intuitive. Functions are best named after what they do and/or what they return. In this example "run" is a pretty vague term, it could mean anything. If it transitions the game state to the next scene, I think scene.next_scene would be more intuitive.

Do you really need SCENES ? How about just passing around scene instances? This approach using a global dictionary of index to instance mappings seems fishy.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for the useful answer. Let's work that through bit by bit: Wildcard imports. I still use them quite often... If I have an "objects.py" module containing 20 object classes, how can I make them accessible without having to type "from objects import bla, bli, blu, blo, foo, bar, ..." ? When that useless typing begins, I put a *, make sure there are no clashes and that's it... \$\endgroup\$ – Don Polettone Jun 9 '15 at 23:39
  • \$\begingroup\$ @DonPolettone from foo import bar.* implies that you are importing every class in bar, even those you don't need. It's better to instead import only what you need, and leave the rest alone (so they are not put in memory, and things like that). A lot of IDEs handle that for you, in some way or another, and save you a lot of typing. \$\endgroup\$ – Phrancis Jun 10 '15 at 3:03
  • \$\begingroup\$ I'd say running a scene is pretty intuitive \$\endgroup\$ – michaelsnowden Jun 10 '15 at 3:16
  • 1
    \$\begingroup\$ It looks like there will eventually be many rooms. I think a wildcard import is reasonable here, as long as there is only one wildcard import in this file. Listing everything explicitly just adds unnecessary verbosity. \$\endgroup\$ – 200_success Jun 10 '15 at 5:55
  • \$\begingroup\$ @michaelsnowden I agree that was a bit weak. I rewrote that paragraph \$\endgroup\$ – janos Jun 10 '15 at 6:14
6
\$\begingroup\$
  • You shouldn't store the version of your game in a variable named VER. Preferably, you should use __version__. At the top of your file, put this. You can also use __author__ to describe the project with your name as well.
__version__ = "0.01"
__author__ = "My name"
...
  • The function run should be renamed to main, just to stay consistent.
  • The docstring for run should have more info than just """Starts the game""". Preferably, it should be fleshed out with useful information about run, like the arguments, and what it does.
  • The way you've formatted your dictionaries is, odd. Dictionaries, list, and tuples can look like this. While your current style doesn't violate PEP8. I find this style easier to read.
SCENES = {
    0: Lobby(),
    1: LivingRoom(),
    2: Bathroom()
}
  • From looking at your code, you don't use time or sys anywhere in this file. Don't import modules if they aren't needed.
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ PEP 396: The module version should be available as a string through the __version__ attribute. \$\endgroup\$ – 200_success Jun 10 '15 at 5:57
  • \$\begingroup\$ The OP's dictionary layout is permitted by PEP-8 (except for the space before the colon, which you've retained). If you're going to give style tips, get them right! \$\endgroup\$ – jonrsharpe Jun 10 '15 at 7:32
  • \$\begingroup\$ @200_success Edited post. \$\endgroup\$ – Ethan Bierlein Jun 10 '15 at 15:11
  • \$\begingroup\$ @jonrsharpe Edited post. \$\endgroup\$ – Ethan Bierlein Jun 10 '15 at 15:11
  • \$\begingroup\$ __version__ = "v0.01" is redundant. Just write __version__ = "0.01". \$\endgroup\$ – 200_success Jun 10 '15 at 17:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.