5
\$\begingroup\$

This is my first Python script, and I was hoping to get some feedback. I do have one specific question: is the use of global variables considered a bad practice? When I used them below, my programmer's instincts immediately went off as though I was creating a code-smell, mostly because it seems much less elegant than just having a private String myVariable at the top of a Java class. The use of global myVariable to force the scope seemed a bit hacky. But I don't know a single thing about Python except what I wrote below, so I could be way off.

I also realize that the below code is more complex than it needs to be. I know that it could be written in much fewer lines of code by removing function definitions and such (in fact, my first version was only like 15 lines). But I wrote this more complicated version in order to get a handle on some of the basic concepts like sharing variables, returning from functions, etc.

Any tips or glaring style/best-practices issues are appreciated! (I'm a style-Nazi with Java, so please, don't hold back so that I can learn!)

#!/usr/bin/python

from random import randint

# GLOBALS
guesses = 0
the_number = 0

def checkGuess(guess):        
    global the_number

    if guess <= 0:
        return False
    elif guess == the_number:
        return True
    elif guess < the_number:
        print "The number is HIGHER."
    elif guess > the_number:
        print "The number is LOWER."

    return False    

def isValid(guess):
    return len(guess) > 0 and guess.isdigit()        

def getInput():
    global guesses        
    input = ""

    while not isValid(input):
        input = raw_input("(#" + str(guesses) + ") Guess a number: ")
        if not isValid(input):
            print "Please guess an integer!"

    return int(input)           

def runGame(min, max):    
    global guesses
    global the_number

    the_number = randint(min, max)
    guess = 0

    print "I'm thinking of a number between " + str(min) + " and " + str(max) + " ..."

    while not checkGuess(guess):
        guesses = guesses + 1
        guess = getInput()

    print "YOU WON!"

def printTitle():
    print "----------------------"
    print "----- MASTERMIND -----"
    print "----------------------"
    print ""

def main():
    printTitle()
    runGame(1, 100)

main()
\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

Some notes:

  • Mutable global state is a bad programming practice. Instead, pass values as arguments so functions are black boxes that take values (as arguments) and return values. If such a function does not perform any side-effect (print to console, write a file, ...), then it's a "pure function". Try to write pure functions whenever possible.

  • Conditionals: Don't write a fallback (return False) where some branches get and others don't. Non-overlapping conditionals are more clear.

  • Use name_of_variable and name_of_function.

  • Try to use Python 3 whenever possible.

I'd write:

from random import randint
from itertools import count

def is_guess_correct(number, guess):
    if guess == number:
        return True
    elif guess < number:
        print("The number is HIGHER.")
        return False
    else:
        print("The number is LOWER.")
        return False

def is_valid_guess(number_string):
    return number_string.isdigit()

def get_number(guess_iteration):
    while 1:
        number_string = input("({0}) Guess a number: ".format(guess_iteration))
        if is_valid_guess(number_string):
            return int(number_string)
        else:
            print("Please enter a valid integer!")

def run_game(nmin, nmax):
    number = randint(nmin, nmax)
    print("I'm thinking of a number between {0} and {1}...".format(nmin, nmax))

    for guess_iteration in count(1):
        guess = get_number(guess_iteration)
        if is_guess_correct(number, guess):
            print("YOU WON!")
            break

if __name__ == '__main__':
    run_game(1, 100)
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for your feedback. What do you mean by "Try to use Python 3?" Which feature or design consideration were you referring to that looked like an earlier version? \$\endgroup\$
    – asteri
    Aug 25, 2013 at 13:27
  • \$\begingroup\$ print is not a statement anymore, it's a function, so I concluded you are using Python2. \$\endgroup\$
    – tokland
    Aug 25, 2013 at 13:34
  • \$\begingroup\$ Got it, thanks. One more question. I'm used to for... in... logic iterating over each element in a set. I just want to fully understand what's happening in for guess_iteration in count(1). I get that guess_iteration is being incremented each time the loop runs, but is count(1) kind of a hacky "while(true)" sort of thing? Or is the count() function specifically made for infinite loops that you have to break out of? \$\endgroup\$
    – asteri
    Aug 25, 2013 at 15:28
  • 1
    \$\begingroup\$ yes, with count you avoid the verbose while+counter (I wouldn't call a hack, I think it's way better because is more declarative). How it works: count(n) returns a iterator, check docs.python.org/2/library/itertools.html#itertools.count and stackoverflow.com/questions/19151/build-a-basic-python-iterator. Regarding the last question, count is a basic functional abstraction, an infinite lazy counter that comes handy in a lot of situations. \$\endgroup\$
    – tokland
    Aug 25, 2013 at 15:47
3
\$\begingroup\$

You can solve anything without globals. If not, you are doing wrong. If you are using globals, the debugging will be a nightmare. In the checkGuess() you can use single ifs instead of elif. The return will terminate the function and after the first 3 check, the last one can't be anything else than grater than the_number. If you want to check a variable not to be null/zero/empty/None, you can use simply the if var: condition. It will be False if var is null/zero/empty/None. In getInput() you can get the first value before the loop. If you get inside the loop and you use an additional if, then there will be 2 useless condition which slow down the app (I know only a little but...).

#!/usr/bin/python
from random import randint


def checkGuess(guess, the_number):
    if guess <= 0:
        return False
    if guess == the_number:
        return True
    if guess < the_number:
        print "The number is HIGHER."
    else:
        print "The number is LOWER."

    return False


def isValid(guess):
    return guess and guess.isdigit()


def getInput(guesses):
    input = raw_input("(#" + str(guesses) + ") Guess a number: ")
    while not isValid(input):
        print "Please guess an integer!"
        input = raw_input("(#" + str(guesses) + ") Guess a number: ")

    return int(input)



def runGame(min, max):
    the_number = randint(min, max)
    guesses = 0
    guess = 0

    print "I'm thinking of a number between " + str(min) + " and " + str(max) + " ..."

    while not checkGuess(guess, the_number):
        guesses += 1
        guess = getInput(guesses)

    print "YOU WON!"


def printTitle():
    print "----------------------"
    print "----- MASTERMIND -----"
    print "----------------------"
    print 


if __name__=='__main__':
    printTitle()
    runGame(1, 100)
\$\endgroup\$
1
\$\begingroup\$

Instead of the printTitle function which is less efficient

def printTitle():
  print "----------------------"
  print "----- MASTERMIND -----"
  print "----------------------"
  print ""

Just save the title to a variable and print it out or make a new file and import the variable from it.

title = """
----------------------
----- MASTERMIND -----
----------------------

"""
print(title)
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! Could you please edit your answer and explain what benefit this would have? \$\endgroup\$ Sep 10, 2019 at 22:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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