Simple Quiz game with Strategy and Factory pattern

Question

I have a simple quiz game where user can give simple answers to simple questions and program will give a result. Please, guys, make a code review and if it is possible answer to my questions placed at the bottom.

Model classes looks like that:

public class Answer
{
 public Answer(int id, int questionId, string content)
 {
 Id = id;
 Content = content;
 QuestionId = questionId;
 }
 public int Id { get; private set; }
 public int QuestionId { get; private set; }
 public string Content { get; private set; }
}
public class Question
{
 public Question(int id, string content, char answer)
 {
 Id = id;
 Content = content;
 Answer = answer;
 }
 public int Id { get; private set; }
 public string Content { get; private set; }
 public char Answer { get; private set; }
}

Abstractions:

interface IAnswerService
{
 List<IList<Answer>> GetAnswers();
}
interface IQuestionService
{
 IList<Question> GetQuestions();
}
public interface ICalculationService
{
 int CalculatePoints(Dictionary<Question, char> userAnswers); 
}

Constants:

public static class Constants
{
 public static class Answers
 {
 public const char A = 'a';
 public const char B = 'b';
 public const char C = 'c';
 public const char D = 'd';
 }
}

Core services:

static class Factory
{
 public static T CreateInstance<T>() where T : new()
 {
 return new T();
 }
}

Services:

public class AnswerService : IAnswerService
{
 public List<IList<Answer>> GetAnswers()
 { 
 return new List<IList<Answer>>() {
 new List<Answer>() { new Answer(11, 3, "Sequoia"), new Answer(12, 3, 
 "Berch"), new Answer(13, 3, "Lindens"), new Answer(14, 3, "Alder") },
 new List<Answer>() { new Answer(1, 1, "1"), new Answer(2, 1, "2"), 
 new Answer(3, 1, "5"), new Answer(4, 1, "6") },
 new List<Answer>() { new Answer(7, 2, "More than 1"), new Answer(
 8, 2, "More than 2"), new Answer(9, 2, "More than 5"),
 new Answer(10, 2, "More than 6") }, 
 new List<Answer>() { new Answer(15, 4, "yes, I do!"), new 
 Answer(16, 4, "Sure!"), new Answer(17, 4, "Exactly"), new 
 Answer(18, 4, "Yeap!") }
 };
 }
}

CalculationAnswerByAdding class of Services:

class CalculationAnswerByAdding : ICalculationService
{
 public int CalculatePoints(Dictionary<Question, char> userAnswers)
 {
 var sum = 0;
 foreach (var question in userAnswers)
 {
 if (question.Key.Answer == question.Value)
 sum += 1;
 }
 return sum;
 }
}

CalculationAnswerBySubtracting of Services:

class CalculationAnswerBySubtracting : ICalculationService
{
 public int CalculatePoints(Dictionary<Question, char> userAnswers)
 {
 var sum = 10;
 foreach (var question in userAnswers)
 {
 if (question.Key.Answer == question.Value)
 sum -= 1;
 }
 return sum;
 }
}
public class QuestionService : IQuestionService
{
 public IList<Question> GetQuestions()
 {
 return new List<Question>() {
 new Question(1, "How many are there contintents?", 
 Constants.Constants.Answers.A),
 new Question(2, "How many are there colours?", Constants.Constants.Answers.B),
 new Question(3, "What is the tallest tree?", Constants.Constants.Answers.C),
 new Question(4, "Do you like dolphins?", Constants.Constants.Answers.D),
 };
 }
}

And Program.cs:

Main() method of Program.cs:

static void Main(string[] args)
{
 IQuestionService questionService = Factory.CreateInstance<QuestionService>(); 
 var questions = questionService.GetQuestions();
 IAnswerService answerService = Factory.CreateInstance<AnswerService>();
 var answers = answerService.GetAnswers();
 var questionAnswers = questions.ToDictionary(q => q,
 q => answers
 .SelectMany(a => a)
 .Where(b => b.QuestionId == q.Id)
 .ToList());
 var userAnswers = new Dictionary<Question, char>();
 GetAsnwers(questionAnswers, userAnswers);
 ICalculationService calculationService = Factory
 .CreateInstance<CalculationAnswerByAdding>(); 
 var userSum = calculationService.CalculatePoints(userAnswers);
 Console.WriteLine(userSum > 3 ? $"Yeah, it is great. Your points are {userSum}." 
 : $"Hey, it is great. Your points are {userSum}");
}

GetAsnwers() method of Program.cs:

 private static void GetAsnwers(Dictionary<Question, List<Answer>> 
 questionAnswers, Dictionary<Question, char> userAnswers )
 {
 foreach (var questionAnsw in questionAnswers)
 {
 AskQuestion(questionAnsw);
 List<char> allowedAnswers = new List<char>()
 {
 Constants.Constants.Answers.A,
 Constants.Constants.Answers.B,
 Constants.Constants.Answers.C,
 Constants.Constants.Answers.D,
 };
 while (true)
 {
 var userKey = Console.ReadKey().KeyChar;
 Console.WriteLine();
 if (!allowedAnswers.Contains(userKey))
 {
 AskQuestion(questionAnsw, true);
 }
 else
 {
 userAnswers.Add(questionAnsw.Key, userKey);
 break;
 }
 }
 }
 }

AskQuestion() method of Program.cs:

private static void AskQuestion(KeyValuePair<Question, List<Answer>> questionAnswer, 
 bool showPossibleKeys = false)
{
 if (showPossibleKeys) {
 Console.WriteLine();
 Console.WriteLine("Possible keys are A, B, C or D");
 }
 Console.WriteLine(questionAnswer.Key.Content);
 questionAnswer.Value
 .ForEach(a => Console.WriteLine(a.Content));
 }

Guys, please, see my code. Is my code solid? Do I correctly use strategy and factory patterns? Any improvements and comments are greatly appreciated.

In addition, do my classes organized well? Especially, is it okay that I moved Factory into folder CoreServices? Or is there a better way to organize classes? enter image description here

Answer 1

Instead of asking 'did I use pattern X correctly?', I would ask 'does pattern X provide a good solution to my problem?'. And to answer that question, you need to consider the requirements of your program. Why do you think this program needs strategies, services and factories?

Model classes

• Regarding your model classes, why do answers reference questions by their id? Note how this makes things more cumbersome for the caller: Main has to do extra work to match answers to questions.
• Why do questions refer to their correct answer with a char? Not only is that inconsistent with the use of ids, it's also error-prone because it depends on the order in which answers are returned. Such labeling is probably best left to the UI layer, anyway.
• Read-only properties don't need a private setter anymore: { get; } is sufficient nowadays.

I would go for the following (I've left out constructors for brevity's sake, and ids because they don't seem to be necessary):

public class Question
{
 public string Content { get; }
 public IReadOnlyCollection<Answer> AvailableAnswers { get; }
 public Answer CorrectAnswer { get; }
}
public class Answer
{
 public string Content { get; }
}

Note that the Answer class now only contains a string, so you could remove it and store a collection of answer strings in Question directly, together with the index of the correct answer.

Services

• Why split questions and answers into two 'services'? Questions and answers are often, if not always, used together, so this split is making things more difficult than they need to be. It's also more error-prone, because now you need to keep two separate services in sync. I would merge these into a single question repository.
• Both calculation 'service' implementations do the same work, more or less. I'd probably create some kind of Result class, that can be used to keep track of how many questions a user answered correctly and incorrectly. That'll give these calculation algorithms a higher-level overview, and reduces code duplication.
• I'm not really sure whether abstracting the scoring mechanism is really useful, but I don't know what your plans are with this program, so I can't say much about that. That hard-coded 10 in CalculationAnswerBySubtracting does look problematic though - certainly that should be configurable?

Factories and constants

• That factory class doesn't add any value over using new directly. If it's intended to simulate a DI container: normally you ask a container for a concrete implementation of an 'abstract' interface, not the other way around.
• Those constants are verbose but don't really clarify anything. Answers.A and 'A' are both equally vague ways to reference an answer. Labeling questions like this is a UI detail that shouldn't leak into the rest of the program. Besides, with these constants you pretty much hard-coded a limit of 4 answers per question, which seems like an unnecessary restriction.
• AskQuestion doesn't use those constants, and because their values are lower-case, not upper-case, this can result in seemingly valid inputs being rejected (try pressing shift + A instead of just hitting the A key).

Other notes

• Personally I don't like those Service and Factory suffixes. I would use names like IQuestionRepository and IScoreCalculator - more descriptive, less cluttered.
• In foreach (var question in userAnswers), question isn't actually a Question but a key-value pair, so that name is a bit misleading. A dictionary is probably not the best way to pass this information around anyway - it doesn't preserve order, and you don't need its lookup functionality, you just need a collection of question-answer pairs.
• GetAsnwers 'returns' results by modifying one of its arguments... why doesn't it just return its results, which is how most programmers would expect it to work?
• Using Question as a key in a dictionary, without overriding Equals and GetHashCode, can cause problems. Because right now you're not doing any key lookups with different (but equal) Question instances (no key lookups at all, in fact) the current code works fine, but this could easily cause problems in the future.

• StepUp: Thanks for so thorough and great answer! I really like your answer. I agree with you. However, I have a question. My intention to use Id properties in model classes such as Answer and Question is to map them accordingly their id's from database. How to create a better classes for mapping them from database?
• Pieter Witvoet: Your Question and Answer classes are used throughout the program, and since nothing there needs those ids I (currently) see no need for those properties. If you're loading them from a database then they'll likely have row ids, yes, but that's an implementation detail that only the question repository needs to know about. How to load or map them... that's a different question.