3
\$\begingroup\$

I created a simple guessing game for the sake of practice, it's been a while since I've had any code reviewed. I do have a tendency to over-engineer things sometimes and I'm not typically aware of it until well after the programs creation, so please let me know if I've done such a thing. Other than that, I'm open to any and all suggestions.

Note, I did use mathematical notation of [1, 101) to denote that 1 is in the valid range of numbers and 101 in not included in the valid range of numbers.

Program.cs

public class Program
{
    private const string IntroductionStatement = "Guess the randomly chosen number.\r\nYou will be told if the number is LARGER or SMALLER than your guess after each attempt.";
    private const string VictoryStatement = "You have won the game!";
    private const string DefeatStatement = "Sorry, you have lost the game!";
    private const string IncorrectGuessStatement = "Sorry that was incorrect.";
    private const string PromptStatement = "How many attempts would you like? ";
    private const string GuessStatement = "Your Guess: ";

    private const string NumberRangeStatement = "The possible range of numbers is [{0}, {1}).\r\n";
    private const string CurrentAttemptStatement = "\r\nAttempt #{0} of {1}.";
    private const string LargerSmallerStatment = "The number is {0} than your guess.";
    private const string AnswerStatement = "The number was {0}.";

    static void Main(string[] args)
    {
        Console.WriteLine(IntroductionStatement);
        Console.WriteLine(String.Format(NumberRangeStatement, GuessThatNumberGame.MinimumNumber, GuessThatNumberGame.MaximumNumber));

        int maxAttempts = GetMaxAttemptsFromPlayer();
        var game = new GuessThatNumberGame(maxAttempts);

        do
        {

            Console.WriteLine(String.Format(CurrentAttemptStatement, game.Attempt, game.MaxAttempts));
            var playerGuess = GetNumberGuessFromPlayer();
            game.SubmitGuess(playerGuess);

            if (game.HasBeenWon != true)
            {
                Console.WriteLine(IncorrectGuessStatement + " " + String.Format(LargerSmallerStatment, game.Number > playerGuess ? "LARGER" : "SMALLER"));
            }

        } while (game.HasBeenWon == null); // True means we've won, False means we've lost. Null means it's still in progress.

        Console.WriteLine($"\r\n{(game.HasBeenWon == true ? VictoryStatement : DefeatStatement)}");
        Console.WriteLine(String.Format(AnswerStatement, game.Number));

        Console.ReadKey();
    }

    private static int GetMaxAttemptsFromPlayer()
    {
        return GetNumberFromPlayer(PromptStatement);
    }

    private static int GetNumberGuessFromPlayer()
    {
        return GetNumberFromPlayer(GuessStatement);
    }

    private static int GetNumberFromPlayer(string prompt)
    {
        var number = 0;
        do
        {
            Console.Write(prompt);
        } while (Int32.TryParse(Console.ReadLine(), out number) == false);
        return number;
    }
}

GuessThatNumberGame.cs

public class GuessThatNumberGame
{
    private static Random _Randomizer = new Random();

    public const int MinimumNumber = 1;
    public const int MaximumNumber = 101;

    public int Attempt { get; private set; }
    public int MaxAttempts { get; private set; }
    public int Number { get; private set; }
    public bool? HasBeenWon { get; private set; }

    public GuessThatNumberGame(int maxAttempts)
    {
        if (maxAttempts <= 0)
        {
            throw new ArgumentOutOfRangeException("maxAttempts");
        }

        Number = _Randomizer.Next(MinimumNumber, MaximumNumber);
        MaxAttempts = maxAttempts;
        Attempt = 1;
    }

    /// <summary>
    /// Enter a guess from the player.
    /// </summary>
    public void SubmitGuess(int guessedNumber)
    {
        // No more attempts if they've already lost.
        if (HasBeenWon == false)
        {
            return;
        }

        // Add this to the number of attempts.
        Attempt += 1;

        // If they guessed correctly set to victory.
        if (guessedNumber == Number)
        {
            HasBeenWon = true;
        }

        // This was their last attempt if we're over the max now.
        if (Attempt > MaxAttempts)
        {
            HasBeenWon = false;
        }
    }
}

Sample: Victory

Guess the randomly chosen number. You will be told if the number is LARGER or SMALLER than your guess after each attempt. The possible range of numbers is [1, 101).

How many attempts would you like? 10

Attempt #1 of 10. Your Guess: 50 Sorry that was incorrect. The number is SMALLER than your guess.

Attempt #2 of 10. Your Guess: 25 Sorry that was incorrect. The number is LARGER than your guess.

Attempt #3 of 10. Your Guess: 35 Sorry that was incorrect. The number is LARGER than your guess.

Attempt #4 of 10. Your Guess: 40 Sorry that was incorrect. The number is LARGER than your guess.

Attempt #5 of 10. Your Guess: 45 Sorry that was incorrect. The number is SMALLER than your guess.

Attempt #6 of 10. Your Guess: 43 Sorry that was incorrect. The number is LARGER than your guess.

Attempt #7 of 10. Your Guess: 44

You have won the game! The number was 44.

Sample: Defeat

Guess the randomly chosen number. You will be told if the number is LARGER or SMALLER than your guess after each attempt. The possible range of numbers is [1, 101).

How many attempts would you like? 5

Attempt #1 of 5. Your Guess: 50 Sorry that was incorrect. The number is SMALLER than your guess.

Attempt #2 of 5. Your Guess: 25 Sorry that was incorrect. The number is SMALLER than your guess.

Attempt #3 of 5. Your Guess: 12 Sorry that was incorrect. The number is SMALLER than your guess.

Attempt #4 of 5. Your Guess: 6 Sorry that was incorrect. The number is SMALLER than your guess.

Attempt #5 of 5. Your Guess: 3 Sorry that was incorrect. The number is LARGER than your guess.

Sorry, you have lost the game! The number was 4.

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

Magic Values

I took a look on your source code. It is quite good, and I like the way you define constants for values, but there are still some magic values, such as "maxAttempts".

Exceptions

In side the method GuessThatNumberGame(int maxAttempts), you throw an ArgumentOutOfRangeException. I think you can add more detail in the error message then it is easier to understand the problem exactly. Maybe:

throw new ArgumentOutOfRangeException("maxAttempts must be more than 0");

And I don't see where do you handle the exception. As an user, I expected that I will receive some error message when I enter an invalid value instead of the app crash.

UX:

Let see this function:

private static int GetNumberFromPlayer(string prompt)
        {
            var number = 0;
            do
            {
                Console.Write(prompt);
            } while (Int32.TryParse(Console.ReadLine(), out number) == false);
            return number;
        }

It look good, but when user input invalid numbers( ex: a string) then nothing show to the console, user can not understand what is their next step. I think you can check the input there, and ask user enter a number:

private static int GetNumberFromPlayer(string prompt)
{
    var number = 0;
    Console.Write(prompt);

    do
    {           
        If(Int32.TryParse(Console.ReadLine(), out number))
        {
            break;  
        }
        else
        {
            Console.Write("please enter a number");   
        }
    } while (true);
    return number;
}

Regard!

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Could you elaborate on the first point please? Not sure how maxAttempts is a magic value since it's obtained from the user. Unless you're referring to my exception if-statement preventing it from being less than 1? \$\endgroup\$ – Shelby115 Oct 9 '18 at 15:13
  • \$\begingroup\$ Hi, I mean the string "maxAttempts", not the int maxAttempts. P/S: when you are using condition, don't need to compare with value.( while (Int32.TryParse(Console.ReadLine(), out number), instead of while (Int32.TryParse(Console.ReadLine(), out number) == false) ). \$\endgroup\$ – Nhan Phan Oct 9 '18 at 15:22
  • 1
    \$\begingroup\$ Did you mean while(!Int32.TryParse(Console.ReadLine(), out number)? because while(Int32.TryParse(Console.ReadLine(), out number) is the opposite condition I wish to check. If that is what you meant, I prefer == false to ! because it's easier to notice while scrolling through the code. \$\endgroup\$ – Shelby115 Oct 9 '18 at 15:25
  • 1
    \$\begingroup\$ I had just "maxAttempts" because the first parameter for the first overload of ArgumentOutOfRangeException(..) was named paramName but I suppose it does have another overload where the parameter's name is message so thanks for that. Thanks for the UX section too, in this case I think I would keep it the way it is (they should know they're playing a number guessing game) but I do fail to consider other user possibilities quite often so it's great advice! \$\endgroup\$ – Shelby115 Oct 9 '18 at 15:27
  • 2
    \$\begingroup\$ I'm not sure why this hasn't been mentioned, but there's a nameof operator that allows you to replace "maxAttempts" with nameof(maxAttempts), making it refactor-proof. \$\endgroup\$ – Pieter Witvoet Oct 9 '18 at 15:31

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.