5
\$\begingroup\$

I posted a recent question on code review detailing my previous C# object-oriented application which used animals as the main context. Although this example doesn't really include inheritance or anything similar, I did try and create something a little bit more interactive. Maybe, just maybe this could be practical for a quiz application?

What could I improve? What other features could I add and maybe demonstrate inheritance with?

    public class Question
    {
        public int QuestionNumber { get; set; }
        public string Description { get; set; }
        public string Answer { get; set; }
    }
    public class Quiz
    {
        public void AddQuestion(Question q)
        {
            q.QuestionNumber = Questions.Count;
            q.Description += " (" + (q.QuestionNumber + 1) + ")".ToString();
            Questions.Add(q);

            if (Questions.Count > QuizLength)
            {
                Console.WriteLine("You have too many questions, please change your QuestionLength.");
            }
        }
        public void ReadQuestions(int customIndex = 0)
        {
            if(customIndex == QuizLength) 
            {
                Console.WriteLine(Environment.NewLine + "Quiz has finished."); 
                for(int x = 0; x < Questions.Count; x++)
                    Console.WriteLine($"Question {x + 1} - {Questions[x].Description} | Answer - {Questions[x].Answer}");
                Console.WriteLine($"You got a score of {Score}/{QuizLength}");
            }
            else
            while(customIndex < Questions.Count)
            {
                Timer timer = new Timer(QuestionTimeLimit);
                timer.Start();
                timer.Elapsed += (sender, e) =>
                {
                    Console.WriteLine("Time limit over...");
                    timer.Stop();
                    ReadQuestions(customIndex + 1);
                };

                Console.WriteLine(Questions[customIndex].Description);
                string input = Console.ReadLine();
                if(input.ToLower().Trim() == Questions[customIndex].Answer.ToLower().Trim())
                {
                    Console.WriteLine("Correct answer");
                    timer.Stop();
                    customIndex++;
                    Score++;
                }
                else
                {
                    Console.WriteLine("Incorrect answer");
                    break;
                }
            }
        }
        public List<Question> Questions = new List<Question>();
        public int Score { get; set; }
        public int QuizLength { get; set; }
        public bool HasTimeLimit { get; set; }

        private int _QuestionTimeLimit;
        public int QuestionTimeLimit
        {
            get
            {
                return _QuestionTimeLimit;
            }
            set
            {
                if (HasTimeLimit == true)
                    _QuestionTimeLimit = value;
                else
                    Console.WriteLine("You haven't assigned a time limit value.");
            }
        }
        static class Program
        {
            static void Main(string[] args)
            {
                Quiz q = new Quiz()
                {
                    HasTimeLimit = true, QuizLength = 4, QuestionTimeLimit = 30000,
                };
                q.AddQuestion(new Question() {  Description = "What is the capital of Canada?", Answer = "Ottawa", });
                q.AddQuestion(new Question() { Description = "What is the capital of Australia?", Answer = "Canberra", });
                q.AddQuestion(new Question() { Description = "What is the capital of New Zealand?", Answer = "Wellington", }); 
                q.AddQuestion(new Question() { Description = "What is the capital of Mexico?", Answer = "Mexico City", });
                q.ReadQuestions();

                Console.ReadLine();
            }
        }

I've had fears that maybe this code is a bit messy, but that might just be me. If so, (or if you think so) how could I make it cleaner?

Could I improve this question? Am I missing anything? Tell me in the comments if you think so.

Thanks,

\$\endgroup\$
0

3 Answers 3

4
\$\begingroup\$

There's a lot of juggling going on here, and I'm not convinced all of it is needed.

Stringing a string

q.Description += " (" + (q.QuestionNumber + 1) + ")".ToString();

The ToString() is wholly irrelevant here. ")" is already a string, there's no point to convert it to a string again.

As a matter of preference, I would advocate using string interpolation here:

$"({q.QuestionNumber + 1})"

It's generally cleaner than string concatenation.

Business logic vs UI logic

q.QuestionNumber = Questions.Count;
q.Description += " (" + (q.QuestionNumber + 1) + ")".ToString();

The question q already contains the QuestionNumber, so you don't gain anything from also appending it to the question description.

Appending that question number can be done in the UI logic, and it shouldn't be foisted into your actual data entity. You're muddying the lines between your business logic and your view/UI logic.

So I would skip the entire q.Description += ... line, and move that to the actual UI if it is needed:

var q = Questions[customIndex];
Console.WriteLine($"q.Description ({q.QuestionNumber + 1})");

Numbering

You have multiple ways of numbering your questions and you jump from one to the other. There is no consistency in your approach.

Your questions track a QuestionNumber property, but when you want to print the question number, you instead rely on an index counter:

for(int x = 0; x < Questions.Count; x++)
    Console.WriteLine($"Question {x + 1} - ...");

The purpose of QuestionNumber is being undone completely by never meaningfully using it. Either don't track it to begin with, or actually use it.

In this case, I would advocate for not tracking it. A question's number is based on its position in the list of questions, so it makes more sense to derive the question number based on its position in the list, instead of individually tracking it.

Pointless hurdles, duplicate data & creating your own problems

Questions.Add(q);

if (Questions.Count > QuizLength)
{
    Console.WriteLine("You have too many questions, please change your QuestionLength.");
}

QuizLength seems to be used for two things:

  1. "Preventing" the addition of more questions

What is the purpose of QuizLength, if not to enforce the maximum amount of questions that you can add?

Yet you put no restriction on the actual questions that get added. You just print a message to the console but then add the question anyway. That's a lot of bark with no bite.

If you wish to cap the question limit, then don't add excess questions to the list:

if (Questions.Count < QuizLength)
{
    Questions.Add(q);
}
else
{
    Console.WriteLine("Question limit reached!");
}
  1. Knowing how many questions to cycle over

You don't need to store that in a separate variable. Questions.Count already tells you how many questions there are.

Over all, keeping separate track of the quiz length makes no sense here. It creates a pointless arbitrary validation check that you have to comply with, and in all other cases it causes problems that would not exist if the validation check didn't exist to begin with.

  • If you end up with less questions than the defined quiz length, your code is going to blow up. Had you relied on Questions.Count instead of a separately tracked QuizLength, that would not have happened.
  • If you end up with more questions than the defined quiz length, then why block the creator from adding the extra questions, since they were allowed to decide their own quiz length anyway?
  • If you end up with more questions and don't block the addition of more questions, then your code is going to ignore asking the extra questions as QuizLength will cause it to stop before all questions have been asked.

If you simply remove QuizLength, let the quiz creator add however many questions they want without trying to impose a limit, and then use Questions.Count as a way to track your quiz length, then all these problems wouldn't exist.

Be very wary of adding features that cause problems instead of solving them. QuizLength is one of those.

More arbitrary hurdles!

HasTimeLimit is another one of those arbitrary and pointless hurdles to cross.

public int QuestionTimeLimit
{
    // ...
    set
    {
        if (HasTimeLimit == true)
            _QuestionTimeLimit = value;
        else
            Console.WriteLine("You haven't assigned a time limit value.");
    }
}

I don't understand your reasoning here. You're being given a time limit (since the consumer is setting the time limit value), and your code is obtusely responding with "but I haven't been told that a time limit was going to be set!".

The fact that someone is currently setting the time limit shouldn't be blocked by some arbitrary expectation that this person did not pre-emptively announce that they were going to set the time limit.

Your logic is the equivalent of the following conversations:

Customer: "Hi, I would like to buy one of your Foobars please."
Shopkeeper: "I wasn't told whether you'd like to buy a Foobar, so I won't sell you one".

The logical response is "but I'm telling you right now that I want to buy one!". The current conversation inherently contradicts the shopkeeper's assumption.

Similarly, the response to your "You haven't assigned a time limit value" message is that I am telling you right now that I do want to set a time limit. The current context (setting the time limit) inherently contradicts your code's reasoning that the time limit wasn't going to be set.

You're just creating extra hurdles, which is just going to trip you up more, for no benefit at all.

Don't get me wrong, it happens to every developer that they build something, change it, and then it turns out that the thing they though they needed has now unexpectedly become unnecessary. On top of that, I surmise that you're a beginner who is still experimenting with their code before settling on an end result.

Both of those are perfectly reasonable explanations as to how this code came to be, and neither of them make you a bad developer.

But you really need to re-evaluate your logic and spot those issues, because if you don't clean up after yourself, you're just making your own life harder for no reason at all.

Learn to sanity-check yourself. Look at your finished work, and evaluate if all of its parts are actually adding value to the end product. Had you done so, HasTimeLimit should've popped up as a part that doesn't add value.

Double-stacking your methods

public void ReadQuestions(int customIndex = 0)
{
    if(customIndex == QuizLength) 
    {
        // print results
    }
    else
        // ask ALL the questions
    }
}

The method has two completely distinct purposes. These two purposes should be separated into two distinct methods, e.g. AskQuestions and PrintResults

Recursion or iteration?

public void ReadQuestions(int customIndex = 0)
{
    // ...
    while(customIndex < Questions.Count)
    {
        // ...
        ReadQuestions(customIndex + 1);
        // ...
    }
}

For the life of me, I cannot comprehend the flow you're intending to take here. You're iterating over a collection (since you're using a while iterator), but at the same time you're also recursively calling ReadQuestions, and each recursive step itself uses a while iterator!

That makes no sense. You either iterate or you recurse. Not both.

It seems you're using the while as a simple way to break your recursion. There's no need for that to iterate, you can just use an if here. That is, if you are to use recursion.

However, you shouldn't be using recursion here. This isn't an n-depth problem where the result of one step is the input of the next step.

This is a simple iteration over a list of questions, and it would be tremendously more readable if you simplify the algorithm to just that.

public void Run()
{
    int totalScore = 0;

    foreach(var question in Questions)
    {
        var questionScore = Ask(question);
        totalScore += questionScore;
    }

    PrintResults(totalScore);
}

Before we delve into the Ask and PrintResults methods, look at how simple and straightforward the structure of this algorithm is. Ask each question, then print the end result.

Compare this to your code, where you have a recursive method with an interation in each step, and custom written end clause where a completely different task (printing the results) is being run within the same recursive stack.

private int Ask(Question q)
{
     Timer timer = new Timer(QuestionTimeLimit);
     timer.Elapsed += (sender, e) =>
     {
         Console.WriteLine("Time limit over...");
         return 0;
     };

     Console.WriteLine(question.Description);
     timer.Start();
            
     string input = Console.ReadLine();
     timer.Stop();
     
     if(input.ToLower().Trim() == Questions[customIndex].Answer.ToLower().Trim())
     {
         Console.WriteLine("Correct answer");
         return 1;
     }
     else
     {
         Console.WriteLine("Incorrect answer");
         return 0;
     }
}

private void PrintResults(int totalScore)
{
    Console.WriteLine("Quiz has finished."); 

    foreach(var question in Questions)
    {
        Console.WriteLine($"Question {question.QuestionNumber + 1} - {question.Description} | Answer - {question.Answer}");
    }

    Console.WriteLine($"You got a score of {totalScore}/{Questions.Count}");
}

Further improvements can be made here, such as not using Console.Writeline inside your business logic class, but I'm going to consider those improvements off-topic for the current skill level of the exercise at hand.


What other features could I add and maybe demonstrate inheritance with?

You don't buy bread because you have a bread knife in the kitchen. You buy a bread knife because you have bread.

Similarly, you don't write code with the sole purpose of having it use inheritance. You use inheritance when you are writing code that warrants the use of inheritance (to improve the code).

This code example has no use for inheritance whatsoever. There are no variant objects here. You have a quiz, and you have questions. Those are two completely different objects that have no feasible inheritance relationship.

That being said, additional features that would warrant inheritance would include:

  • Different kinds of questions (multiple choice, true/false, open ended, ...)
  • Different kinds of quizzes (ask all questions, pull random questions from a larger question repository)
  • Different kinds of score calculations (flat rate per question, bonus points for correct answer streaks, penalties for incorrect answer streaks, ...)
  • Different kinds of users (quiz takers, quiz creators, admins)

In general, inheritance is used in cases where there are multiple variations on the same idea. Everything that is shared between the variations should be inherited from their common ancestor, and everything that is unique to a specific variation should belong to that variation alone.

\$\endgroup\$
0
1
\$\begingroup\$
  1. Readability / Style

You could start with making the code more readable, by running it through a formatter such as this one https://codebeautify.org/csharpviewer , and read up on code style and standards for C#.

  1. Repetition

Also, instead of repeating q.AddQuestion(new Question() { Description = over and over, you could put your questions and answers in a map and then iterate over the map to add questions. This makes it more flexible when you want to add many questions, or possibly read from user input / file input, etc.

https://stackoverflow.com/questions/1598070/c-sharp-equivalent-of-c-mapstring-double

  1. Inheritance

Also, why do you want to "demonstrate inheritance" ? For its own sake? Or is this an assignment that requires you to demonstrate inheritance?

\$\endgroup\$
1
\$\begingroup\$

I do not want to re-iterate the points of the other answers, but just give you an idea on how you could introduce inheritance.

You could introduce different types of questions, like:

  • yes/no questions
  • multiple choice questions
  • plain text questions (like: what is the name of the highest mountain in ...?).
  • number questions (like: what is the height of the mount Everest? where you must check that the result is within a certain range).

These questions require different ways to be asked (e.g. a multiple choice question will have to print a list of numbered choices) and their answers to be evaluated (e.g. a yes/no question could test for the answers "Y", "y", "yes", "yeah" etc.).

It is then obvious to formulate the questions and answers as objects in a class hierarchy as a demonstration of polymorphism.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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