I am training for a Symfony certificate. I found a Github repository providing questions to prepare for the exam. The repository contains a data directory with a bunch of questions mapped by topic in Yaml format.
Because raw yaml files are not user-friendly to train with, I made a quick Python script providing a user-interface which displays the questions and allows you to answer.
The script, available on GitHub, allows the user to select a topic and the amount of questions to load from that topic. The questions are loaded randomly and dynamically from the files. After the user answered all the questions a table is provided giving an overview of the correct answers and the score.
Python3 quiz interface 1 Python3 quiz interface 2
The code is fully functional but I am wondering if improvements can be made.
- Can the readability be improved?
- Can the code structure be improved?
- Am I following the PEP styling guide?
- Can some snippets be shortened?
import os
import random
import yaml
from prettytable import PrettyTable
DATA_FOLDER = 'data/'
class Styling:
GREEN = '033円[92m'
YELLOW = '033円[33m'
RED = '033円[91m'
ENDC = '033円[0m'
BOLD = '033円[1m'
ENDBOLD = '033円[0m'
class CLIHandler:
@staticmethod
def display_message(message, color='', bold=False):
print(f'{color}{Styling.BOLD if bold else ""}{message}{Styling.ENDBOLD if bold else ""}{Styling.ENDC}')
@staticmethod
def display_topics(topics):
CLIHandler.display_message('-- SYMFONY CERTIFICATE PRACTICE --\n', Styling.GREEN, True)
for i, topic in enumerate(topics):
CLIHandler.display_message(f'{i + 1}. {topic.name}')
@staticmethod
def display_question(question, question_number):
CLIHandler.display_message(f'\n{Styling.YELLOW}Question #{question_number}{Styling.ENDC} {Styling.BOLD}{question}{Styling.ENDBOLD}', bold=True)
@staticmethod
def display_table(field_names, rows, align='l'):
table = PrettyTable()
table.align = align
table.field_names = [f'{Styling.BOLD}{Styling.GREEN}{field}{Styling.ENDC}{Styling.ENDBOLD}' for field in field_names]
for row in rows:
table.add_row(row)
print('\n', table)
@staticmethod
def input_message(message):
return input(f'{Styling.BOLD}{message}: {Styling.ENDBOLD} ')
@staticmethod
def clear_screen():
os.system('clear')
class Topic:
def __init__(self, name, path):
self.name = name
self.path = path
self.questions = []
def load_topic(self, amount_of_questions):
while len(self.questions) < amount_of_questions:
question = self.__load_random_question()
if question not in self.questions:
self.questions.append(question)
def show_questions(self):
for i, question in enumerate(self.questions):
question.show_question(i + 1)
try:
answers = input('> ').split(',')
question.select_answers(answers)
except (ValueError, IndexError):
CLIHandler.display_message('Invalid input. Skipping...', Styling.RED)
def show_results(self):
rows = []
for i, question in enumerate(self.questions):
question_str = question.question[:75] + (question.question[:75] and '..')
correct_answers = question.correct_answers()
your_answers = question.selected_answers()
result = '✅' if set(correct_answers) == set(your_answers) else '❌'
rows.append([
f'{Styling.YELLOW}#{i + 1}{Styling.ENDC} {question_str}',
', '.join([answer.value[:35] + (answer.value[:35] and '..') for answer in correct_answers]),
result
])
correct_count = self.__correct_answers_count()
CLIHandler.display_table(
field_names=['Question', 'Correct Answer', 'Result'],
rows=rows
)
CLIHandler.display_message(
f'{Styling.BOLD}Score:{Styling.ENDBOLD} {Styling.RED}Wrong: {len(self.questions) - correct_count}{Styling.ENDC} - '
f'{Styling.GREEN}Correct: {correct_count}{Styling.ENDC}'
)
def __correct_answers_count(self):
return sum(
set(question.selected_answers()) == set(question.correct_answers())
for question in self.questions
)
def __load_random_question(self):
question_file = random.choice([f for f in os.listdir(self.path) if f.endswith('.yaml')])
with open(self.path + '/' + question_file) as file:
questions = yaml.safe_load(file)['questions']
random_question = random.choice(questions)
question = Question(random_question['uuid'], random_question['question'])
for answer in random_question['answers']:
question.add_answer(Answer(answer['value'], answer['correct']))
return question
@staticmethod
def list_topics():
return [Topic(topic.lower(), os.path.join(DATA_FOLDER, topic)) for topic in os.listdir(DATA_FOLDER)]
class Question:
def __init__(self, uuid, question):
self.uuid = uuid
self.question = question.replace('\n', ' ').replace('\r', ' ')
self.answers = []
def __eq__(self, other):
if isinstance(other, Question):
return self.uuid == other.uuid
return False
def add_answer(self, answer):
self.answers.append(answer)
def show_question(self, question_number):
CLIHandler.display_question(self.question, question_number)
for i, answer in enumerate(self.answers):
CLIHandler.display_message(f'\t[{Styling.GREEN}{i + 1}{Styling.ENDC}] {answer.value}')
def select_answers(self, answers):
answers = [int(answer) - 1 for answer in answers]
for answer in answers:
if 0 <= answer < len(self.answers):
self.answers[answer].select()
CLIHandler.display_message(f'{Styling.YELLOW}Your answer: {Styling.ENDC}{self.answers[answer].value}')
else:
raise IndexError('Invalid answer selected')
def selected_answers(self):
return [answer for answer in self.answers if answer.selected]
def correct_answers(self):
return [answer for answer in self.answers if answer.correct]
class Answer:
def __init__(self, value, correct):
self.value = value
self.correct = correct
self.selected = False
def select(self):
self.selected = True
def main():
topics = Topic.list_topics()
CLIHandler.display_topics(topics)
try:
selected_topic = int(CLIHandler.input_message('\nSelect topic')) - 1
amount_of_questions = int(CLIHandler.input_message('Amount of questions'))
except ValueError:
CLIHandler.display_message('Invalid input. Exiting...', Styling.RED)
return
if not (0 <= selected_topic < len(topics)):
CLIHandler.display_message('Invalid input. Exiting...', Styling.RED)
return
CLIHandler.clear_screen()
topic = topics[selected_topic]
topic.load_topic(amount_of_questions)
topic.show_questions()
topic.show_results()
if __name__ == '__main__':
main()
-
5\$\begingroup\$ I have rolled back Rev 5 → 4. Please see What should I do when someone answers my question?. \$\endgroup\$Sᴀᴍ Onᴇᴌᴀ– Sᴀᴍ Onᴇᴌᴀ ♦2024年12月17日 00:14:04 +00:00Commented Dec 17, 2024 at 0:14
2 Answers 2
modules, not classes
I like that you're organizing things. But this is not Java.
class Styling:
GREEN = '033円[92m'
YELLOW = '033円[33m'
RED = '033円[91m'
ENDC = '033円[0m'
BOLD = '033円[1m'
ENDBOLD = '033円[0m'
class CLIHandler:
@staticmethod
def display_message( ... ): ...
@staticmethod
def display_topics( ... ): ...
Use modules to organize such things. Use a class for something that holds state, over which you're trying maintain a class invariant.
So the color constants might be at top-level within a styling.py
module,
which you then import
as needed.
Similarly for the display functions, in a cli_handler.py
module.
Typically a @staticmethod
will be a _private()
helper
for some public method that accepts a self
parameter.
A class full of only static methods is a class that should not exist. It maintains no state variables, and defends no class invariant. Prefer to put such static methods at top-level of some suitable module.
nested printing
The app code doesn't necessarily have to know about those color constants
at all. Consider creating a def green(msg: str) -> str:
helper,
and similarly a bold(msg)
helper.
Then the helper can ensure ENDC and ENDBOLD always appear in the proper place.
Furthermore the nesting works out nicely when we compose them,
e.g. print("Hello", bold(red("world")), "!")
CLS
There's no need to fork off a child process here.
@staticmethod
def clear_screen():
os.system('clear')
The ANSI escape codes let you print CSI 2 J to accomplish that. Just define another string constant.
name mangling
I don't understand what's going on here.
def __correct_answers_count(self):
...
def __load_random_question(self):
...
Why are you requesting name mangling for inherited classes?
I don't see any use of inheritance, and there's no design
notes in the comments explaining the requirement.
Mangling is usually not what you want.
Recommend you just remove it.
Prefer single _
underscore for a _private
name,
e.g. _correct_answers_count
.
extra class
The Topic
and Question
classes appear to be well motivated.
Each would benefit from the addition of a """docstring""".
This class seems less well motivated:
class Answer:
def __init__(self, value, correct):
self.value = value
self.correct = correct
self.selected = False
def select(self):
self.selected = True
Consider using a @dataclass decorator to save a little bit of constructor boilerplate.
The selected
attribute seems odd.
In many setups I would expect to see a selected_answer_index
which lives outside of this class and satisfies the need.
But suppose we do really need the attribute.
Don't write a def select()
setter routine.
We're all grownups here, and the attribute is public.
Calling code should just reach in and set the flag.
Python packages worry about SemVer and breaking changes,
but they have a quite different set of concerns around
that compared with java / maven binary compatibility concerns.
When you're tempted to write a getter or setter routine,
think twice and resist the urge to write one.
The caller should probably just be accessing the public attribute.
Linters will flag any caller attempts to access _private
attributes.
There are use cases where it makes sense, but they are relatively uncommon. We might want to maintain / enforce a class invariant such as "mass shall be non-negative", or log that a value was updated. And often we'll use a @property decorator when defining getters / setters.
ANSI colors are nice, but they're not always appropriate.
Consider depending on the
termcolor package.
Then a user could export TERM=dumb
to suppress those escape codes.
-
\$\begingroup\$ I want to thank you for your elaborate feedback. This is exactly the kind of feedback I hoped for. I updated my original post with the updated code. In my opinion the code already looks better and more elegant thanks to your feedback. I however, decided to keep the Answer class as explained in the edit. \$\endgroup\$O'Niel– O'Niel2024年12月17日 00:10:58 +00:00Commented Dec 17, 2024 at 0:10
-
3\$\begingroup\$ Great, glad to help you on your journey. And Sᴀᴍ Onᴇᴌᴀ is just following the site's rules, I understand they can seem slightly odd to newcomers. The idea is that in a year or three folks might visit this Question and see the text which motivated a given Answer, so we try to preserve context. Don't change the code, but it still is OK to add a sentence or two. For example you might offer a link to your GitHub repo, where folks can see the evolution of the code over multiple edits. It's a good way to share your journey with others. \$\endgroup\$J_H– J_H2024年12月17日 00:43:31 +00:00Commented Dec 17, 2024 at 0:43
-
2\$\begingroup\$ Thanks. And I understand. Edited to include a link to the Github repo. \$\endgroup\$O'Niel– O'Niel2024年12月17日 01:12:13 +00:00Commented Dec 17, 2024 at 1:12
-
1\$\begingroup\$ Woo hoo, looks good! Git is the elephant that never forgets. \$\endgroup\$J_H– J_H2024年12月17日 01:32:37 +00:00Commented Dec 17, 2024 at 1:32
These strings are terminal-specific:
GREEN = '033円[92m' YELLOW = '033円[33m' RED = '033円[91m' ENDC = '033円[0m' BOLD = '033円[1m' ENDBOLD = '033円[0m'
You should be examining the TERM
environment variable to determine the correct strings (if any) to emit, since not all the world uses ANSI-compatible terminals.
-
4\$\begingroup\$ Better yet use colorama so colour works on both Unix and Windows. \$\endgroup\$2024年12月17日 03:38:00 +00:00Commented Dec 17, 2024 at 3:38