patternpythonMinor
A tool to help you memorize text
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.
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:
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
You can assign the result of a boolean to a variable in Python, so you could turn these three lines:
Into just one:
You don't need to use a
you can use
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.
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
I assume you have this bare
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.
Don't have an integer called
Python has a concept called truthiness where many types of data can be evaluated as a boolean easily. So if you just go
Also you have duplicate code here. It could be avoided with a little rearranging.
The main thing that might look confusing to you here is the
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 = FalseInto just one:
show_word = random.randrange(10) > 4You 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 offor 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 += 1That 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 = 0The main thing that might look confusing to you here is the
continue keyword. It's used in loops to tell Python toCode 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 = Falseshow_word = random.randrange(10) > 4for 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.