patterncsharpMinor
Quiz Engine implementation
Viewed 0 times
enginequizimplementation
Problem
I'm trying to find a nice API for a Quiz Engine. This is what I have so far:
Implementation:
How could this be improved?
Implementation:
public class Question
{
public string Title { get;set; }
public List Answers { get; set; }
public Answer CorrectAnswer { get; set; }
}
public class Answer
{
public bool IsCorrect { get; set; }
public string Content { get; set; }
}
public class QuizzEngine{
Queue questions;
public QuizzState QuizzState { get; set; }
public QuizzEngine(IEnumerable questions)
{
this.questions = new Queue(questions);
QuestionCount = this.questions.Count;
}
void StartQuizz()
{
QuizzState = QuizzState.Going;
while(QuizzState == QuizzState.Going)
{
MoveToNextQuestion();
if(QuizzState == QuizzState.Going) return;
Answer answer = GetAnswer();
TryAcceptAnswer(answer);
}
}
Answer GetAnswer()
{
Answer answer;
GetAnswerFromUI(out answer);
return answer;
}
void GetAnswerFromUI(out Answer answer)
{
// somehow gets Answer through the UI
answer = null;
}
public void TryAcceptAnswer(Answer answer)
{
var isCorrectAnswer = CurrentQuestion.CorrectAnswer.Equals(answer);
if(isCorrectAnswer) CorrectAnswerCount++;
}
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?
Solution
Good
Not so good
The good things are said, so let us focus on the bad things.
Code isn't tested
How / where should one see this ?
Here, after calling
that the property is changed to
Scope of property setters and never used properties
As one of many samples let us take a look at the
The
Single responsibility principle
Where is the SRP viaolated ? Here
The
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
To receive an answer for a question, the
For decoupling the
- 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;
Answer answer = GetAnswer();
TryAcceptAnswer(answer);
}
}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
answer = null;
}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.Code Snippets
void StartQuizz()
{
QuizzState = QuizzState.Going;
while(QuizzState == QuizzState.Going)
{
MoveToNextQuestion();
if(QuizzState == QuizzState.Going) return;
Answer answer = GetAnswer();
TryAcceptAnswer(answer);
}
}public void MoveToNextQuestion()
{
if(questions.Count == 0)
{
QuizzState = QuizzState.Finished;
CurrentQuestion = null;
return;
}
CurrentQuestion = questions.Dequeue();
}public class Answer
{
public bool IsCorrect { get; set; }
public string Content { get; set; }
}void GetAnswerFromUI(out Answer answer)
{
// somehow gets Answer through the UI
answer = null;
}Context
StackExchange Code Review Q#66587, answer score: 8
Revisions (0)
No revisions yet.