6
\$\begingroup\$

In my competition, I was to create a game that asked at least 5 easy questions, and up to ten hard questions, summing fifteen questions. Please review my code, especially how I formatted it with the class. ALL criticism is appreciated.

#SkillsUSA Computer Programming Problem Number One - Math Challenge

import random
import time

class Math_game(object):

    def __init__(self):

        print("Welcome to the Official Math Game of SkillsUSA!\nYou will have at least five relatively simple questions,\nand up to ten difficult ones.\nGood luck!")
        time.sleep(5)
        x = 3
        while x != 0:
            print("Starting in ",x,"...")
            time.sleep(1)
            x -= 1

        #Above we can see a cheesy intro with the time countdown.


        self.tried = 0
        self.correct = 0
        self.easyQuestionsAsked = 0
        self.numCorrect = -1


        #The __init__ method is simply the initiating object. When an object of the class is made, this method/function is automatically called.
        #By automatically declaring all of these objects, we can start keeping track of the number of questions attempted, correct, etc.

    def prob(self, maximum, operators):

        self.maximum = maximum
        self.operators = operators
        self.num_a, self.num_b = [random.randint(1,self.maximum),random.randint(1,self.maximum)]

        #In the above line, we are able to declare two variable values in one, efficient line.

        operators = ['+','-','*']

        self.operator = operators[random.randint(0,int(self.operators))] 

        #The operators parameter of the "prob" method is used to access the list of operators, If only two operators are available to use, a random number
        #chooses between + or -. Otherwise, it will choose between + or - or *.

        if self.maximum < 50:

            if self.num_a <= self.num_b:
                self.num_a = random.randint(1,self.maximum)

        #If it is an easy problem and we are not allowed to use negatives, and the first number is smaller than the second, just default to 25.

        if self.operator== '+':
            self.answer = self.num_a + self.num_b
        elif self.operator == '-':
            self.answer = self.num_a - self.num_b
        else:
            self.num_a, self.num_b = random.randint(-12,12), random.randint(-12,12)
            # If we are using the multiplication operator, set the numbers to max out at 12, or be as low as -12.
            self.answer = self.num_a * self.num_b



        #Above, we use the parameter, userself.answer, and see if the self.answer is correct. Below, we the returned value is whether the user correctly self.answered the question.

        return self.num_a, self.operator, self.num_b


    def isCorrect(self, userInput):

        self.userInput = userInput
        #Simply returning whether the user answer was correct or not.
        return self.userInput == self.answer



game = Math_game()
#Make an object of the  Math_game class.
while game.tried <= 15:
    if game.easyQuestionsAsked <= 5:
        game.prob(25,1)
        game.easyQuestionsAsked += 1
        #While we have not asked 15 questions yet, and while there have been less than 5 easy questions asked, ask another easy question.
    else:
        game.prob(50,2)
    userAnswer = int(input("Problem:  {0} {1} {2}  :  ".format(game.num_a, game.operator, game.num_b)))
    #If we do not HAVE to ask an easy question, feel free to ask a hard one.

    if game.isCorrect(userAnswer):
        game.numCorrect += 1
        print("Right!")
    else:
        print("Wrong!")
    game.tried += 1
    #Accumalting questions asked.

if game.numCorrect > 11:
    print("Great job! ({0}/15)".format(game.numCorrect))
else:
    print("Try again for a better score! ({0}/15)".format(game.numCorrect))
print("This window will automatically close in ten seconds.")

time.sleep(10)
quit()
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

A few notes touching upon encapsulation and responsibilities:

    def __init__(self):

        print("Welcome to the Official Math Game of SkillsUSA!\nYou will have at least five relatively simple questions,\nand up to ten difficult ones.\nGood luck!")
        time.sleep(5)
        x = 3
        while x != 0:
            print("Starting in ",x,"...")
            time.sleep(1)
            x -= 1

the initialization method of a class should be responsible to get the object into a workable state. It's supposed to initialize any required fields and then stop there.

This one here doesn't. Whenever some code creates a "new instance" of Math_game you'll get this welcome message, your thread will be locked up for 8 seconds, all this is stuff you don't want to have in your initializer. Only after that you get to the actually interesting stuff, namely initializing your fields.

Additionally this method is missing a docstring (that seems to be rather unconventional on __init__ methods, thanks to Gareth Rees) and instead has some non-parsable comments, which are suited to explain the code to python beginners (like I am one :D). You might prefer to add the docstring on class-level as suggested by Gareth.

That being said, your "cheesy intro" should be a separate method, taking a few parameters.


Next thing is: whenever a module gets imported in python, code in there is executed. This includes your current code. You definitely shouldn't do that.

The simple and idiomatic fix for that is:

if __name__ == '__main__':

For more information on this construct, read up at stackoverflow


Overall your class could use more "API documentation". As of now if I wanted to use this from somewhere else, as a minigame or something, I'd have to jump through some hoops (with the problematic __init__ only being the first). This includes the less convenient name prob. This method takes two arguments and if I didn't have the source code, I'd have no clue what they were.

The first step to fixing this is getting a proper docstring in there. The next thing is to extract some responsibilities to clarify your design.

You should not return the solution when generating the problem. Instead you should store it somewhere in the class instead, so it can be checked properly with isCorrect.


While we're there: Your naming is sometimes a little off. You're violating the official conventions in a few places isCorrect being one of them. Methods, as well as variables and fields are supposed to be named in snake_case, Classes are supposed to be named in CapWords (without underscores)...

There's also tools that check the PEP-8 conventions for you, one of them being the IDE pycharm. Make use of these tools to simplify your life when coding ;)

\$\endgroup\$
0
3
\$\begingroup\$

Make it generic and make it maintainable.

To give a simple example:

print("Welcome to the Official Math Game of SkillsUSA!\nYou will have at least five relatively simple questions,\nand up to ten difficult ones.\nGood luck!")`

and

print("This window will automatically close in ten seconds.")

time.sleep(10)`

The first one is way too long, but that's not the only problem. If you ever extend your code, you'll lose track of what message is displayed where. Define a WELCOME_MSG and put the string in there, reducing the actual statement to

print(WELCOME_MSG)

In the second statement I've mentioned you've made a potential trap for yourself. If you ever decide to reduce or increase the delay, you'll have to change the value in both the print() and the time.sleep(). Again, make a variable (make it something obvious, SHUTDOWN_DELAY) and set it to the amount necessary. Put the rest in a SHUTDOWN_MSG. Result:

print(SHUTDOWN_MSG, SHUTDOWN_DELAY)

time.sleep(SHUTDOWN_DELAY)
\$\endgroup\$

Your Answer

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

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