5
\$\begingroup\$

I have been using Python to learn programming, but I am just recently beginning to utilize classes. I have a basic game below. Please review and let me know what I should improve on.

import random

class Game:

    def __init__(self):
        self.winningNumber = random.randint(0,100)        
        self.humanGuess()

    def humanGuess(self):
        self.humanGuessNumber = int(input("Guess from 0-100"))
        self.comGuess()
        return self.humanGuessNumber
    def comGuess(self):
        self.comGuessNumber = random.randint(0,100)
        self.conclude()
        return self.comGuessNumber
    def conclude(self):
        if abs(self.winningNumber - self.humanGuessNumber)< abs(self.winningNumber - self.comGuessNumber):
            print("You win")
        else:
            print("You suck")


game = Game()
\$\endgroup\$
5
\$\begingroup\$

You defined the class Game. However, this is an extremely vague name. Even if you have already named your module something like guessthenum, you should still give the class a specific name. I would select GuessTheNum as the class name.

Next up, we have your initialization function:

def __init__(self):
    self.winningNumber = random.randint(0,100)        
    self.humanGuess()

Nothing much to talk about here, other than my minor nitpick that you should add a space after your commas. I do have something to talk about the humanGuess and comGuess functions:

def humanGuess(self):
    self.humanGuessNumber = int(input("Guess from 0-100"))
    self.comGuess()
    return self.humanGuessNumber

def comGuess(self):
    self.comGuessNumber = random.randint(0,100)
    self.conclude()
    return self.comGuessNumber

The problem with the humanGuess function is that it forces input to be passed using the input function. In my opinion, classes should not engage in any form of IO unless strictly necessary (for example, a class for reading files).

Following this change, there is no need to return anything from humanGuess and comGuess. After all, the data is stored in the class.

To fix these problems, I would change both all three functions to this:

def __init__(self):
    self.winningNumber = random.randint(0, 100)

def humanGuess(self, n):
    self.humanGuessNumber = n

def comGuess(self):
    self.comGuessNumber = random.randint(0, 100)

Next, the conclude function:

def conclude(self):
    if abs(self.winningNumber - self.humanGuessNumber)< abs(self.winningNumber - self.comGuessNumber):
        print("You win")
    else:
        print("You suck")

Once again, following the no IO rule, I restructured the function such that it would return a boolean value indicating if the player wins:

def conclude(self):
    return (
        abs(self.winningNumber - self.humanGuessNumber)
        < abs(self.winningNumber - self.comGuessNumber)
    )

Now that we've cleared up the functions, let's take a look at the whole class:

class GuessTheNum:
    def __init__(self):
        self.winningNumber = random.randint(0, 100)

    def humanGuess(self, n):
        self.humanGuessNumber = n

    def comGuess(self):
        self.comGuessNumber = random.randint(0, 100)

    def conclude(self):
        return (
            abs(self.winningNumber - self.humanGuessNumber)
            < abs(self.winningNumber - self.comGuessNumber)
        )

I find that the structure of this class is unwieldy. If I were to design a GuessTheNum class, I would have just a single isWin function. It would take in my guess, generate a computer guess, then tell me if I suck or not.

Here's the way I would have written the full program:

class GuessTheNum:
    def __init__(self):
        self.winningNumber = random.randint(0, 100)

    def isWin(self, n):
        self.humanGuessNumber = n
        self.comGuessNumber = random.randint(0, 100)
        return (
            abs(self.winningNumber - self.humanGuessNumber)
            < abs(self.winningNumber - self.comGuessNumber)
        )

gtn = GuessTheNum()
n = int(input("Guess a number from 0 to 100: "))
if gtn.isWin(n):
    print("You win")
else:
    print("You suck")

EDIT 1:

@jonrsharpe reminded me that I had a DRY violation: 0 and 100 was repeated in a few places. Here's the updated version:

class GuessTheNum:
    def __init__(self, lower, upper):
        self.lower, self.upper = lower, upper
        self.winningNumber = random.randint(self.lower, self.upper)

    def isWin(self, n):
        self.humanGuessNumber = n
        self.comGuessNumber = random.randint(self.lower, self.upper)
        return (
            abs(self.winningNumber - self.humanGuessNumber)
            < abs(self.winningNumber - self.comGuessNumber)
        )

lower, upper = 0, 100
gtn = GuessTheNum(lower, upper)
n = int(input("Guess a number from %d to %d: " % (lower, upper)))
if gtn.isWin(n):
    print("You win")
else:
    print("You suck")

EDIT 2:

@TheBlackCat gave a few suggestions and reminded me of the PEP style. This fixes another DRY with random.randint.

class GuessTheNum:
    def _gen_number(self):
        return random.randint(self.lower, self.upper)

    def __init__(self, lower=0, upper=100):
        self.lower, self.upper = lower, upper
        self.win_num = self._gen_number()

    def isWin(self, human_guess):
        com_guess = self._gen_number()
        return (
            abs(self.win_num - human_guess)
            < abs(self.win_num - com_guess)
        )

lower, upper = 0, 100
gtn = GuessTheNum(lower, upper)
n = int(input("Guess a number from %d to %d: " % (lower, upper)))
if gtn.isWin(n):
    print("You win")
else:
    print("You suck")
\$\endgroup\$
  • \$\begingroup\$ Feel free to ask me any questions. \$\endgroup\$ – wei2912 Apr 18 '15 at 13:20
  • 1
    \$\begingroup\$ You have 0 and 100 in three places - DRY! \$\endgroup\$ – jonrsharpe Apr 18 '15 at 13:36
  • \$\begingroup\$ @jonrsharpe Reminds me, I forgot to have the class take in an upper and lower bound. Fixed. \$\endgroup\$ – wei2912 Apr 18 '15 at 15:07
  • 1
    \$\begingroup\$ I would probably have a _gen_number method that generates a random number based on self.lower, and self.upper. You could then call this to get self.winningNumber and self.comGuessNumber. And I don't think self.humanGuessNumber and self.comGuessNumber need to be attributes of self, since they are never used outside of the isWin method. They should probably be just variables in isWin. Also, it would probably be good to have default values for lower and upper, such as 0 and 100, respectively. \$\endgroup\$ – TheBlackCat Apr 20 '15 at 11:47
  • 1
    \$\begingroup\$ Also, the method and attribute names are not pep8 compliant. They should be all lowercase, separated by underscores. \$\endgroup\$ – TheBlackCat Apr 20 '15 at 11:58

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.