# Quiz Engine implementation

I'm trying to find a nice API for a Quiz Engine. This is what I have so far:

Implementation:

public class Question
{
public string Title { get;set; }

}

{
public bool IsCorrect { get; set; }
public string Content { get; set; }
}

public class QuizzEngine{
Queue<Question> questions;

public QuizzState QuizzState { get; set; }

public QuizzEngine(IEnumerable<Question> questions)
{
this.questions = new Queue<Question>(questions);
QuestionCount = this.questions.Count;
}

void StartQuizz()
{
QuizzState = QuizzState.Going;

while(QuizzState == QuizzState.Going)
{
MoveToNextQuestion();
if(QuizzState == QuizzState.Going) return;
}
}

{
}

{
// somehow gets Answer through the UI
}

{
}

public void MoveToNextQuestion()
{
if(questions.Count == 0)
{
QuizzState = QuizzState.Finished;
CurrentQuestion = null;
return;
}

CurrentQuestion = questions.Dequeue();
}

public Question CurrentQuestion { get; private set; }
public int CorrectAnswerCount { get; private set; }
public int QuestionCount { get; private set; }
}

public enum QuizzState{
Going,
Finished
}


How could this be improved?

• One tiny minor point: It's spelled "Quiz" – Nick Udell Oct 14 '14 at 12:04
• @NickUdell That's embarrassing, but let's ignore that for now. – Raz Megrelidze Oct 14 '14 at 12:53

Good

• Meaningful names are used for classes, methods, parameter and properties
• using interfaces over implementation
• using enums to signal a state

Not so good

• Code isn't tested
• Scope of property setters is public where they should be private
• Using of properties which are never used
• Single responsibility principle is violated

The good things are said, so let us focus on the bad things.

Code isn't tested
How / where should one see this ?

void StartQuizz()
{
QuizzState = QuizzState.Going;

while(QuizzState == QuizzState.Going)
{
MoveToNextQuestion();
if(QuizzState == QuizzState.Going) return;
}
}


Here, after calling MoveToNextQuestion(), the code is checking the QuizzState property. So we assume the property has changed and if we take a look at this said method we see,

public void MoveToNextQuestion()
{
if(questions.Count == 0)
{
QuizzState = QuizzState.Finished;
CurrentQuestion = null;
return;
}

CurrentQuestion = questions.Dequeue();
}


that the property is changed to QuizzState.Finished, if the question queue is empty. But the check is about if(QuizzState == QuizzState.Going) return;. So after the first call to MoveToNextQuestion() the code exists the while loop.

Scope of property setters and never used properties

As one of many samples let us take a look at the Answer class, so we can kill 2 birds with one stone

public class Answer
{
public bool IsCorrect { get; set; }
public string Content { get; set; }
}


The IsCorrect property is nowhere used in the whole code. But if it would be used somewhere, why should we allow to set / change the value of this property from a public place ? Especially for a Q&A engine, there shouldn't be a possibility to cheat !

Single responsibility principle

Where is the SRP viaolated ? Here

void GetAnswerFromUI(out Answer answer)
{
// somehow gets Answer through the UI
}


The QuizEngine shouldn't be responsible for any UI related stuff. But wait, how are we supposed to receive the answers and also how should we notify another object ( like an UI ) about the next question ?

That is what events and methods or properties are for.
To signal a changed state, like there is a question to be answered, an event should be used by the QuizEngine.
To receive an answer for a question, the QuizEngine should provide either a property setter or a method.
For decoupling the QuizEngine and the UI the QuizEngine should implement an interface which provides the event and the answer setting method/property.

• The IsCorrect property is nowhere used in the whole code I would probably implement INotifyPropertyChanged and then bind the IsCorrect property to a radio button. And i agree that GetAnswerFromUI was stupid. Forgot to make StartQuizz public. – Raz Megrelidze Oct 14 '14 at 12:39
• if(QuizzState == QuizzState.Going) return; the if statement should check if QuizzState == QuizzState.Finished. – Raz Megrelidze Oct 14 '14 at 12:51

A few points on top of @Heslacher's excellent review.

Another violator of the single responsibility principle is the MoveToNextQuestion method, since it does two things:

1. Moves to the next question

When a method does something in addition to what its name suggests, that becomes a burden on the programmer: since the second action is not in the name, you have to remember it, or you have to re-read the implementation to be reminded.

When there is a single method like this, it makes you question the entire class: you cannot trust any of the methods, who knows what their implementations might be hiding?

This method should really have been simply this:

public void MoveToNextQuestion()
{
CurrentQuestion = questions.Dequeue();
}


But then, when do you update the game state? Technically, the very action of moving to the next question is in fact a change in the game state. At least for now, it seems that looking at the state of the questions queue should be enough to check and manage the game state. In particular, it will make sense to add a method for this:

public boolean HasNextQuestion()
{
return questions.Count != 0;
}


With this, StartQuizz can be simpler:

public void StartQuizz()
{
while (HasNextQuestion())
{
MoveToNextQuestion();
}
}


Notice that the original StartQuizz method was also violating SRP, as it was messing with the game state too. Now that's all gone: this method doesn't need to know how the state is kept, it just needs to know to run as long as there is a next question.

As a result, the QuizzState enum is no longer needed.

I fear the QuestionCount property can easily become a burden:

public QuizzEngine(IEnumerable<Question> questions)
{
this.questions = new Queue<Question>(questions);
QuestionCount = this.questions.Count;
}

public int QuestionCount { get; private set; }


What if you change something later in the behavior of the questions queue? You will have to remember to adjust QuestionCount accordingly. Instead of setting this property in the constructor, it would be better to make this a method returning questions.Count.

Why think in terms of procedures?

void GetAnswerFromUI(out Answer answer)
{
// somehow gets Answer through the UI
}


How about converting this procedure with a single output parameter to a function?

Answer GetAnswerFromUI()
{
// somehow gets Answer through the UI

This will also simplify the GetAnswer method:
Answer GetAnswer()