4
\$\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 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()
\$\endgroup\$

1 Answer 1

3
\$\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.

\$\endgroup\$

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.