10
\$\begingroup\$

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()
Peilonrayz
44.4k7 gold badges80 silver badges157 bronze badges
asked Dec 16, 2024 at 4:15
\$\endgroup\$
1

2 Answers 2

9
\$\begingroup\$

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.

answered Dec 16, 2024 at 7:57
\$\endgroup\$
4
  • \$\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\$ Commented 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\$ Commented Dec 17, 2024 at 0:43
  • 2
    \$\begingroup\$ Thanks. And I understand. Edited to include a link to the Github repo. \$\endgroup\$ Commented Dec 17, 2024 at 1:12
  • 1
    \$\begingroup\$ Woo hoo, looks good! Git is the elephant that never forgets. \$\endgroup\$ Commented Dec 17, 2024 at 1:32
7
\$\begingroup\$

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.

answered Dec 16, 2024 at 19:15
\$\endgroup\$
1
  • 4
    \$\begingroup\$ Better yet use colorama so colour works on both Unix and Windows. \$\endgroup\$ Commented Dec 17, 2024 at 3:38

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.