# OO structure for holding and scoring exam data

I'm writing some code to administer and grade multiple choice exams. I'd like this to be easily reusable, and I don't really want to have to do things like lay out quizzes and score them with a bunch of list and dict indexing for things like answers, prompts, etc.

I've tried to implement some classes to provide a consistent and simple interface for the rest of the code. (There's a long-term goal here of writing my own instructional management system.) The idea is to take an exam formatted as JSON with both standalone questions and questions that come in blocks, and sensibly represent them so that external code can just do easy stuff like pass a list of question-answer pairs to an exam object for grading.

This is a baseline for future functionality which will include programmatically adding/removing questions (and an interface for same), supplying iterable question-answer groupings to administer exams, providing a statistical summary of taker's scores by subject area, saving scores for later analysis etc.

My mind more naturally works in imperative and functional styles, not so much object-oriented, so I'd love some feedback on this implementation from that standpoint. Does this look like a sensible way of setting up the data structure? Is it appropriately pythonic? Am I violating any major OOP principles?

from collections import OrderedDict

class Question(object):
"""A single question with a unique correct answer.
Attributes:
block: string or None; string if this question belongs to block of
questions, where the string is unique identifier for that block.
qid: string, unique question identifier
prompt: string, question prompt
ID as number/letter and second string is answer text.
explanation: string containing explanation of correct answer.
subjects: list of strings representing topics (for competency analysis)
"""
def __init__(self, BLOCK, QID, PROMPT, ANSWERS, CORRECT, EXPLANATION, SUBJECTS):
self.qid = QID
self.prompt = PROMPT
self.correct = CORRECT
self.explanation = EXPLANATION
self.subjects = SUBJECTS
if not BLOCK:
self.block = None
else:
self.block = BLOCK

def ans(self):
return {'block': self.block, 'qid': self.qid, 'prompt': self.prompt, 'answers': self.answers, 'correct': self.correct, 'explanation': self.explanation, "subjects": self.subjects}

tempans = self.ans()
if studAns == self.correct:
tempans['gotRight'] = True
else:
tempans['gotRight'] = False
return tempans

def getBlock(self):
return self.block

def getID(self):
return self.qid

def dictRepr(self):
return {"block": self.block, "qid": self.qid, "prompt": self.prompt, "answers": self.answers, "correctans": self.correct, "explanation": self.explanation, "subjects": self.subjects}

class QBlock(object):
"""A qBlock is a block of questions, where each block has header text (like
a prompt that applies to all questions) and question objects within it.
Example: 'for questions 1-5, assume the following is true... '
Attributes are header text, list of questions, and blockid.
blockid must match blockid attribute of question that belongs in block.
"""

self.blockid = BLOCKID
self.questions = []

if type(aQuestion) is not Question:
raise TypeError
self.questions.append(aQuestion)

def getID(self):
return self.blockid

def getQuestions(self):
return self.questions

def dictRepr(self):

class NoneBlock(QBlock):
"""Special qBlock for standalone questions (that don't have an assigned
block in the underlying data file)
"""
def __init__(self):
self.blockid = 0
self.questions = []

class Exam(object):
def __init__(self):
self.blocks = OrderedDict()
self.questions = OrderedDict()

def assignBlock(self, aQuestion):
# aQuestion is a question object.  dumps q in noneblock if its designated block does not exist.
if aQuestion.getBlock() and (aQuestion.getBlock() in self.blocks):
else:
if 0 not in self.blocks:
self.blocks[0] = NoneBlock()

# aBlock is a block object.
self.blocks[aBlock.getID()] = aBlock

self.questions[aQuestion.getID()] = aQuestion
self.assignBlock(aQuestion)

with open(jsonfile) as thejson:
for biter in qdict['blocks']:
for qiter in qdict['questions']:
qtemp = Question(qiter['block'], qiter['qid'], qiter['prompt'], qiter['answers'], qiter['correctans'], qiter['explanation'], qiter['subjects'])

return [eachblock.ask() for eachblock in self.blocks.values()]

# answers is a list of (qid, student answer) pairs as tupe of strings


Sample data:

{
"blocks":
[
{
"blockid": "BLOCK01",
}
],
"questions":
[
{
"block": "BLOCK01",
"qid": "Q1",
"prompt": "What noise does the animal make?",
"correctans": "A",
"explanation": "Have you ever seen a cat?",
"subjects": ["Cats", "Noises"]
},
{
"block": "",
"qid": "Q2",
"prompt": "What is the opposite of up?",
"correctans": "B",
"explanation": "If you don't know this, you're probably falling as we speak.",
"subjects": ["Directions", "Life Skills"]
}
]
}


The idea is that the sample data represents an exam, which is saved in a json on disk; the user initializes an exam object and then calls exam.load() on the JSON to get the questions in, at which point the user's external code can display the exam to a taker using exam.administer(), accept the taker's answers and call exam.grade() to score them and do something else with that output, display explanations to the taker, etc.

• Welcome to Codereview, Paul. I hope you get some fine answers. – Legato Oct 3 '15 at 23:49

### OOP

Since you say you're new to OOP, here are a few quick tips:

• When designing classes, think in terms of Abstract Data Types (ADT): an ADT is data + the things you can do on that data, for some well-defined purpose
• Encapsulation, information hiding: what information (data, operations) should the ADT hide? Internal details should be hidden from users.

So far you're doing fine, except for some naming issues. For example looking at Question:

• data: a question has an id, prompt, answers, etc, some of them with getters. So far so good.
• operations: ask, ans, grade, dictRepr

The data of this ADT is fine. The operations sound a bit odd. It's not clear enough what they might do without looking at the implementation, and for example grade does something very different from what I would guess. So there's at least a problem with naming.

On closer look, the methods return different representations of the question data as dictionary objects, with some of the data intentionally hidden. And I suppose ans is only used internally by grade. And the code in ans is duplicated in dictRepr. I suggest the following renames:

• ask -> question_data_as_dict
• dictRepr -> as_dict
• ans -> eliminate, as_dict already does the same job
• grade -> graded_data_as_dict

The idea is that if you look at these methods, you understand better the operations on the ADT.

### Coding style

Some general naming issues:

• Variable names should be snake_case, not SHOUTCASE
• Method names should be snake_case, not camelCase

See more tips in the coding style guide.

### Naming

Some of the names are quite poor. For example QBlock would be nicer to spell out as QuestionBlock.

In this piece of code:

for biter in qdict['blocks']:
for qiter in qdict['questions']:
qtemp = Question(qiter['block'], qiter['qid'], qiter['prompt'], qiter['answers'], qiter['correctans'],


Some of the variables can be easily named better:

• btemp -> block
• qtemp -> question

### The Pythonic way

if not BLOCK:
self.block = None
else:
self.block = BLOCK


Probably this is good enough:

self.block = BLOCK


if studAns == self.correct:
tempans['gotRight'] = True
else:
tempans['gotRight'] = False


You can use boolean expressions directly:

tempans['gotRight'] = studAns == self.correct

• The number of parameters in Question constructor is too long (>=3) which is a code smell. – CodeYogi Oct 8 '15 at 15:28

I have more experience with Python than OOP so I'm going to focus a bit more on what makes your code Pythonic, espeically since janos already answered on that a bit.

Try to separate out the single line summary of your docstring with a blank line, it's more readable and used in some automated parsers.

"""A single question with a unique correct answer.

Attributes:
block: string or None; string if this question belongs to block of
questions, where the string is unique identifier for that block.


Using variable names in all caps is usually reserved for constants. You should make these lowercase instead. block wont clash with self.block, but if you insist on different names then don't use caps to delineate.

You could change your if check to a ternary, this is a debatable improvement but I like using them in cases like this:

              # value1  statement   value2
self.block = block if block else None


It evaluates as value1 if statement == True, otherwise it evaluates as value2.

ask and ans are good clean functions. I'd recommend adding brief docstrings clarifying the data they return. And just use the name answer. Shortening for 3 letters is rarely worth the slight clarity loss.

In grade, you could set tempans['getRight'] to just equal the expression you're using to test. It's easier to read because it's clear that the value gets set directly to that expression.

def grade(self, studAns):
tempans = self.ans()
tempans['gotRight'] = studAns == self.correct
return tempans


I'm going to contradict janos here, but Python doesn't care much for getters and setters. It's Pythonic to access attributes directly. So if you want to get the value of some_question's block attribute, just read it with some_question.block. You can do the same with setting it too, just set some_question.block = some_value. If you really want to make a value private and inaccessible outside the class, the common style is add an underscore to the start of the name (ie. _block). That doesn't affect how it works, but signals to another coder that you don't want it accessed externally.

Python operates on the idea that it shouldn't restrict you to using getters and setters, instead letting you do what you want. So you may as well operate within those boundaries because it's complicated to actually enforce any kind of private variable system in Python.

Of course the exception to this is if you ever need to adjust data before returning/assigning it. Or if you need to validate values before setting them to attributes. In these cases it makes perfect sense, but it's entirely unnecessary to use functions for plain getters and setters in Python.

Also with dictRepr, you should name it dict_repr as the Python naming convention is snake_case, not camelCase. That said, I'd name it something other than repr as that seems similar to the common function name __repr__. Use dict_format instead perhaps?