Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I was hoping I could get some of you to look over a program I wrote. I am just beginning to learn about Python (for the last month or so). Therefore, I may not be aware of techniques that would make this program better. Currently I'm studying Python 2.7. Any advice or suggestions you could provide would be appreciated. Thanks again.

##Flash Card Program

# Import required modules
import random
import sys
import os
import math

# variables
right_answer_total = float(0)
wrong_answer_total = float(0)
percentage = 100 * (float(right_answer_total) / float(answer_total)) if (right_answer_total > 0 and answer_total > 0) else 0


# dictionary containing the information for the questions & answers
word_drills = {'class': 'Tell Python to make a new kind of thing.',
               'object': 'Two meanings: the most basic kind of thing, and any instance of some thing.',
               'instance': 'What you get when you tell Python to create a class.',
               'def': 'How you define a function inside a class.',
               'self': 'Inside the functions in a class, self is a variable for the instance/object being accessed.',
               'inheritance': 'The concept that one class can inherit traits from another class, much like you and your parents.',
               'composition': 'The concept that a class can be composed of other classes as parts, much like how a car has wheels.',
               'attribute': 'A property classes have that are from composition and are usually variables.',
               'is-a': 'A phrase to say that something inherits from another, as in a Salmon *** Fish',
               'has-a': 'A phrase to say that something is composed of other things or has a trait, as in a Salmon *** mouth.'}


# Main portion of the program
def start():
    # For loop that creates a list named keys. It grabs 3 random keys from the dictionary word_drills
    keys = [x for x in random.sample(word_drills, 3)]
    # User is presented with a question. A value from the previous randomly selected keys is selected as the 'question'
    correctanswer = word_drills[random.choice(keys)]
    print "Question: ", correctanswer 
    # Set the variables key1, key2, & key3 to the 3 keys in the list 'keys'
    key1, key2, key3 = keys[0], keys[1], keys[2]
    # User is presented with 3 choices. 
    print "\n\n(a)%s   (b)%s   (c)%s" % (key1, key2, key3)
    a, b, c = word_drills[key1], word_drills[key2], word_drills[key3]
    selection = raw_input("> ")
    print selection
    if selection == "a":
        if a == correctanswer:
            print "That's correct!"
            answered_correctly()
        else:
            print "I'm sorry, that is incorrect..."
            not_answered_correctly()
    elif selection == "b":
        if b == correctanswer:
            print "That's correct!"
            answered_correctly()
        else:
            print "I'm sorry, that is incorrect..."
            not_answered_correctly()
    elif selection == "c":
        if c == correctanswer:
            print "That's correct!"
            answered_correctly()
        else:
            print "I'm sorry, that is incorrect..."
            not_answered_correctly()
    else:
        print "That is not a valid selection."
        exit(0)

# Function when user is correct
def answered_correctly():
    global right_answer_total
    global answer_total
    global percentage
    right_answer_total += 1
    answer_total = float(right_answer_total + wrong_answer_total)
    percentage = 100 * (float(right_answer_total) / float(answer_total)) 
    stat_tracking()

# Function when user is incorrect
def not_answered_correctly():
    global wrong_answer_total
    global answer_total
    global percentage
    wrong_answer_total += 1
    answer_total = float(right_answer_total + wrong_answer_total)
    percentage = 100 * (float(right_answer_total) / float(answer_total)) 
    stat_tracking()

# Stat tracking
def stat_tracking():
    os.system('cls' if os.name=='nt' else 'clear') 
    print "-" * 37
    print "|         Stat Tracking             |"
    print "-" * 37
    print "| Correct | Incorrect |  Percentage  |"
    print "-" * 37
    print "|    %d    |     %d     |     %d %%     |" % (right_answer_total, wrong_answer_total, percentage) 
    print "-" * 37
    print "\n\n\n"
    start()

stat_tracking()
share|improve this question
add comment

2 Answers

Here's a step by step Pythonization walk-trough.

Trivial simplifications

Before:

answer_total = float(right_answer_total + wrong_answer_total)
percentage = 100 * (float(right_answer_total) / float(answer_total)) 

After:

percentage = 100 * right_answer_total / float(right_answer_total + wrong_answer_total)

Reasoning: a single division argument (either numerator or denominator) is sufficient to be of a float type so that the float division was performed.


Before:

keys = [x for x in random.sample(word_drills, 3)]

After:

keys = random.sample(word_drills, 3)

Reasoning: [x for x in l] results in a copy of l since neither filtering nor element transformations are performed.


Before:

key1, key2, key3 = keys[0], keys[1], keys[2]
print "\n\n(a)%s   (b)%s   (c)%s" % (key1, key2, key3)
a, b, c = word_drills[key1], word_drills[key2], word_drills[key3]

After:

print "\n\n(a)%s   (b)%s   (c)%s" % tuple(keys)

Reasoning: same effect with less code; variables key1, key2, key3, a, b, c are unnecessary (see below).

Code Refactoring

Looks like you came from functional programming world. Each of your functions ends in calling another function. Since Python has no tail call optimization you will end up in stack overflow eventually.

Thus your algorithm must be revised in a more imperative way. The whole program looks like this:

  1. Repeat in an infinite loop:
    1. Print statistics
    2. Prepare a question
    3. Gather user input
    4. Exit program if user input was invalid
    5. Check whether user's answer was correct
    6. Update answer statistics

For the outer loop a while True: construction will do.

Statistics printing is mostly left intact. The function name changed from stat_tracking to print_stats to reflect the innards. Note that there is no tail call to start() since this function will be invoked from start() not vice versa. Also, to avoid global variables, stats are passed as paramenters:

def print_stats(right_answer_total, wrong_answer_total, percentage):
    os.system('cls' if os.name=='nt' else 'clear')
    print "-" * 37
    print "|         Stat Tracking             |"
    print "-" * 37
    print "| Correct | Incorrect |  Percentage |"
    print "-" * 37
    print "|    %d    |     %d     |     %d %%     |" % (right_answer_total, wrong_answer_total, percentage)
    print "-" * 37
    print "\n\n\n"

Bearing simplifications in mind and renaming the variables to reflect their meanings in mind here's the question preparation:

possible_answers = random.sample(word_drills, 3)
correct_answer = random.choice(possible_answers)
question = word_drills[correct_answer]
print "Question:", question
print "\n\n(a)%s   (b)%s   (c)%s" % tuple(possible_answers)

Since the whole program resides in a loop there's no need for exit()ing, a simple break will do:

if selection not in ('a', 'b', 'c'):
    print "That is not a valid selection."
    break

Checking the answer for correctness needs a little bit more explanation.

First of all, the answered_correctly() and not_answered_correctly() are gone. They were mostly the same, only a single line differed.

Getting the user's answer is basically inferring an index from possible_answers. Since we validated selection (see above) we are sure that its value is in "a", "b", "c". "a", "b", "c" correspond to 0, 1, 2 indexes of the possible_answers list respectively. Since "a", "b", "c" are laid out sequentially in ASCII table ord(selection) - ord('a') can be used to convert the string to answer index.

Now figuring out whether user was correct or not is a matter of checking whether answer == correct_answer.

Along with statistics update the code looks like this:

answer = possible_answers[ord(selection) - ord('a')]
if answer == correct_answer:
    print "That's correct!"
    right_answer_total += 1
else:
    print "I'm sorry, that is incorrect..."
    wrong_answer_total += 1

percentage = 100 * right_answer_total / float(right_answer_total + wrong_answer_total)

Final Touches

sys and math modules are unused, so do not import them.

The main entry point function is called main() rather then start(). Though that is just a convention.

To avoid your program starting when it is imported (as import program) rather then started as a program (as python program.py) the following trick is used:

if __name__ == '__main__':
    main()

And here's the whole code:

#!/usr/bin/env python

import random
import os

# dictionary containing the information for the questions & answers
word_drills = {'class': 'Tell Python to make a new kind of thing.',
               'object': 'Two meanings: the most basic kind of thing, and any instance of some thing.',
               'instance': 'What you get when you tell Python to create a class.',
               'def': 'How you define a function inside a class.',
               'self': 'Inside the functions in a class, self is a variable for the instance/object being accessed.',
               'inheritance': 'The concept that one class can inherit traits from another class, much like you and your parents.',
               'composition': 'The concept that a class can be composed of other classes as parts, much like how a car has wheels.',
               'attribute': 'A property classes have that are from composition and are usually variables.',
               'is-a': 'A phrase to say that something inherits from another, as in a Salmon *** Fish',
               'has-a': 'A phrase to say that something is composed of other things or has a trait, as in a Salmon *** mouth.'}


# Main portion of the program
def main():
    right_answer_total = 0
    wrong_answer_total = 0
    percentage = 0.0

    while True:
        print_stats(right_answer_total, wrong_answer_total, percentage)

        possible_answers = random.sample(word_drills, 3)
        # User is presented with a question. A value from the previous randomly selected possible_answers is selected as the 'correct_answer'
        correct_answer = random.choice(possible_answers)
        question = word_drills[correct_answer]
        print "Question: ", question
        print "\n\n(a)%s   (b)%s   (c)%s" % tuple(possible_answers)

        selection = raw_input("> ")
        if selection not in ('a', 'b', 'c'):
            print "That is not a valid selection."
            break

        answer = possible_answers[ord(selection) - ord('a')]
        if answer == correct_answer:
            print "That's correct!"
            right_answer_total += 1
        else:
            print "I'm sorry, that is incorrect..."
            wrong_answer_total += 1

        percentage = 100 * right_answer_total / float(right_answer_total + wrong_answer_total)

# Stat tracking
def print_stats(right_answer_total, wrong_answer_total, percentage):
    os.system('cls' if os.name=='nt' else 'clear') 
    print "-" * 37
    print "|         Stat Tracking             |"
    print "-" * 37
    print "| Correct | Incorrect |  Percentage |"
    print "-" * 37
    print "|    %d    |     %d     |     %d %%     |" % (right_answer_total, wrong_answer_total, percentage) 
    print "-" * 37
    print "\n\n\n"

if __name__ == '__main__':
    main()
share|improve this answer
add comment

A few point. These are all mostly smaller tweaks.

# variables
right_answer_total = float(0)
wrong_answer_total = float(0)
percentage = 100 * (float(right_answer_total) / float(answer_total)) if (right_answer_total > 0 and answer_total > 0) else 0

At this point we know right_answer_total and wrong_answer_total are both 0, so percentage can be initialized to 0 without any extra logic. Additionally, you can never get an answer half-right, so right_answer_total and wrong_answer_total should be integers. Store the data properly and then convert it if you need to later.

You are creating a number of temporary variables that are not providing much benefit.

key1, key2, key3 = keys[0], keys[1], keys[2]
# User is presented with 3 choices. 
print "\n\n(a)%s   (b)%s   (c)%s" % (key1, key2, key3)

This can be done with

print "\n\n(a)%s   (b)%s   (c)%s" % tuple(keys)

I also don't think you are particularly gaining much by making a,b and c. If you switch to 1-3 instead of a-c, you can remove a lot of the repeated code.

print "\n\n(1)%s   (2)%s   (3)%s" % tuple(keys)

try:
    selection = int(raw_input("> "))
    if  selection < 0 or selection > 3:
        print "That is not a valid selection."
        exit(0)

    if word_drills[keys[selection-1]] == correctanswer:
        print "That's correct!"
        answered_correctly()
    else:
        print "I'm sorry, that is incorrect..."
        not_answered_correctly()
except ValueError:
    print "That is not a valid selection."
    exit(0)

You can specify a field with when doing a string format that will keeping the stats like from messing up your box boundary.

This last point is the only thing I believe to be a bigger point. The way you are looping through the methods will eventually cause a stack overflow exception. For every question the user answers, the call stack increases by three.

stat_tracking() calls start() which calls answered_correctly() which calls stat_tracking(). This will continue and none of the methods will ever return until the user enters an invalid selection and the application quits.

Instead, you should remove calls to stat_tracking() from answered_correctly() and not_answered_correctly(), remove the call to start() from stat_tracking(), and use a while loop to control the repetition.

while True:
    stat_tracking()
    start()
share|improve this answer
add comment

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.