Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Here is my code for a sprinting game in Python. You have to tap the 'a' and 'd' keys as fast as you can to run 100 meters. Any improvements?

import msvcrt
import time
high_score = 50
name = "no-one"
while True:
    distance = int(0)
    print("\n--------------------------------------------------------------")
    print('\n\nWelcome to the 100m sprint, tap a and d rapidly to move!')    
    print('* = 10m')
    print("\n**Current record: " + str(high_score) + "s, by: " + name)
    print('\nPress enter to start')
    input()
    print('Ready...')
    time.sleep(1)
    print('GO!')
start_time = time.time()
while distance < 100:           
    k1 = msvcrt.getch().decode('ASCII')
    if k1 == 'a':
        k2 = msvcrt.getch().decode('ASCII')
        if k2 == 'd':
            distance += 1
            if distance == 50:
                print("* You're halfway there!")
                elif distance % 10 == 0:
                    print('*')
fin_time = time.time() - start_time
fin_time = round(fin_time,2)
print('Well done you did it in...'+str(fin_time))
if fin_time < high_score:
    print("Well done you've got a new high score ")
    name = input("Please enter your name : ")
share|improve this question
2  
Why distance = int(0)? It's no different to distance = 0. – SuperBiasedMan Feb 17 at 17:36
1  
Your code as-posted won't run (there's at least an IndentationError on the line elif distance % 10 == 0:. I'm sure that's just a copy/paste error, but note that the "el" in elif means you won't get a "*" if distance == 50 (and you probably want one!) – Adam Smith Feb 17 at 18:11
1  
@D.C oh I see -- you special-case the "*" on distance==50. My mistake! :) – Adam Smith Feb 17 at 18:14
up vote 15 down vote accepted

Your code is very straightforward, which is good.


Use a conditional for printing values that don't exist yet

At the very top of your code, you are setting these two variables to default values:

high_score = 50
name = "no-one"

To me, it seems that your reason for this is so that, a few lines later when you are printing these values, you have some data to print.

However, since this is not actual data that means nothing for the user and you are using it as a placeholder, you can use a conditional when printing these messages:

if name == "":
    ... print high score ...

Then, you can set the default values to:

high_score = 0
name = ""

However, this would require later that you ensure that the user does not enter their username as "".


Store high score

Along with that above tip, what you could also try doing is storing the high score data. You don't need some file format. In fact, you could handle your own special type.

Here could be an example high score save/data file:

50
D.C.

where the first line has the completed time, and the second line has the user who achieved that time.

Then, in your script, you can simply read from the high score file.


Remove nesting

In your while distance < 100 loop, you have quite a few levels of nesting. While the code in there isn't too complicated, it still is a bit harder on the eye.

To improve readability, try to keep everything on the same level (without breaking indentation rules):

while distance < 100:           
    k = msvcrt.getch().decode('ASCII')
    if k != 'a':
        continue

    k = msvcrt.getch().decode('ASCII')
    if k != 'd':
        continue

    distance += 1
    if distance == 50:
        print("* You're halfway there!")
    elif distance % 10 == 0:
        print('*')

Note: I merged the two k1 and k2 variables into one since the k1 one is no longer needed when k2 is being checked.


share|improve this answer
1  
Your reworked while loop doesn't handle the "d" fail case very well. Try running it and inputting "a"` then "f". You'll need to start over with "a" instead of correcting to "d". – Adam Smith Feb 17 at 18:20
1  
"I merged the two k1 and k2 variables into one since the k1 one is needed when k2 is being checked." You mean "no longer needed"? – jpmc26 Feb 17 at 18:28
1  
@jpmc26 Yup, fixing now. – SirPython Feb 17 at 22:50
1  
@AdamSmith I may be wrong, but I believe my code still does exactly what the OP's original code does. Say, for example, the user doesn't enter d for their second key press; the second conditional would fail, and then program execution would hit the bottom of the loop and go back up to the top. – SirPython Feb 17 at 22:54
1  
@SirPython you're totally right! I was stuck in my own head about how it should work and didn't read his code accurately. – Adam Smith Feb 17 at 23:32

If you didn't know, your code is not cross platform. This is as msvcrt is MS Windows only. To allow use on Linux and OSX you should use sys, tty and termios, as shown in this SO answer.

Doing this will allow more users to use your program.

share|improve this answer
    
oh thank you, I am a bit confused though. – D.C Feb 17 at 16:21
3  
@D.C No problem, what are you confused about? – Joe Wallis Feb 17 at 16:28
    
looking at your comment, I'm not sure what I have to do to my code – D.C Feb 17 at 16:31
1  
@D.C I changed the formatting, to hopefully improve clarity. Mostly just reimplement the SO answer. – Joe Wallis Feb 17 at 16:38

Refactor for clarity

Some things you're doing are complicated, and get repeated a lot! Let's handle "wait for a letter" and "move" separately, so our event loop looks like:

wait_for("a")
wait_for("d")
move()

This should be pretty simple:

def wait_for(ch):
    """Wait forever until the keyboard hears `ch`"""
    while True:
        if msvcrt.getch().decode('ASCII') == ch:
            return

def move():
    global distance  # SEE NOTE BELOW
    distance += 1
    if distance % 10 == 0:
        print("*", end="")
    if distance == 50:
        print(" You're halfway there!")
    else:
        print()  # just a newline

Note that I'm using a global now! "But Adam," I hear you cry, "I've heard that globals are evil, second only to Beelzebub and GOTO in being considered harmful!"

This is true! But we also want to be able to modify this variable with a function. Hmmm, maybe this could benefit from encapsulation! Classes to the rescue!

Encapsulate

Wrapping your game in a class will let you give it attributes that its methods can modify! This will let us do things like factor out mutating functions into methods of the class Game.

class Game(object):
    def __init__(self):
        distance = 0
        self.highscore = tuple()  # start with no high score
        # high scores will be a tuple of form
        # (name, time)

    @staticmethod
    def wait_for(ch):
        while True:
            if msvcrt.getch().decode("ASCII") == ch:
                return

    def move(self):
        self.distance += 1
        if self.distance % 10 == 0:
            print("*", end="")
        if distance == 50:
            print(" You're halfway there!")
        else:
            print()

Now your move method modifies its instance's attribute named distance, instead of some unknowable global variable. This is much better. Let's go further:

# inside class Game:

    @staticmethod
    def print_header():
        print("\n--------------------------------------------------------------")
        print('\n\nWelcome to the 100m sprint, tap a and d rapidly to move!')    
        print('* = 10m')
        if self.highscore:
            # use implicit string concatenation here to break at a logical point
            print("\n**Current record: " + str(self.highscore[1]) + "s "
                  "by: " + self.highscore[0])
        print('\nPress enter to start')
        input()

    def run(self):
        """Handle event loop"""

        start = datetime.datetime.now()  # SEE NOTE BELOW
        while self.distance < 100:
            self.wait_for("a")
            self.wait_for("d")
            self.move()
        end = datetime.datetime.now()
        d_time = (end - start).total_seconds()
        if not self.highscore or d_time < self.highscore[1]:
            # if the high score isn't set, or if this score beats it:
            name = input("...")
            self.highscore = (name, d_time)  # round as desired

    def start(self):
        """Starts one run of the game"""
        self.distance = 0  # reset to zero
        self.print_header()
        self.run()

There's a lot to go through here, but most of it makes sense if you think of things as responsible for ONE thing.

wait_for => Halts execution until its character gets detected.
move => Handles advancing self.distance and displaying feedback to user.
print_header => Prints the greeting screen. Really could be rolled into start.
run => Handles all the "during game" logic. Commonly called the event loop.
start => Begins ONE run of the game (prints headers and calls event loop)

I did want to point out that I changed the time.time() call to datetime.datetime.now(). time.time will be very confused if you start playing a game at 23:59:57 and it takes you more than 3 seconds to finish. datetime.timedelta can handle that elegantly. The difference between two datetime objects is a timedelta, which has the method total_seconds. We use that here to get the race time.

Let's run this puppy

Now that we're using a class, we need to instantiate it and make some calls so it will work. Luckily this is easy and idiomatic with an if __name__ == "__main__" clause.

# at the bottom of the file, under your class definition
if __name__ == "__main__":
    game = Game()  # instantiate the class
    while True:
        game.start()  # run the game once
        if input("Play again? (y/n) ").lower() == "y":
            continue  # play again, including printing headers with new high score!
        else:
            break  # quit

Since game is now an instance of the class Game, its state persists between game starts. In this case the only mutable state should be the high score.


As pointed out by jpmc26, having distance be an instance variable is silly since it's only involved in the actual run of a game and has no meaning between runs. That actually simplifies the code a bit, because it lets us move wait_for and move inside run as nested functions.

class Game(object):
    def __init__(self):
        self.highscore = tuple()

    def run(self):
        def wait_for(ch):
            # copy the whole function from above
        def move():
            distance += 1
            if distance % 10 == 0:
                # etc of this function as above, just dropping the "self"s
        distance = 0
        start = datetime.datetime.now()
        while distance < 100:
            wait_for("a")
            wait_for("d")
            move()
        # ... etc as above

In fact, in reading this again, it's probably easier to drop the distance variable completely and simply run the wait loop 100 times.

class Game(object):
    DISTANCE = 100
    def run(self):
        def wait_for(ch):
            # as above
        # no more move()
        start = datetime.datetime.now()
        for _ in range(self.DISTANCE):
            wait_for("a")
            wait_for("d")
            # do your print stuff in here
        # etc as above
share|improve this answer
1  
Very nice point about refactoring, and that's a good idea to bring the game into a class. – SirPython Feb 17 at 22:56
1  
start is a poor name for the method if it actually runs a play of the game to completion. I find your choice of abstraction unintuitive, as well. The Game object tracks the high scores, which can only happen between plays of the game, but the Game object itself is not responsible for any of the behavior between plays of the game. The choice of making distance an instance variable is even more confusing to me. This is not something that's useful across plays, so leaving it lying around increases the likelihood of bugs trying to manage it. It would be better to scope it within a play. – jpmc26 Feb 17 at 23:20
1  
@jpmc26 true! You could refactor move to be a function inside start which would let you local-scope distance! That's probably a better idea. I like start in this case because it mirrors the way threading.Thread works -- a start method that you call externally that manages some porcelain while executing its run method. – Adam Smith Feb 17 at 23:30
1  
Thread.start returns immediately, though. It doesn't wait for the thread to finish. That's kind of my point. The name makes you think the method returns immediately when it doesn't. – jpmc26 Feb 17 at 23:34

Isn't it best to start the high score at 0?

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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