You've clearly been around, and most of your code does follow the PEP8 guidelines, with some minor exceptions:
- Use two blank lines between top level functions – This opens up the file and helps separate the functions apart
- Add docstrings and comments to neccessary parts – Whilst most of it is intuitive, there are some parts that are not as intuitive. I.e. the usages of
correct_dict
and action
list, and non-intuitive usage on how to end program
But all in all, stylewise it looks quite nice.
Code review
I do have some more issues when reviewing your design choices for this utility:
- No clear way of actually ending it – How do you end it? How many knows that you can use Ctrl-C to issue the keyboard interrupt? And this doesn't work in all environments either. When I run your program from "Spyder-Py2", I have to use the end program button to actually stop it. I would add a stop word, i.e. 'quit' or an empty string to terminate program
- Little or no instructions – Even though your clear the terminal to avoid seeing previous answers, you could provide a little information. When the screen only says 'see', you don't have much information as to what to do. I see you call it "no noise", but some instructions are always good to have, and since you clear the terminal, there is plenty of space
- Why
correct_dict[choice] % 3 == 0
? – This is counter intuitive, how can it be more than 3 in your current logic, and why not simply test on equals 3 (or larger than 3?) Why the modulo operation?
- Ineffective file handling – Why do you read the entire file for each time you want to review a new flashcard? You are opening, reading, appending and writing the file way too many times. You should rather read it once at startup into a list, and use the list to keep your extras and removals when answering false or correct.
- Why the destructive file handling? – If you have a successful session, you'll lost the original contents of your flashcard file. Never to be seen again. To me, it would make more sense to leave it intact for secondary runs.
- Change
sleep
vs correct/wrong status printing – To me it makes more sense to remove the sleep
(which didn't work correctly within Spyder as it didn't output the messages before after the sleep, and then the terminal cleared), and rather state the status when asking the new question.
- Separate action list from action input – To jam these together makes it hard to read. It is a neat option to use a list for actions, but it doesn't read well. It is better to split these, so that you have a list of actions, and then after doing your input, you choose in this list.
Add error handling – All too often we skip error handling, which is a little sad. Especially in such a utility program that this is with file handling it should have error handling if the file doesn't exist, isn't readable or writable, and similar cases. This also applies to if the action is not a legal action.
And what if the flashcard file is empty, or you complete the set? Neither of these situations are handled in your current code.
Allow user to specify flashcard file – If the user could specify the flashcard file as an argument, you would make this utility a lot more useful as you could prepare multiple sets
Code refactored
Here is my version (written originally in Python 2.7, but I think it should run directly in Python 3):
import random
import os
import sys
from collections import defaultdict
# input = raw_input # Crude Python 2 hack...
FLASHCARD_FILENAME = "flashcards.txt"
CORRECT_ANSWER_COUNT = 3
def clear_terminal():
"""Clear the terminal independent of OS.
Credit to: http://stackoverflow.com/questions/2084508/clear-terminal-in-python
"""
os.system('cls' if os.name == 'nt' else 'clear')
def read_flashcard_file(filename):
"""Reads all flashcards from file into a list of (question, answer)."""
flashcards = []
with open(filename) as flashcard_file:
for line in flashcard_file:
# Split line on colon into question and answer
question, answer = line.lower().split(":")
# Remove white space around each, and append to flashcard list
flashcards.append((question.strip(), answer.strip()))
return flashcards
def write_flashcard_file(filename, flashcards):
"""Overwrite filename with new flashcards."""
with open(filename, "w+") as filename:
for flashcard in flashcards:
filename.write('{0} : {1}\n'.format(*flashcard))
def add_flashcards(filename):
"""Add multiple new flashcards to your current flashcard file."""
flashcards = read_flashcard_file(filename)
print("Enter 'end' at question prompt to stop adding questions")
print("Enter 'cancel' at question prompt to abort all additions")
question = ''
while question != 'end':
question = input("\nEnter the question for the new flashcard: ").lower().strip()
answer = input("Enter the answer for the new flashcard: ").lower().strip()
if question == 'cancel':
break
elif question == 'end':
continue
flashcard = (question, answer)
if flashcard in flashcards:
print("This flashcard already exists in file. Not added")
else:
flashcards.append(flashcard)
else:
# Only called on normal ending of while loop
write_flashcard_file(filename, flashcards)
def remove_flashcards(filename):
"""Remove multiple flashcards from your current flashcard file."""
# Read flashcards from file
flashcards = read_flashcard_file(filename)
print("Enter 'end' at question prompt to stop removing questions")
print("Enter 'cancel' at question prompt to abort all removals")
remove_question = ''
while remove_question != 'end':
remove_question = input("\nEnter the question to remove: ").lower().strip()
if remove_question == 'cancel':
break
elif remove_question == 'end':
continue
question_indexes = [ idx for idx, (question, answer) in enumerate(flashcards)
if question == remove_question]
if not question_indexes:
print("Couldn't find question: '{}'".format(remove_question))
else:
for idx in question_indexes:
del flashcards[idx]
else:
# Only called on normal ending of while loop
write_flashcard_file(filename, flashcards)
def review_flashcards(filename):
"""Review all flashcards, repeating if wrong, and clearing if correct."""
flashcards = read_flashcard_file(filename)
# Will hold how many times a flashcard has been answered correctly
correct_answer = defaultdict(int)
previous_answer = None
while True:
clear_terminal()
print("Use 'end' as answer to stop reviewing.")
if previous_answer:
print("Your previous answer was: {}".format(previous_answer))
# print("Correct_answer: {}".format(correct_answer))
# print("Flashcards: {}".format(flashcards))
# Select a random flashcard, and store both as flashcard and split version
if flashcards:
question, answer = flashcard = random.choice(flashcards)
else:
print("Congratulations!!! You've completed the entire set")
break
new_answer = input("\n {} : ".format(question)).lower().strip()
if new_answer == 'end':
break
correct = new_answer == answer
previous_answer = "Correct!" if correct else "Wrong, the correct answer was: '{}'".format(answer)
if not correct:
# Add the question to list again, to increase testing on it
flashcards.append(flashcard)
else:
# If answered correctly a given number of times, remove from list
correct_answer[flashcard] += 1
if correct_answer[flashcard] >= CORRECT_ANSWER_COUNT:
# Remove all instances of that flashcard from list
for remove_idx in reversed([idx for idx, element in enumerate(flashcards)
if element == flashcard]):
del flashcards[remove_idx]
# Defined here instead of top, to be able to refer to the functions directly
ACTIONS = {
"review" : (review_flashcards, "Review flashcards"),
"add" : (add_flashcards, "Add one or more flashcard to set"),
"remove" : (remove_flashcards, "Remove one or more flashcards from set")
}
def main(filename):
"""Main loop asking and handling: add, remove and review of flashcards."""
while True:
print("\nYour actions are:")
print(" 'end' : To end program")
for idx, (action_choice, (_, action_text)) in enumerate(list(ACTIONS.items())):
print (" '{}' : {}".format(action_choice, action_text))
action_choice = input("Action: ").lower().strip()
if action_choice == 'end':
break
elif action_choice in ACTIONS:
ACTIONS[action_choice][0](filename)
else:
print("Illegal option, please choice again")
if __name__ == "__main__":
# Enter some magic to choose filename from arguments...
print('args: {}'.format(sys.argv))
if len(sys.argv) == 2 and sys.argv[1] != '':
filename = sys.argv[1]
else:
filename = FLASHCARD_FILENAME
main(filename)
Some comments to my own code:
- I haven't added error handling related to file operations
- I've used the
while...else
structure instead of exception handling to provide two alternative ways out of add_flashcards
and remove_flashcards
. The else
of a loop is entered if the loops terminates normally, that is if break
is not called. This is used to give the user an option to cancel or abort the edit operation on flashcard file
- When removing a question from the list, I remove it from all places in the list. This calls for searching through the list and removing them in reversed order. If not done in reversed order, the list of indexes will be wrong as the list shifts when you delete items.
- I've opted to keep the "end" action out of the
ACTIONS
list, to emphasize that this is not an ordinary action. It could be added to the list, but I still think I would have kept that as a separate choice to show how to terminate out of the loop
- I've added descriptions to the
ACTIONS
list using a tuple. This could probably be done in a nicer manner, but as it stands it does allow for easily adding new functions which automatically will be included in the action menu. This also keeps the menu and action items closely in sync
- The filename selection from arguments is rudimentary, and you should most likely better use argparse or docopts modules to do this. The code presented is more a proof of concept, rather than a good implementation.