3
\$\begingroup\$

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) < 5:
                    show_word = False
                for char_idx in range(0,len(word)):
                    if (word[char_idx].isalpha() and show_word) or not word[char_idx].isalpha():
                        current_line += word[char_idx]
                    else:
                        missing_words[idx] += word[char_idx]
                        if not args.no_color:
                            current_line += '\033[91m'
                        current_line += '_'
                        if not args.no_color:
                            current_line += '\033[0m'
                if word[-1] != '\n':
                    current_line += ' '
                if len(missing_words[idx]):
                    missing_words.append(u'')
                    idx += 1
            missing_words.pop()
            attempts = 0
            while len(missing_words):
                print(current_line)
                try:
                    attempt = raw_input('Enter the next missing word: ')
                    if unicode(attempt,'utf-8') == missing_words[0]:
                        print("Correct! 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
                    elif args.attempts > 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 + "'")
\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

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 ignore the rest of the code in the loop and go to the next iteration. This way, if the answer is incorrect but the user has attempts left, Python will continue onto the next iteration.

This is a good error message, but you should use str.format to make nicer looking string formatting.

print("File not found: '{}'".format(args.filename))

It's helpful both for coercing variables into strings but also for making the string one coherent string of characters you can read rather than breaking it up.

\$\endgroup\$
2

Your Answer

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

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