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()
1 Answer 1
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")
-
\$\begingroup\$ Feel free to ask me any questions. \$\endgroup\$wei2912– wei29122015年04月18日 13:20:53 +00:00Commented Apr 18, 2015 at 13:20
-
1\$\begingroup\$ You have
0
and100
in three places - DRY! \$\endgroup\$jonrsharpe– jonrsharpe2015年04月18日 13:36:26 +00:00Commented Apr 18, 2015 at 13:36 -
\$\begingroup\$ @jonrsharpe Reminds me, I forgot to have the class take in an upper and lower bound. Fixed. \$\endgroup\$wei2912– wei29122015年04月18日 15:07:39 +00:00Commented Apr 18, 2015 at 15:07
-
1\$\begingroup\$ I would probably have a
_gen_number
method that generates a random number based onself.lower
, andself.upper
. You could then call this to getself.winningNumber
andself.comGuessNumber
. And I don't thinkself.humanGuessNumber
andself.comGuessNumber
need to be attributes ofself
, since they are never used outside of theisWin
method. They should probably be just variables inisWin
. Also, it would probably be good to have default values forlower
andupper
, such as0
and100
, respectively. \$\endgroup\$TheBlackCat– TheBlackCat2015年04月20日 11:47:08 +00:00Commented Apr 20, 2015 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– TheBlackCat2015年04月20日 11:58:42 +00:00Commented Apr 20, 2015 at 11:58
Explore related questions
See similar questions with these tags.