HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

A tool to help you memorize text

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
youtexthelpmemorizetool

Problem

I wrote a snippet of code to help me memorize text out of files and was hoping to get some feedback on the code, since Python isn't my strongest language.

You can see the code on GitHub, but I've pasted it below as well.

#!/usr/bin/python
# coding=utf-8

import argparse
import codecs
import random
import sys

parser = argparse.ArgumentParser(description="""A tool to help memorize some text! When provided with a file, this program will remove random words from each line of text and ask you to provide the words that were removed.""")
parser.add_argument('--no-color', action='store_true', help='hide colorful underlining')
parser.add_argument('--a', dest='attempts', default=3, help='number of attempts to allow per word (0 for unlimited attempts, default: 3)')
parser.add_argument('filename', metavar='filename', type=str, help='the text file')

args = parser.parse_args()

try:
    with codecs.open(args.filename, 'r', 'utf-8') as f:
        current_line = ''
        for line in f:
            missing_words = [u'']
            idx = 0
            for word in line.split(' '):
                show_word = True
                if random.randrange(10)  0 and attempts == args.attempts:
                        print("Too many attempts. The word was '" + missing_words[0] + "'.")
                        if args.no_color:
                            current_line = current_line.replace(''.join('_' for char in missing_words[0]),missing_words[0],1)
                        else:
                            current_line = current_line.replace(''.join('\033[91m_\033[0m' for char in missing_words[0]),missing_words[0],1)
                        missing_words.pop(0)
                        attempts = 0
                    else:
                        print("Incorrect. Please try again.")
                        attempts += 1
                except:
                    sys.exit(0)
except IOError:
    print("File not found: '" + args.filename + "'")

Solution

Python's style guide (PEP0008) says that lines should aim to be 79 characters long. This prevents people having to scroll across as they read a line. You've broken that multiple times. I'd suggest using ways of breaking lines to avoid it. Like this:

parser = argparse.ArgumentParser(description="""A tool to help memorize some text!
When provided with a file, this program will remove random words from each line
of text and ask you to provide the words that were removed.""")
parser.add_argument('--no-color', action='store_true',
                    help='hide colorful underlining')
parser.add_argument('--a', dest='attempts', default=3,
                    help=('number of attempts to allow per word '
                          '(0 for unlimited attempts, default: 3)') )


To explain that last one. You can spread what would be one line over multiple lines by putting brackets around it. You can then break strings up without even using + because they'll be implicitly concatenated together, turning it back into one long string.

You can assign the result of a boolean to a variable in Python, so you could turn these three lines:

show_word = True
if random.randrange(10) < 5:
    show_word = False


Into just one:

show_word = random.randrange(10) > 4


You don't need to use a for loop and refer to the character by index, you can directly get Python to loop over the individual elements of a list. So instead of

for char_idx in range(0,len(word)):
    if (word[char_idx].isalpha() and show_word) or not word[char_idx].isalpha():


you can use

for char in word:
    if (char.isalpha() and show_word) or not char.isalpha():


Much cleaner and easier to read.

What is this part doing? It's quite unclear and I had to read back up and down to understand it. A comment would give very useful context here.

# If there was a word added, append a new empty string to missing words.
if len(missing_words[idx]):
    missing_words.append(u'')
    idx += 1


That comment could be improved, but I think even better would be a refactoring of this code. You should make a temporary empty string, then at the end check if it contains anything and append that. Performing operations on the list item only to append a new item one is kind of backwards. Also, for the record, you could eliminate idx anyway because with Python you can use -1 as an index value to get the last item in the list. So you could just do missing_words[-1] and get the same result.

I assume you have this bare try except so that people don't get errors spat at them for using Ctrl+C to exit the program. But in that case you should still use a specific error like KeyboardInterrupt otherwise you're just hiding your errors and making debugging harder. Also it's important not to put too many lines inside a try except when it can be avoided. If my presumption is right and this is to catch KeyboardInterrupts nicely, then it only needs to be around the raw_input line.

try: 
    attempt = raw_input('Enter the next missing word: ')        
except KeyboardInterrupt:
    sys.exit(0)


If you have other errors you want to catch, you can catch multiple ones. Just make sure they're in a comma separated list and then any of those exceptions will run your exit.

except (KeyboardInterrupt, SystemExit):


Don't have an integer called attempts and a string called attempt. Either switch the int to being tries or switch the string to being guess. It's just confusing the other way and liable to cause typos. Also it could be a good idea to start the loop with next_word = missing_words[0]. It would save you some typing throughout and is more clear about what the variable is.

''.join('_' for char in missing_words[0]) is long and confusing. You can actually just use the multiplication operators on a string, so '_' * len(missing_words[0]) would work fine.

Python has a concept called truthiness where many types of data can be evaluated as a boolean easily. So if you just go elif args.attempts, that will return True for integers that aren't 0, and False for 0 itself.

elif args.attempts and attempts == args.attempts:


Also you have duplicate code here. It could be avoided with a little rearranging.

if unicode(attempt,'utf-8') == missing_words[0]:
    print("Correct! The word was '" + missing_words[0] + "'.")
elif args.attempts > 0 and attempts == args.attempts:
    print("Too many attempts. The word was '" + missing_words[0] + "'.")
else:
    print("Incorrect. Please try again.")
    attempts += 1
    continue

if args.no_color:
    current_line = current_line.replace('_' * len(nextWord), nextWord, 1)
else:
    current_line = current_line.replace('\033[91m_\033[0m' * len(nextWord),
                                        nextWord, 1)
missing_words.pop(0)
attempts = 0


The main thing that might look confusing to you here is the continue keyword. It's used in loops to tell Python to

Code Snippets

parser = argparse.ArgumentParser(description="""A tool to help memorize some text!
When provided with a file, this program will remove random words from each line
of text and ask you to provide the words that were removed.""")
parser.add_argument('--no-color', action='store_true',
                    help='hide colorful underlining')
parser.add_argument('--a', dest='attempts', default=3,
                    help=('number of attempts to allow per word '
                          '(0 for unlimited attempts, default: 3)') )
show_word = True
if random.randrange(10) < 5:
    show_word = False
show_word = random.randrange(10) > 4
for char_idx in range(0,len(word)):
    if (word[char_idx].isalpha() and show_word) or not word[char_idx].isalpha():
for char in word:
    if (char.isalpha() and show_word) or not char.isalpha():

Context

StackExchange Code Review Q#102407, answer score: 6

Revisions (0)

No revisions yet.