5
\$\begingroup\$

I have spent a few hours on this code, but think it could be improved a bit. It requires eSpeak to be installed to run the speech synthesis, but the voice can be toggled on or off using the togglevoice command. This was created for Python 2.7.3.

My major concerns are the fluctuations of the use of global to define global variables, the re-use of the variable n in functions and readability of the d array. It currently works fine on a Raspberry Pi running the latest version of Raspbian.

import os
def talk(n):
    if voice_mode == 1:
        os.system("espeak -ven+m3 '" + n + "' 2>/dev/null")
    else:
        print n

def print_score(n):
    if n == 0:
        other_n = 1
    else:
        other_n = 0
    if d[n][1] == 0:
        score1 = "love"
    else:
        score1 = str(d[n][1])
    if d[other_n][1] == 0:
        score2 = "love"
    else:
        score2 = str(d[other_n][1])
    talk(score1 + " " + score2)

def win(n):
    global first_server
    talk(d[n][0] + " wins the game")
    d[n][2] += 1
    d[0][1] = 0
    d[1][1] = 0
    deuce_mode = 0
    serve_turns = old_serve_turns
    if first_server == 0:
        first_server = 1
        current_server = 1
    else:
        first_server = 0
        current_server = 0
    if d[n][2] == match_points:
        talk(d[n][0] + " wins the match")
        d[0][2] = 0
        d[1][2] = 0

def add_score(n):
    global deuce_mode
    global current_server
    global serve_turns
    d[n][1] += 1
    if n == 0:
        other_n = 1
    else:
        other_n = 0
    if deuce_mode == 0:
        if d[0][1] == d[1][1] and d[0][1] == game_points - 1:
            deuce_mode = 1
            old_serve_turns = serve_turns
            serve_turns = 1
        if current_server == 0:
            print_score(0)
        else:
            print_score(1)
        if d[n][1] == game_points:
            win(n)
    else:
        if d[n][1] == d[other_n][1] + 2:
            if current_server == 0:
                print_score(0)
            else:
                print_score(1)
            win(n)
        else:
            if current_server == 0:
                print_score(0)
            else:
                print_score(1)
    if (d[0][1] + d[1][1]) % serve_turns == 0:
        if current_server == 0:
            current_server = 1
            talk(d[1][0] + " is serving next")
        else:
            current_server = 0
            talk(d[0][0] + " is serving next")

d = []
for i in xrange(2):
    d.append(["", 0, 0])
d[0][0] = "Player 1"
d[1][0] = "Player 2"
game_points = 11
match_points = 2
deuce_mode = 0
voice_mode = 1
serve_turns = 2
old_serve_turns = 2
current_server = 2
first_server = 0

talk("Ready to play")
while True:
    c = raw_input("Command: ").lower()
    if c == "z":
        if current_server != 2:
            add_score(0)
        else:
            talk(d[0][0] + " is serving first")
            current_server = 0
            first_server = 0
    elif c == "x":
        if current_server != 2:
            add_score(1)
        else:
            talk(d[1][0] + " is serving first")
            current_server = 1
            first_server = 1
    elif c == "1name":
        d[0][0] = raw_input("Player 1 name: ")
        talk("Welcome, " + d[0][0])
    elif c == "2name":
        d[1][0] = raw_input("Player 2 name: ")
        talk("Welcome, " + d[1][0])
    elif c == "gm":
        game_points = int(raw_input("Game points: "))
        match_points = int(raw_input("Match points: "))
    elif c == "s":
        serve_turns = int(raw_input("Serve turns: "))
    elif c == "togglevoice":
        if voice_mode == 0:
            voice_mode = 1
        else:
            voice_mode = 0
    elif c == "quit":
        quit()
    else:
        talk("Command not recognised")
\$\endgroup\$
2
\$\begingroup\$

Line by line analysis

import os

You need 2 blank lines before each global function definition.

def talk(n):

n is not descriptive and associated with a number. Don't use one letter variables unless it's really obvious (for example i loop counter, k and v as dict key and a value, or d as Twisted's deferred object. Something like phrase would be better.

    if voice_mode == 1:

Where does the voice_mode come from? Global variables are bad, pas it as a parameter to a talk method. Also, canonical way of checking if boolean is True is simply if voice_mode.

        os.system("espeak -ven+m3 '" + n + "' 2>/dev/null")
    else:
        print n

def print_score(n):

Again, n does not tell reader a lot.

    if n == 0:
        other_n = 1

What is other_n? If there's no better name for it - add a comment.

    else:
        other_n = 0
    if d[n][1] == 0:

Where did d come from? Pass it to a function. Also, better name.

        score1 = "love"
    else:
        score1 = str(d[n][1])
    if d[other_n][1] == 0:
        score2 = "love"
    else:
        score2 = str(d[other_n][1])
    talk(score1 + " " + score2)

def win(n):

Same thing with the n variable. Also, docstring might be helpful for this method.

    global first_server

Global variables are bad, really bad. Make your method take and return this if you need the variable.

    talk(d[n][0] + " wins the game")

This d is really quite obscure.

    d[n][2] += 1
    d[0][1] = 0
    d[1][1] = 0
    deuce_mode = 0
    serve_turns = old_serve_turns
    if first_server == 0:
        first_server = 1
        current_server = 1
    else:
        first_server = 0
        current_server = 0
    if d[n][2] == match_points:
        talk(d[n][0] + " wins the match")
        d[0][2] = 0
        d[1][2] = 0

def add_score(n):

Same thing: add docstring, replace n with better name and pass all the global variables as arguments to the function.

    global deuce_mode
    global current_server
    global serve_turns
    d[n][1] += 1
    if n == 0:
        other_n = 1
    else:
        other_n = 0
    if deuce_mode == 0:
        if d[0][1] == d[1][1] and d[0][1] == game_points - 1:
            deuce_mode = 1
            old_serve_turns = serve_turns

I don't see old_serve_turns used anywhere.

            serve_turns = 1
        if current_server == 0:
            print_score(0)
        else:
            print_score(1)
        if d[n][1] == game_points:
            win(n)
    else:
        if d[n][1] == d[other_n][1] + 2:
            if current_server == 0:
                print_score(0)
            else:
                print_score(1)
            win(n)
        else:
            if current_server == 0:
                print_score(0)
            else:
                print_score(1)
    if (d[0][1] + d[1][1]) % serve_turns == 0:
        if current_server == 0:
            current_server = 1
            talk(d[1][0] + " is serving next")
        else:
            current_server = 0
            talk(d[0][0] + " is serving next")

d = []

You need either a better data structure, or at least a good comment explained what the d is. Also, d needs a better name.

for i in xrange(2):
    d.append(["", 0, 0])
d[0][0] = "Player 1"
d[1][0] = "Player 2"
game_points = 11
match_points = 2
deuce_mode = 0
voice_mode = 1
serve_turns = 2
old_serve_turns = 2
current_server = 2

You only play with server 0 and 1, correct? Why do you need to obscure and add server 2 which is only there to start the game? Better have a flag of some sort.

first_server = 0

talk("Ready to play")
while True:
    c = raw_input("Command: ").lower()

Better name: command.

    if c == "z":
        if current_server != 2:
            add_score(0)
        else:
            talk(d[0][0] + " is serving first")
            current_server = 0
            first_server = 0
    elif c == "x":
        if current_server != 2:
            add_score(1)
        else:
            talk(d[1][0] + " is serving first")
            current_server = 1
            first_server = 1
    elif c == "1name":
        d[0][0] = raw_input("Player 1 name: ")
        talk("Welcome, " + d[0][0])
    elif c == "2name":
        d[1][0] = raw_input("Player 2 name: ")
        talk("Welcome, " + d[1][0])
    elif c == "gm":
        game_points = int(raw_input("Game points: "))
        match_points = int(raw_input("Match points: "))
    elif c == "s":
        serve_turns = int(raw_input("Serve turns: "))
    elif c == "togglevoice":
        if voice_mode == 0:
            voice_mode = 1
        else:
            voice_mode = 0
    elif c == "quit":
        quit()
    else:
        talk("Command not recognised")

Even though this if/else tree is acceptable in your scope, you might want to look into Strategy pattern. This is further reading though.

Summary

Overall, your variable naming needs to be more readable. Don't use global variables, just pass the ones you need as arguments. For example, your talk function could be changed like this:

def talk(phrase, voice_mode=True):
    """Pronounce the phrase if the voice mode is on, otherwise print."""
    if voice_mode:
        os.system("espeak -ven+m3 '%s' 2>/dev/null" % phrase)
    else:
        print phrase

Don't forget to specify docstrings for non-trivial methods. Make them long if you need to. Use comments extensively to elaborate if variable name or a data structure is not descriptive enough.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Thanks! This was put together in a rush without much python experience. You really cleared some things up for me! The d array is for holding user data, such as d[0][0] holds Player 1's name and d[1][1] holds Player 2's score. \$\endgroup\$ – ntzm Apr 3 '14 at 7:15
  • \$\begingroup\$ @NatZimmermann Might be better to name it player_data and turn it into a dict: player_data = {'player1': {'name': 'Foo', 'score': 3}, 'player2': {'name': 'Bar', 'score': 8}}. This is more explicit and lets you easier add additional parameters to the structure (if you decide to keep track of players' height for example). \$\endgroup\$ – Ruslan Osipov Apr 3 '14 at 15:13
  • \$\begingroup\$ Ok thanks, I'll get to changing all the stuff now. \$\endgroup\$ – ntzm Apr 3 '14 at 15:15
  • \$\begingroup\$ @NatZimmermann If you're interested - you can create another topic (don't adjust or edit this one) with your improved code and you might get higher level analysis (think design patterns). \$\endgroup\$ – Ruslan Osipov Apr 3 '14 at 15:57

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.