current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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()
share|improve this question

2 Answers 2

up vote 4 down vote accepted

Some notes:

  • Using Global variables (when being updated, constants are ok) is a terrible 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.

  • One blank line between functions, not two. In general, you should use blank lines sparingly, otherwise you are increasing the length of the code with no real gain.

  • 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)
share|improve this answer
    
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? – asteri Aug 25 '13 at 13:27
    
print is not a statement anymore, it's a function, so I concluded you are using Python2. – tokland Aug 25 '13 at 13:34
    
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? – asteri Aug 25 '13 at 15:28
1  
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. – tokland Aug 25 '13 at 15:47
up vote 2 down vote

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)
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.