# Simple math quiz in C#

I am only a beginner when it comes to programming, but after reading tons of tutorials on and offline, I have been able to create this simple math quiz. It does work as intended, and I'm really just fishing for someone to yell at me for doing something glaringly bad practice in the code below.

using System;

namespace ForthConsoleProject
{
class MainClass
{
public static void Main(string[] args)
{
Random randomgen = new Random();

//int timer = 30; I considered a timer function but that ended up being way over my head.
int num01 = randomgen.Next(11);
int num02 = randomgen.Next(11);
int numofquestions;
int numofquestionsleft;
int numofcorrect = 0;

Console.Write("How many questions would you like to answer? ");
numofquestionsleft = numofquestions;

// This is the loop which handles the actual question/answer core of the game.
// Answering a question correctly increases your score.
while (numofquestionsleft > 0){

Console.Write("What is " + num01 + " times " + num02 + "? ");
answer = num01 * num02;
{
Console.WriteLine(playeranswer + " is correct!");
numofcorrect++;
}
else {
Console.WriteLine(playeranswer + " is incorrect! Try again.");
}
numofquestionsleft--;
num01 = randomgen.Next(11);
num02 = randomgen.Next(11);
}
// Letting the user know how many answers they got right.
Console.WriteLine("You got " + numofcorrect + " of " + numofquestions + " correct!");

}
}
}

• Try this exercise: Your boss walks in and demands a test where people are forced to practice: addition, subtraction and division as well. How would you rewrite the above code? Remember you don't want to repeat yourself too much, and you want your code to be easily understandable. If you complete the above exercise then you will be x100 better than most programmers out there. – BKSpurgeon Oct 7 '16 at 6:37
• @BKSpurgeon Can they choose which operation to practice or must they practice at random? – Clol7 Oct 7 '16 at 6:44
• it's up to you! The beauty of exercises like the above is that you are forced to THINK, and you will fail your way to success. i am writing my own solution (but it's in ruby) using TDD to answer this very question you asked. i've myself started looking into actually DESIGNING good code (as opposed to merely programming) ............what you find is that good code allows for easy changes........but bad code will repeat the above 1000 times, be error prone and difficult to change. Start with TDD basics, fail the tests and pass. repeat until you have nicely 'abstracted' code. – BKSpurgeon Oct 7 '16 at 8:09
• @BKSpurgeon I've finished the exercise in the style of the above code, I haven't even finished learning what I can from the answers below: – Clol7 Oct 7 '16 at 9:17
• @Clol7 It is Code Review Policy to not edit the question after an answer has been posted, as it invalidates the answers. I reverted your edit, but feel free to post a new and improved question, tho. – asibahi Oct 7 '16 at 9:28

This is pretty nice. I got some comments, some of which are my personal opinion (for example how to declare variables), some are accepted conventions (naming), and some are bug fixes.

# Declaring variables

While it might be more performance friendly to declare all variables from the outset, it is best practice, I think, to declare variables only within the scope they're used. This is to decrease variable pollution in bigger functions. In such a simple app, it doesn't matter. But as your application gets bigger and better you don't want too many variables in scope.

From your code: num01, num02, playeranswer, and answer are only used within the loop. So it is better to declare then inside the loop (or you might accidentally use them outside or simply crowd your intellisense).

Also, for some variables you're splitting declaration (int numofquestions;) and assignment (numofquestions = Convert.ToInt32(Console.ReadLine());), and for other you're not. I think it is better to have declaration and assignment in one line, it is terser and more readable: (int numofquestions = Convert.ToInt32(Console.ReadLine());).

# Naming

.Net convention is to use PascalCase for methods and classes and the like, and camelCase for variables. So, in your code: playeranswer becomes playerAnswer, and so on. You don't have any methods defined but you need to know the convention. It makes it easier to read your code. Again, it doesn't really matter for such a short app but for bigger apps such practices are a life saver. (A crude example, but I am sure you have seen the photo where KidsExchange reads like KidSexChange due to uniform capitalization KIDSEXCHANGE.)

I also like questionCount more than numOfQuestions. Shorter and uses clearer vocab.

# Using a for loop

for loops are easier for humans to process than while loops, I think. Well easier for me.

So this :

int questionCount= Convert.ToInt32 (Console.ReadLine ());
int questionLeftCount = questionCount;

while (questionLeftCount > 0)
{
// .. do stuff
questionLeftCount --;
}


Can be better written as:

int questionCount= Convert.ToInt32 (Console.ReadLine ());

for (int questionLeftCount = questionCount; questionLeftCount > 0; questionLeftCount--)
{
// do stuff ..
}


And it is exactly the same.

Also, since questionLeftCount is only used within the for loop definition, just use i or iter for iterator. It is standard practice. (It also standard practice to increment rather decrement). I am sure you have seen something that looks exactly like this in your readings.

for (int i = 0; i < questionCount; i++)
{
// do stuff ..
}


Much shorter and nicer.

# Verifying input

Your code crashes when I type something that is NOT a number. Obviously if you're shipping the app (I know you're not, but humor me), you can't have that.

The trick is to use int.TryParse to parse inputs. TryParse returns a bool to indicate whether the conversion is successful or not, and has an out variable which assigns a value to an already declared variable. The syntax is a bit weird and takes some getting used to, but it is being made nicer in C# 7.0 (not released yet).

// Usage

{
// do stuff with playerAnswer
}
else
{
// show error message
}


If you want to keep asking for input until you get something valid, you can use a while loop :

 int playerAnswer;

{
Console.WriteLine ("Invalid Input. Please type a number using digits");
}


Something of note: you need this little piece of code in two places. When you ask for the number of questions and when you ask for the answer. Instead of typing it twice, you should extract it into a method. You can easily do that in Visual Studio by selecting the code you want to extract into a method, pressing Ctrl + . or the small light bulb to the side, and selecting, well, Extract Method. It does all the thinking for you and it is fantastic.

I am not very imaginative so I just called it RequestInput.

static int RequestInput ()
{

{
Console.WriteLine ("Invalid Input. Please type a number using digits");
}

}


# Final result

class MainClass
{
public static void Main ()
{
var rand = new Random ();

Console.Write ("How many questions would you like to answer? ");

int correctCount = 0;
int questionCount = RequestInput ();

for (int i = 0; i < questionCount; i++)
{
int num01 = rand.Next (11);
int num02 = rand.Next (11);

Console.Write ($"What is {num01} times {num02} ?"); int playerAnswer = RequestInput (); if (num01 * num02 == playerAnswer) { Console.WriteLine ($"{playerAnswer} is correct!");
correctCount++;
}
else
{
Console.WriteLine ($"{playerAnswer} is incorrect!"); } } Console.WriteLine ($"You got {correctCount} of {questionCount} correct.!");

}

static int RequestInput ()
{
{
Console.WriteLine ("Invalid Input. Please type a number using digits");
}

}
}


I did a few other things I like but that are just style choices and mild annoyances (for me). Using var instead of Random, for example, makes it easier for me to read, since Visual Studio has the same coloring for keywords like var and int but different for class names like Random. I also removed Console.ReadKey () from within the loop because it annoyed me when trying out your app. I removed Try Again because you didn't actually let me try again the same question. You could make it possible to try until you get the right answer in a similar pattern to the one dome earlier when verifying input. Finally, I used string interpolation syntax (introduced in C# 6.0) which is much, much, more pleasant to use.

# Bonus, how to set a timer

There is a Timer class, which I have never used. (I am a beginner myself, mind you). There is also System.Diagnostics.StopWatch, which as you can tell is used for diagnostics.

Here is the code with Stopwatch. Mind that you'll need using System.Diagnostics at the top of the file.

class MainClass
{
public static void Main ()
{
var rand = new Random ();

Console.Write ("How long do want to play in seconds?");

var ts = TimeSpan.FromSeconds(RequestInput ()); // type is System.TimeSpan

int correctCount = 0;
int questionCount = 0;

var sw = Stopwatch.StartNew ();

while (sw.Elapsed < ts)
{
// .. same old
questionCount++;
}
sw.Stop ();

Console.WriteLine ($"You got {correctCount} of {questionCount} correct in {sw.Elapsed}.!"); Console.ReadKey (); } }  You will notice I used Stopwatch.StartNew() which is a static method that creates a new instance of Stopwatch and starts it immediately. You could also do var sw = new Stopwatch(); sw.Start(); Edit: i realized there was a bug in my original implementation because I didn't understand how the TimeSpan struct works. I think it should work as expected now for longer times that 1 minute. Make sure to check the MSDN documentation Let me know in the comments if there is anything unclear. Edit : On the other answer, I think creating new classes for a simple console app is overkill, but it is definitely good practice for larger apps and it allows you to extend your game more flexibly. • My mind has been blown by both of these answers. I may need to take some time to parse these in my own head before they go on the app. As for what I'm working on right now, I'm trying to figure out a way to snip out repeating decimals from the questions, as a user couldn't repeat that. – Clol7 Oct 7 '16 at 7:40 • @asibahi you have some very good points here I didn't consider! As for creating new classes for simple apps - I don't think it's overkill. It costs you absolutely nothing, but can improve not only readability of your code, but also make it easier to extend. I had tendency to write this kind of apps not using any classes whatsoever. After couple dozen of them I noticed, that being explicit in gathering and naming common variables it is really great way to improve code quality. Names matter, so you have to not only think about the code, but also what purpose some variable serves :-) – Paweł Hemperek Oct 7 '16 at 7:50 • +1 with minor nit. if there is a performance impact of declaring variables at the start (which I doubt) it will be to slow things down (since I am setting up variables that some code paths might not use). Always declare (and iniitalize if possible) at point of first use. – pm100 Oct 7 '16 at 15:59 • The timer code doesn't seem to work in that it doesn't stop your play if you exceed the timer. it just tells you how long it took you to finish. – Clol7 Oct 7 '16 at 21:09 • @Clol7 yeah that's right. It only stops asking questions when the time limit is reached. To interrupt play you'd need parallelism and multi-threading, which is what the Timer class does. I actually don't know how to implement that in a Console app. (Well I never tried but still) – asibahi Oct 8 '16 at 5:19 It's fairly simple program, nevertheless we can improve it with some object orientation You have following lines in your code int num01 = randomgen.Next(11); int num02 = randomgen.Next(11); int playeranswer; int answer; int numofquestions; int numofquestionsleft; int numofcorrect = 0;  this is some form of primitive obsession antipattern. Why not introduce object, let's call him, GameRound to explicitly tell, what this integers are representing. Let's go even further - we could expand you quiz, so user would not have to calculate product of two factors, but product of any number factors. public class GameRound { public GameRound(List<int> factors) { this.Factors = factors; } private List<int> Factors { get; set; } private int CorrectAnswer { get { // this line uses LINQ Aggregate method to calculate result - no magic at all :-) return this.Factors.Aggregate((a, b) => a * b); } private set {} } public bool CheckAnswer(int answer) { return CorrectAnswer == answer; } }  this is some basic setup. Now, every round is described by concrete object. Notice that since when we moved all those ints into class, you can easily extend it, to support, for example, subtraction or division. We have our rounds, so now let's describe whole game! Let's create another object called Game. public class Game { public Game(IEnumerable<GameRound> rounds) { this.Rounds = rounds; this.CorrectAnswers = 0; this.NumberOfRounds = this.Rounds.Count(); } public IEnumerable<GameRound> Rounds { get; private set; } public int CorrectAnswers { get; private set; } public int NumberOfRounds { get; private set; } public bool AnswerQuestion(GameRound round, int providedAnswer) { if(round.CheckAnswer(providedAnswer)) { this.CorrectAnswers++; return true; } return false; } }  Why we would like to have AnswerQuestion in Game instead of just incrementing value of CorrectAnswers in Main? Because one of the most important thing object oriented programming is encapsulation. Encapsulation means, that we want to hide internals and provide some API to the user, so he can change state of our object without knowing internal processes. How this applies to our case? We wouldn't like someone just doing game.CorrectAnswers = 100;, would we? Let's add one more method to our GameRound class, so we can construct our question inside it and in Main we could only display this question to the user. public string Question() { StringBuilder questionBuilder = new StringBuilder("What is "); int numberOfFactors = this.Factors.Count(); for(int i = 1; i <= numberOfFactors; i++) { if(i == numberOfFactors) { questionBuilder.Append(this.Factors.ElementAt(i - 1)); } else { questionBuilder.Append(string.Format("{0} times ", this.Factors.ElementAt(i - 1))); } } questionBuilder.Append("?"); return questionBuilder.ToString(); }  whit this method we hide question constructing process - everything we would have to do in order to display question is invoking this method. This gives you convenient way of extending your quiz, if you would like to add other kinds of questions (mentioned subtraction, addition etc.). So code responsible for handling the game would look like this static void Main(string[] args) { Random randomGenerator = new Random(); int numberOfQuestions = 0; Console.Write("How many questions would you like to answer? "); numberOfQuestions = Convert.ToInt32(Console.ReadLine()); IEnumerable<GameRound> rounds = GenerateRounds(/* arguments */); Game game = new Game(rounds); foreach(GameRound round in game.Rounds) { Console.WriteLine(round.Question()); int providedAnswer = Convert.ToInt32(Console.ReadLine()); game.AnswerQuestion(round, providedAnswer); } Console.WriteLine("You got " + game.CorrectAnswers + " of " + game.NumberOfRounds + " correct!"); }  I intentionally skipped implementation of GenerateRounds function, so you could write it by yourself :-) EDIT Taking into consideration doubts expressed in comment, here is more details about what this code is really about. Generally what I did is refactoring this procedural code into object oriented. This means, that instead of having code, where asking for input is next to calculating some things and displaying output I moved code responsible for doing certain tasks into classes, that are explicitly telling what this classes responsibility is. Take GameRound for example - just by looking at name you know, that this class is probably responsible for handling things related to one round of your quiz. After writing this code I realized, that we could name this class simply Question, which would express what this class represents even better. Maybe in program like this one you can't see the benefits of this approach, but in general case creating classes, that have proper names and responsibilities is really not trivial task. That's why I think every occasion to train dividing your code into classes is worth taking. Let's explain the code. We will start with GameRound class. Method named same as class is a constructor. This methods is invoked every time object of certain type is created. In our case we have public GameRound(List<int> factors) { this.Factors = factors; }  So, whenever one would like to instantiate object of type GameRound one would have to pass as an argument List<int>. We want to be sure, that every time object of this type is created, it has non-null value of property Factors which we are using later to generate our question for the user. private List<int> Factors { get; set; }  This line declares a property named Factors. Every property has getter and setter. Code in getter is used whenever someone is trying to read value of property, so gameRound.Factors would internally call get method. On the other hand, when we are doing this.Factors = factors we are in fact calling set. In our case both of this methods are empty, because we don't need to do any additional code when reading or assigning value to Factors, so we can use auto-implemented properties. In private int CorrectAnswer { get { return this.Factors.Aggregate((a, b) => a * b); } private set {} }  you have example of using a custom getter. Whenever someone is calling gameRound.CorrectAnswer the correct answer is being calculated and returned. This function uses Aggregate function provided by LINQ. What this function does is it performs an operation on each element of the list taking into account the operations that have gone before as was great explained in this answer. We have also CheckAnswer method, which is responsible of checking if answer passed as an argument is equal to our calculated correct answer and returning true or false. public bool CheckAnswer(int answer) { return CorrectAnswer == answer; }  Thanks to that method we can write gameRound.CheckAnswer(42) which tells you exactly what is going on and hides the specifics of how it's done. Again - in this case, there is not much benefit of encapsulating answer-checking "algorithm", but in more complex solution, it really makes a difference. If we would have to check answers multiple times in our code, whenever someone would like to change our checking-answer "algorithm", we would have to change all places, where our answer is being checked and this can lead to serious code maintenance problems. Instead, we have only one place, where we check our answer, so we would have to change only this code - it's far simpler. Our Question method allows us to build string which represents our question. Let's analyse this method line by line. StringBuilder questionBuilder = new StringBuilder("What is ");  In this line we are initializing new instance of StringBuilder. This class provided an easy and efficient way of constructing strings in C#. You should use this method whenever you are concatenating strings in loop and you are uncertain how many iterations this loop will do. StringBuilder constructor allows you to pass a string as an argument, so you can initialize your builder with some value at the beggining. Next we have this for loop for(int i = 1; i <= numberOfFactors; i++) { /* string building */ }  We want to construct sentence like "What is factor times factor times factor ... ?" What we should do to construct this kind of string? We should iterate through list of factors and for all of them append string "factor times ". Let's assume we have following list of factors: 3, 4, 5. 1. iteration: we have string "3 times " 2. iteration: we have string "3 times 4 times " 3. iteration: we have string "3 times 4 times 5 times ". Everything is okay, except we have unnecessary "times" after 5! To fix this issue we should append string "factor times" for every factor, except the last one. This is the meaning of those lines of code if(i == numberOfFactors) { questionBuilder.Append(this.Factors.ElementAt(i - 1)); }  If we are processing the last factor, just append current factor. else { questionBuilder.Append(string.Format("{0} times ", this.Factors.ElementAt(i - 1))); }  Otherwise append "factor times". Moving to Game class. To create object of this class one is obliged to provide IEnumerable<GameRound>. IEnumerable is an interface, which allows you to iterate over a collection. In this case we could swap IEnumerable<GameRound> with List<GameRound> but it is always better not to couple your code with specific implementation. Why we should not couple our code with specific implementation? Consider following case. You are supposed to write an application, which will calculate how much taxes you should pay. As we know, in every country we should calculate taxes in different way. Right now you know, that this tax calculator will be used only inside USA. You are writing class TaxCalculator which could look like this public class TaxCalculator { /* some other helpful methods */ public decimal CalculateTaxes(USATaxPolicy taxPolicy) { return taxPolicy.Calculate(); } }  With this class in your application you could do something like this class Program { static void Main(string[] args) { TaxCalculator calculator = new TaxCalulator(); USATaxPolicy taxPolicy = new USATaxPolicy(); decimal taxes = calulcator.CalculateTaxes(taxPolicy); Console.WriteLine("You should pay " + taxes + "$");
}
}


It is pretty obvious what this code does. Now imagine your boss comes in and says "we are expanding - now we want to be able to calculate taxes for customers in Poland". To fulfil this request you might add new method to TaxCalculator class, which would accept PolishTaxPolicy as an argument.

public class TaxCalculator
{
/* some other helpful methods */

public decimal CalculateTaxes(USATaxPolicy taxPolicy)
{
return taxPolicy.Calculate();
}

public decimal CalculateTaxes(PolishTaxPolicy taxPolicy)
{
return taxPolicy.Calculate();
}
}


It would work, but you are violating DRY principle, because you are duplicating code return taxPolicy.Calculate();. What you should do instead is adding an interface. Interface allows you to establish contract - if some class implements given interface, this class must have methods declared in this interface. In this example, you could add following interface

public interface ITaxPolicy
{
decimal Calculate();
}


In interface declaration you are just telling "we have method named something which returns someOtherThing". Now those USATaxPolicy and PolishTaxPolicy classes should implement this interface and you can change your CalculateTaxes method, to accept argument that implements our ITaxPolicy interface.

public TaxCalculator
{
/* some other helpful methods */
public decimal CalculateTaxes(ITaxPolicy taxPolicy)
{
return taxPolicy.Calculate();
}
}


From now on it doesn't matter if you would pass USATaxPolicy, PolishTaxPolicy or some OtherTaxPolicy as long as those classes are implementing ITaxPolicy interface.

Let's get back to our code. We have left AnswerQuestion(GameRound round, int providedAnswer) method. In Main we could access properties of our Game class directly.

foreach(GameRound round in game.Rounds)
{
/* display question and collect answer from user */
{
}
}


It is considered bad practice for class to expose internal structure (you can read more about it in chapter 6 of Clean Code by Uncle Bob). Instead we are exposing public method CheckAnswer, so we can check if provided is correct and store this information. If we would allow user to change CorrectAnswers property directly, one could simply assign some value to this property and our object would end up in incorrect state. For example: we only asked 3 questions, but CorrectAnswers has value of 100. It should not happen!

I hope this addresses some of your questions and doubts!

• I think that this is overkill for such a simple task and your code is still not much more flexible due to GameRound implementation. Instead you should pass Func<double,double,double> as a parameter so like this you can pretty much pass any math operation instead of the single multiplication also if needed you might create and overload for Func<double,double> because some functions only require 1 number i.e log with pre-specified base, absolute value of a number, etc... – Denis Oct 7 '16 at 7:52
• Also if you are choosing everything at random you can create a List of functors and you can pick a random index from there and pass it as parameter.. – Denis Oct 7 '16 at 8:00
• Thank you for your comments - your points are absolutely valid! I'm not saying I've done everything I could and this code is as flexible as it possibly can be. I also think, that some of your proposals would be an overkill ;-). As for my answer - I still think introducing two classes even for task as simple as this one is not an overkill. I consider this code much more readable and explicit of what he does. I think it's important to introduce significance of this features to people, who are learning how to code. But hey, maybe it's just me ;-). – Paweł Hemperek Oct 7 '16 at 8:18
• To be honest, I understand very, very, little of this, I may need to go back to looking at tutorials to figure this one out. – Clol7 Oct 7 '16 at 20:42
• @Clol7 I'm more than happy to answer all your questions! I know it may not appear like that, but this is really simple and I'm sure you will understand those concepts and code without problems :-) feel free to post a comment with any doubt you have! – Paweł Hemperek Oct 7 '16 at 20:46

here is my attempt. I've written it in Ruby. Positives: things are broken down, a method is hardly more than a 10 lines. This makes it eas(ier) to read and later change if you need to. You don't want to plan too far for unexpected things in the future, but just have somewhat of an idea what the boss might turn around and say. So long as the code does it's job and is easy to read, then you're halfway there.

Things become tricky when you need to add functionality. That's why the exercise of adding other operations to your code: addition, or adding multiple numbers, becomes interesting. You are forced to refactor your code to those considerations.

The key thing to take out of this:

the key thing to take out of it is that there are many methods working together, rather than everything put in one method. it's broken down in many parts and simplified. if you compare your code above with the code below, the code below breaks it down into many many problems and methods. try to do the same. perhaps i would recommend you rewriting your code again to consider this particular principle. then use object oriented concepts to rewrite the code again.

Breakdown:

• created a question class
• i've created a quiz class which holds all the questions in an array.
• my question class is doing more than one thing: it is bloated and has more than one responsibility. is this acceptable? In this case, it will do.
• very tightly coupled. this is procedural programming more so than object orientated. i'll have this as a kata and redo this problem with a more OOP approach.

Code is on git hub so you can read/edit download it:

https://github.com/BKSpurgeon/maths_quiz/blob/master/quiz.rb

• I gotta admit that this makes no sense to me since I only know C#. Once I get some more out of Pavel, I'll finish my own, taking into account that I can use other classes. However, I did finish one in the style of the original question, I'm just apparently not allowed to edit the original question – Clol7 Oct 8 '16 at 8:08
• @Clol7 ruby is very similar to c# - you should be able to understand 95% of it right away. def is just a way of naming a method. a class is a blueprint that allows you to make more of something. e.g. i can have a car class which allow me to make lots of cars. have a go at reading the above. – BKSpurgeon Oct 8 '16 at 10:24
• is a little easier to read - it's ruby, it isn't easier to read :-] it's just as bad as vb.net - extremely verbose with all those begins and ends – t3chb0t Oct 8 '16 at 10:46
• I believe you've forgotten one if in the obtain_answer method or is it in ruby optional in certain cases? there is an else but no if – t3chb0t Oct 8 '16 at 10:48
• @t3chb0t thx for commenting - that is the equivalent of a try/catch in ruby – BKSpurgeon Oct 8 '16 at 11:02