6
\$\begingroup\$

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 List<Answer> 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<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;
            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?

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

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;
        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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Raz Megrelidze Oct 14 '14 at 12:39
  • \$\begingroup\$ if(QuizzState == QuizzState.Going) return; the if statement should check if QuizzState == QuizzState.Finished. \$\endgroup\$ – Raz Megrelidze Oct 14 '14 at 12:51
7
\$\begingroup\$

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
  2. Updates quiz state

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();
        Answer answer = GetAnswer();
        TryAcceptAnswer(answer);
    }
}

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
    answer = null;
}

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

Answer GetAnswerFromUI()
{
    // somehow gets Answer through the UI
    answer = null;
    return answer;
}

This will also simplify the GetAnswer method:

Answer GetAnswer()
{
    return GetAnswerFromUI();
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.