Problem
I’ve written this very simple quiz program, I’m trying to get to grips with OOP (Object Orientated Programming), so I’ve started simple with very basic Classes. The nature of the code would have suited using functions but I wanted to practice with Python Classes.
Any comments or suggestions about my code would be most welcome, my ultimate goal is to be as pythonic as possible in future projects.
I’ve tried to follow Python PEP8 guidelines.
from colorama import *
import random
import os
init()
class StartQuiz():
def __init__(self, capital_dict: str = {}, randomize_keys: str = []):
self.capital_dict = capital_dict
self.randomize_keys = randomize_keys
def set_up(self):
with open('capitals.txt') as file:
for data in file:
[country, capital] = data.split(',')
self.capital_dict[country] = capital.strip()
self.randomize_keys = list(self.capital_dict)
random.shuffle(self.randomize_keys)
return [self.capital_dict, self.randomize_keys]
class DisplayAnswer(StartQuiz):
def __init__(self, capital_dict, randomize_keys):
super().__init__(capital_dict, randomize_keys)
def show_answer(self):
for city in self.randomize_keys:
print(Fore.BLUE + f'{self.capital_dict[city]}', end=' ')
print('n')
class DisplayQuestion(StartQuiz):
score: int = 0
def __init__(self, capital_dict, randomize_keys):
super().__init__(capital_dict, randomize_keys)
def ask_question(self):
for country in self.randomize_keys:
print(Fore.YELLOW + f'Capital of {country} is ? ', end='')
answer = input(Fore.WHITE)
if answer.lower() == self.capital_dict[country].lower():
print(Fore.BLUE + 'Correct!')
self.score += 1
else:
print(Fore.RED + 'Incorrect!')
return [self.score, len(self.capital_dict)]
def main():
startQuiz = StartQuiz()
while True:
os.system('CLS')
print(Fore.WHITE + '*** Capital City Quiz ***n')
print('Choose the correct city from the list below:n')
DisplayAnswer(startQuiz.set_up()[0], startQuiz.set_up()[1]).show_answer()
total_score = DisplayQuestion(startQuiz.set_up()[0], startQuiz.set_up()[1]).ask_question()
print(Fore.WHITE + f'nYou scored: {total_score[0]} out of {total_score[1]}')
input('nPress <Enter> to play again')
if __name__ == "__main__":
main()
Solution
The classes’ hierarchy and their names don’t make much sense to me. Ideally, a class should represent one well-defined entity. What kind of entity is StartQuiz
? The name is confusing and there’s no documentation to clarify it. I’d recommend naming it properly (something like QuestionsLoader
) and appropriate doc comments.
It also mixes up reading the input and choosing random questions. These are different things so they belong to different methods (and possibly even different classes).
The DisplayAnswer
is also a very strange class name (it’s something like OutputFormatter
, isn’t it?). I ask the same question again: “What kind of entities do its instance represent?” and I can’t really answer it. The inheritance doesn’t make any sense here. Inheritance represent “is-a” relation. Is a DisplayAnswer
a StartQuiz
? With such names, the question sounds very weird, but even with proper renaming the answer is clearly no (I don’t see any rationale behind an affirmative answer here).
The same applies to the DisplayQuestion
class.
There’s one more thing: there’s pretty much no point in having a class that has one method that prints something to the output and doesn’t encapsulate any state. You’re even using it as a function: DisplayAnswer(startQuiz.set_up()[0], startQuiz.set_up()[1]).show_answer()
. It could be just a function show_answer
, couldn’t it? The situation with the DisplayQuestion
is the same here.
Even if you want to keep them all as classes, I’d strongly recommend taking a step back and redesigning your system. Having classes for the sake of having classes makes no sense. There should be some kind of “physical” (or “real world”) meaning to them. You can start with the following questions:
-
What kind of entities are there in this problem? Typically, an entity can and should be described with a noun like
loader
,formatter
and so on. -
Once you know what these entities are, you can think of the classes to implement them (but again, the “physical” meaning comes first. If something is just one simple stateless action, I’d rather make it a function).
-
This part also includes designing the relations between the classes. Inheritance means “is-a”. Before building a hierarchy, ask yourself: “Is X really Y?”).
There’re a lot of other issues with this code (hardcoded file names, improper default arguments use, lack of documentation and tests and so on), but I think that fixing the high-level design is the first thing to do right now. It’s impossible to write good code without modeling the problem properly, so that comes first.