C#/.Net 4.5 - Number Guessing Game

Question

I'm currently enrolled in an introductory C#/.Net 4.5 course and one of the things I want to focus on is the quality of my code.

This is the first assignment our instructor had us do, the ever-popular number guessing game. The requirements our instructor set out for us were:

• On startup, the program asks the user to select a difficulty (easy, medium, hard).
• Once the difficulty is selected, randomly choose the number and prompt the user to guess.
  • The program must validate inputs for the current game (input is numeric and within the possible range of values for that difficulty).
  • The program must keep track of values guessed for the current game. Repeated guesses are considered invalid and do not count as an attempt.
  • If invalid input is received, inform the user and re-prompt.
• After each guess, tell the user if he was correct, under, or over.
• The number of attempts remaining should be displayed each turn of the game
• The program will ask the user if they want to play again.

In addition to these, our instructor challenged me to use as few loops as possible and to use ref or out parameters at least once (we just learned about them in class), because I am already somewhat familiar with C#.

This is what I came up with:

Program.cs:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace NumberGuessingGame
{
    public class Program
    {
        static void Main(string[] args)
        {
            Program p = new Program();
            p.Run();
        }


        public void Run()
        {
            Console.WriteLine("Welcome to the Number Guessing Game!\n");
            Difficulty diff = Difficulty.Easy;
            bool playAgain = false;
            int guessNum;
            do
            {
                int diffChoice = getInt("Your choices of difficulty are:\n1.) Easy (1-10)\n2.) Medium (1-50)\n3.) Hard (1-100)\n\nWhich difficulty would you like to play? ", 1, 3);
                switch (diffChoice)
                {
                    case 1: 
                        diff = Difficulty.Easy; 
                        break;
                    case 2:
                        diff = Difficulty.Medium;
                        break;
                    case 3:
                        diff = Difficulty.Hard;
                        break;
                }
                Console.WriteLine();
                guessNum = new Random().Next((int)diff)+1;
                int remainingGuesses = 5;
                bool winner = guessNumber(guessNum, diff, ref remainingGuesses, new List<int>());
                if (winner)
                {
                    Console.WriteLine("Winner!! It took you "+(5-remainingGuesses)+" tries!\n");
                }
                else
                {
                    Console.WriteLine("Loser :( \nThe number was "+guessNum+"!!\n");
                }
                playAgain = getBool("Would you like to play another game? (Y/N) ", "Y", "N", true);
            } while (playAgain);
            Console.WriteLine("\nThanks for playing!");
        }

        public bool guessNumber(int answer, Difficulty diff, ref int triesLeft, List<int> guesses)
        {
            if(triesLeft == 0){
                return false;
            }
            else
            {
                if (triesLeft == 1)
                {
                    Console.WriteLine("Last chance!!!\n");
                }
                else
                {
                    Console.WriteLine(triesLeft+" chances left!\n");
                }
                int userGuess = getInt("Guess a number from 1 to " + (int)diff+" (inclusive): ", 1, (int)diff);
                if (guesses.Contains(userGuess))
                {
                    Console.WriteLine("You've already guessed "+userGuess+"!! Try again\n");
                    return guessNumber(answer, diff, ref triesLeft, guesses);
                }
                else
                {
                    Console.WriteLine();
                    triesLeft--;
                    guesses.Add(userGuess);
                    if (userGuess == answer)
                    {
                        return true;
                    }
                    else
                    {
                        if (userGuess > answer)
                        {
                            Console.WriteLine("Over!\n");
                        }
                        else
                        {
                            Console.WriteLine("Under!\n");
                        }
                        return guessNumber(answer, diff, ref triesLeft, guesses);
                    }
                }
            }
        }

        public int getInt(string message, int min, int max)
        {
            bool success = false;
            int result;
            Console.Write(message);
            string input = Console.ReadLine();
            success = Int32.TryParse(input, out result);
            success = (result >= min && result <= max);
            return success ? result : getInt("INVALID INPUT: "+input+"\n"+message, min, max);
        }

        public bool getBool(string message, string trueResponse, string falseResponse, bool ignoreCase = true)
        {
            Console.Write(message);
            string response = Console.ReadLine();
            if (ignoreCase)
            {
                if (response.ToUpper().Equals(trueResponse.ToUpper()))
                {
                    return true;
                }
                if (response.ToUpper().Equals(falseResponse.ToUpper()))
                {
                    return false;
                }
            }
            else
            {
                if (response.Equals(trueResponse))
                {
                    return true;
                }
                if (response.Equals(falseResponse))
                {
                    return false;
                }
            }
            Console.WriteLine("INVALID INPUT: "+response+"; Expected "+trueResponse+" or "+falseResponse+"\n");
            return getBool(message, trueResponse, falseResponse, ignoreCase);
        }
    }
}

Difficulties.cs:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace NumberGuessingGame
{
    public enum Difficulty : int
    {
        Easy = 10,
        Medium = 50,
        Hard = 100
    }
}

Any input would be highly appreciated! I want to focus on becoming a better programmer in any way I can. Don't be afraid to nitpick!

Answer 1

public enum Difficulty : int

There's no need to specify the type. It's an int by default.

Every enumeration type has an underlying type, which can be any integral type except char. The default underlying type of enumeration elements is int. To declare an enum of another integral type, such as byte, use a colon after the identifier followed by the type, as shown in the following example.

MSDN - enum

A couple of things about your Main.

    static void Main(string[] args)
    {
        Program p = new Program();
        p.Run();
    }

1. There's no reason to create a new instance of Program from inside of Program. Just make Run a static method and call it. However...
2. You've probably heard that you shouldn't have a bunch of logic in your Main routine, so you created this Run method to call from Main. Unfortunately, you've missed the point. Instead of stuffing all of the logic into Main, you've stuffed it into Run.

Get rid of Run entirely and move that logic into Main. Then, start extracting methods. By the time you're done, Main should read something like this pseudo code.

WelcomeUser();

var play = true;
var isFirstGame = true;

while (play)
{
    if (!isFirstGame && !play)
    {
        var difficulty = PrompUserForDifficulty();
        PlayGame(difficulty);
    }

    play = AskUserToPlayAgain();
}

SayGoodbye();

And that's it. 99% of the logic should reside in PlayGame() or some equivalent. To clarify, Main shouldn't be entirely void of logic. The logic should simply be minimal.

---

You're returning early here, so there's an opportunity to reduce the levels of indentation.

        if(triesLeft == 0){
            return false;
        }
        else
        {
            (if triesLeft == 1)

Is equivalent to

if (triesLeft == 0)
{
    return false;
}

if (triesLeft == 1)

---

My last point (although more could be said) is that ref and out params are a bit of a code smell. Rarely are they really needed out in the real world. I won't pick on it too much, because your teacher challenged you to use it, but in reality, if you think you need a ref arg, what you really need is a function, a method that returns a value. If you think you need a bunch out args, what you really need is a struct that formally groups those args into a formally defined and coherently grouped data structure (and obviously a method to return it).

Answer 2

I'm not sure whether an enum is he best way to keep the mapping between difficulties and maximum values. If your ideas of these change, or you add more difficulties, then the type itself changes. That shouldn't happen.

I propose that the enum should just list the difficulty level names but not their maximum values. Put the maximum values in a Dictionary<Difficulty,int>.

Answer 3

When you write code, it is very important to make the said code as clear as possible. Methods named getInt and getBool aren't self explanatory. Think that when you read code, you should be able to understand what a method does without reading the code of the method. To show you a good example of what I mean, I wrote a really long answer about the getInt method being about selecting difficulties but I found out at the end of my review that it wasn't the whole point of this method.

I'll attack the getInt method for now. It's sole responsability is to prompt the user for a number. Meaning a better name could be PromptUserForNumber, or something like that! When I name my variables/methods/classes etc, I think about what my method does, resume it in 2-4 words max and that is my method name (works most of the time).

In this method you currently have 3 parameters, message is quite good, though I might have named it promptedMessage or something like that, just to make it even more clear, but that might be nitpicking.

The interesting point is about your two other parameters, min and max. In your cases (prompting for a difficulty and for a guess), you only validate if the number is within a range. But you might need other kind of validation in the future, for example any number greater than 1 (which is a little excessive I might say). In this case you'd have to tweak your code to make it work. My solution for this is to use Func<int,bool>. I'm not sure if you have seen those before since they are a little bit more advanced. This would give you the opportunity to let the caller of your method define the validations to be made on the input. For example :

public static void Main()
{
    PromptUserForNumber("Give me a number higher than one", (i) => i > 1));
    PromptUserForNumber("Give me a number between -10 and 10 but 0 isn't allowed", (i) => 10 > i && i > -10 && i != 0));
}

public static int PromptUserForNumber(string message, Func<int,bool> validation)
{
    //I'll get back to it in a second
}

Now that is great, your prompt method doesn't need to know how to validate, it just uses the Func. Now, this is how we use it :

public int PromptUserForNumber(string promptedMessage, Func<int,bool> validation)
{
    bool success = false;
    int result;
    Console.Write(promptedMessage);
    string input = Console.ReadLine();
    success = Int32.TryParse(input, out result);
    //Magic
    success = validation(result);
    return success ? result : getInt("INVALID INPUT: "+input+"\n"+message, min, max);
}

Now, in my opinion, we have a solid method signature.

We can now look into the code of the said method. You define success at the top of your method, but do not use it until you assign it again, meaning you could join these two statements and remove bool success = false;

bool success = Int32.TryParse(input, out result);

Talking about success (eh eh), what does it mean. What succeeded? Your boolean answers the question "Is the user input valid?" so I think it should be named isInputValid. Also, you have a problem. You assign success (now isInputValid) twice in a row, but never use the first assignation meaning that if your input wasn't a valid integer, your application will process as if the user had input the number 0, which isn't good. What you wanted to do was probably more :

bool isInputValid = Int32.TryParse(input, out result);
isInputValid = isInputValid && validation(result);

Then, this looks weird, what if we could do this in 1 statement. Well, we can. (Also, using Int32 is the same as using int, I prefer using int)

bool isInputValid = int.TryParse(input, out result) && validation(result);

Now, I need to talk about the recursive call. Recursion is, in my opinion, a beautiful monster. It is a pain to read, to write but so awesome once you understand it. In your case, recursion seems a little over the top. Recursive calls aren't that great, seriously. You could make this a little easier to read (and write) by using a while loop.

public int PromptUserForNumber(string promptedMessage, Func<int,bool> validation)
{
    int result;
    Console.Write(message);
    string input = Console.ReadLine();
    bool isInputValid = int.TryParse(input, out result) && validation(result);

    while(!isInputValid)
    {
        Console.Write("Your invalid message");
        input = Console.ReadLine();
        isInputValid = Int32.TryParse(input, out result) && validtion(result);
    }

    return result;
}

With this approach, there is code repetition, but I'd say it is neater. We can fix the repetition by using do...while() with a little tweak (god I hate this word).

public int PromptUserForNumber(string promptedMessage, Func<int,bool> validation)
{
    int result;
    Console.Write(message);
    string input = Console.ReadLine();
    //Part of the tweak
    bool isInputValid = true; 

    do
    {
        Console.Write(isInputValid ? promptedMessage, "Invalid message");
        input = Console.ReadLine();
        isInputValid = Int32.TryParse(input, out result) && validation(result);
    } while(!isInputValid);

    return result;
}

In your prompted message, you always use Console.Write and specify the line breaking character \n. Instead, you can use Console.WriteLine which sums up to :

Console.Write("Hello\n") <==> Console.WriteLine("Hello")

Also, ref and out parameters are dangerous and misunderstood, you shouldn't use them unless you are sure that you need to use them. Here isn't the case in my opinion. But I can't argue with your teacher ;)

Answer 4

Maybe you could respond back to the teacher that you used a class which is passed around by reference :D for example

[TestClass]
public class GuessedNumberTest
{
    [TestMethod]
    public void TestNumberPassedByReference()
    {
        int number = 0;
        GuessNumber(ref number);

        Assert.AreEqual(4, number);
    }

    private static void GuessNumber(ref int number)
    {
        number = 4;
    }

    [TestMethod]
    public void TestClassPassedByReference()
    {
        var guessedNumber = new GuessedNumber {Number = 1};
        GuessNumber(guessedNumber);

        Assert.AreEqual(42, guessedNumber.Number);
    }

    private static void GuessNumber(GuessedNumber guessedNumber)
    {
        guessedNumber.Number = 42;
    }
}

Next I am going to point out 2 things here. One is that even though you are in school, and you are somewhat new to programming I am a firm believer in using tests (and not user input) to test your program. I don't want to start a debate about how to write tests and when. That isn't the point. The point is that writing automated tests have many benefits. Granted at first it is a little harder and slower to write a program. But once you get the hang of it it is amazing. Some of the benefits are that your code can test itself, and it can test itself very fast. For this game I would imagine that it could be written using about 20 tests total. Those 20 tests would (and should) complete in less than a second. (probably in the 100ms range) You may be fast, but not that fast. What's also nice is that it gives you confidence to change your code willy nilly to try new things and know that you didn't break your code. So I am going to take 2 of your methods. Pull them out, and make them testable.

(FYI, I had started a book about how I got from A to B with your code. But instead I decided to just show you what a few of my tests look like, and what your getInt method turned into)... First the tests

[TestClass]
public class InputConverterTests
{
    private MockInputProvider _mockInputProvider;
    private InputConverter _inputConverter;

    [TestInitialize]
    public void Initialize()
    {
        Console.WriteLine("Init");
        _mockInputProvider = new MockInputProvider();
        _inputConverter = new InputConverter(_mockInputProvider);
    }

    [TestMethod]
    public void WithValidNumericInput_getIntReturnsSameInteger()
    {
        _mockInputProvider.SetupReturnValues("2");

        var actual = _inputConverter.getInt("Test", 1, 3);

        Assert.AreEqual(2, actual);
    }

    [TestMethod]
    public void WhenValueIsBelowMin_getIntTriesAgainToGetValidInput()
    {
        _mockInputProvider.SetupReturnValues("0", "-1", "2");

        var actual = _inputConverter.getInt("Test", 1, 3);

        Assert.AreEqual(2, actual);
    }

    [TestMethod]
    public void WhenValueIsAboveMax_getIntTriesAgainToGetValidInput()
    {
        _mockInputProvider.SetupReturnValues("4", "5", "2");

        var actual = _inputConverter.getInt("Test", 1, 3);

        Assert.AreEqual(2, actual);
    }

    private class MockInputProvider : IInputProvider
    {
        private readonly Queue<string> _input = new Queue<string>();

        public void SetupReturnValues(params string[] returnValues)
        {
            returnValues.ToList().ForEach(_input.Enqueue);
        }
        /// <summary>
        /// From IInputProvider
        /// </summary>
        /// <returns>Returns first string that was enqueued.</returns>
        public string GetInput()
        {
            return _input.Dequeue();
        }
    }
}

Those tests all pass with your current code. However, because I have tests I can now easily change your code (fyi it took about 5ms to run all 3 of those tests)

And here is a portion of the resulting new code. Granted it's not that much different... but it is testable.

public class InputConverter
{
    private readonly IInputProvider _inputProvider;

    public InputConverter(IInputProvider inputProvider)
    {
        _inputProvider = inputProvider;
    }

    public int getInt(string message, int min, int max)
    {
        int result;
        Console.Write(message);
        var input = _inputProvider.GetInput();
        if (Int32.TryParse(input, out result)
            && result >= min
            && result <= max)
        {
            return result;
        }
        return getInt("INVALID INPUT: " + input + "\n" + message, min, max);
    }
...
}

Answer 5

A big issue I have with your code is that you have Console.Read and Console.Write scattered through your functions.

It reads like you are thinking through one long process and taking actions as soon as your possibly can.

As an exercise I suggest you refactor so that only your Run (or main) method ever reads or writes from the console. This restriction will force you to write in a more OO/functional way and I think you will be pleased with the results