7
\$\begingroup\$

This is my attempt at writing a guessing game. Where can I improve my code to make it more succinct?

Link to Revision 1 (Guess a number between 1 and 100 (revision 1))

//Program.CS
namespace MidPointGame
{
    class Program
    {
        static void Main(string[] args)
        {
            RunTheGame game = new RunTheGame();
        }
    }
}

//Game.CS
namespace MidPointGame
{
    class RunTheGame
    {
        int _number;

        public RunTheGame()
        {
            const int lbound = 1;
            const int ubound = 100;
            int guessCount;
            bool keepPlaying = true;
            bool computerPlays = false;
            string whoGuessed = "<unknown>";

            Console.WriteLine("Welcome to the numberline game");
            Console.WriteLine("Try and guess the number in the fewest guesses possible");
            Console.WriteLine("The boundary numbers are included in the range.");
            Console.WriteLine("Lets begin");

            while (keepPlaying)
            {
                GenerateNumber();

                if (computerPlays)
                {
                    whoGuessed = "The comupter";
                    guessCount = ComputerGuess(lbound, ubound);
                }

                else
                {
                    Console.WriteLine($"\nGuess a number between {lbound} and {ubound}.");
                    guessCount = HumanGuess();
                    whoGuessed = "You";
                }

                Console.WriteLine($"\n{whoGuessed} guessed it in {guessCount} tries");

                ContinuePlaying(ref keepPlaying,ref computerPlays);
            }

            Console.WriteLine("Thank you for playing\nPress any key to close the program.");
            Console.ReadKey();
        }

        int HumanGuess()
        {
            int _guessCount = 0;
            int difference = 999;

            while (difference !=0)
            {
                difference = ValidateInput() - _number;
                Console.WriteLine(HigherOrLower(difference));
                _guessCount++;
            }
            return _guessCount;
        }

        int ComputerGuess (int lbound, int ubound)
        {
            int guessCount=1;
            int guess = (lbound + ubound) / 2;
            int difference = guess- _number;
            string hiLow = "Got it on the first try!";

            if (difference==0)
            {
                Console.WriteLine($"Computers guess: {guess} {hiLow}");
            }
            else
            {
                while (difference != 0)
                {
                    difference = Math.Sign(guess - _number);
                    if (difference == 1)
                    {
                        ubound = guess;
                        hiLow = "Too high";
                    }
                    else if (difference == -1)
                    {
                        lbound = guess;
                        hiLow = "Too low";
                    }
                    else
                    {
                        hiLow = "Got it.";
                    }
                    guess = (lbound + ubound) / 2;
                    guessCount++;
                }

                Console.WriteLine($"Computers guess: {guess} {hiLow}");
            }

            return guessCount;
        }

        string HigherOrLower(int difference)
        {
            string Outcome="";
            switch (Math.Sign(difference))
            {
                case -1:
                    Outcome = "Too Low";
                    break;
                //case 0:
                //    Outcome = "You guessed it";
                //    break;
                case 1:
                    Outcome = "Too High";
                    break;
            }
            return Outcome;
        }

        int ValidateInput()
        {
            int guess=0;
            Console.WriteLine("What's your guess?");

            while (guess == 0)
            {
                try
                {
                    guess = int.Parse(Console.ReadLine());
                }
                catch (System.FormatException)
                {
                    Console.WriteLine("Guess a valid number");
                }
            }
            return guess;
        }

        void GenerateNumber()
        {
            _number = new Random().Next(1, 100);
        }

        void ContinuePlaying(ref bool keepPlaying,ref bool computerPlays)
        {
            const string invalidEntry = "Please select a valid entry";
            bool validResponse = false;
            computerPlays = false;
            int response = 0;
            Console.WriteLine("Would you like to keep playing?");
            Console.WriteLine("1) Yes");
            Console.WriteLine("2) No");
            Console.WriteLine("3) Yes, but have the computer guess");

            while (!validResponse)
            {
                try
                {
                    response = int.Parse(Console.ReadLine());

                    if (response < 1 || 3 < response)
                        throw new System.FormatException(); //Is there a better way of doing this?

                    validResponse = true;
                    if (response == 1)
                    {
                        keepPlaying = true;
                    }                    
                    else if (response ==2)
                    {
                        keepPlaying = false;
                    }
                    else if (response == 3)
                    {
                        keepPlaying = true;
                        computerPlays = true;   
                    }
                    else
                    {
                        Console.WriteLine(invalidEntry);
                    }
                }
                catch (FormatException)
                {
                    Console.WriteLine(invalidEntry);
                }
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ It kind of bugs me that you can guess any possible number but zero :( \$\endgroup\$ – Marv Mar 20 '17 at 13:33
  • \$\begingroup\$ What does "succint" mean to you? Are you prepared to sacrifice HigherOrLower as a separate method for the sake of "succint"ness? \$\endgroup\$ – brian_o Mar 20 '17 at 17:19
  • \$\begingroup\$ Whatever will improve my code. If it makes it more logical or clearer than I'm all for it. I have a lot to learn so any improvement is welcome. \$\endgroup\$ – IvenBach Mar 20 '17 at 18:38
12
\$\begingroup\$

First, and probably the biggest problem:

RunTheGame game = new RunTheGame();

You are triggering logic in the constructor? Never, ever do class logic in the constructor! The constructor is for creating an object only. In this class, you shouldn't even have a constructor.


Let's look at a better structure for a guessing number game:

GuessingGame class
- Play() void method
- PlayAgain() bool method
- GetInput() int method

So, we have:

public class GuessingGame()
{
    public void Play()
    {
        var counter = 0;
        do
        {
            // take input with GetInput(), check input against number, etc.
            counter++;
        } while (counter < 6);
    }

    public bool PlayAgain()
    {
        Console.Write("Would you like to play again? (y/n): ");
        // read input with Console.ReadLine()
        // if the input is "y", return "true"; otherwise, return "false"
    }

    public int GetInput()
    {
        do  // loop at least once, and until we get valid input
        {
            Console.Write("Enter your guess: ");
            // read input with Console.ReadLine()
            if (int.TryParse(/* your input variable */, out int guess))
            {
                // optionally check range
                return guess;
            }
        } while (true);
    }
}

Now, we play our game like:

var game = new GuessingGame();  // calling the implicit ctor
do
{
    game.Play();
} while (game.PlayAgain());

Now for inspecting your code:

const int lbound = 1;
const int ubound = 100;

What are those doing as local constants? Those should probably be fields. They aren't specific to a single method, the whole class uses them.


Console.WriteLine("Thank you for playing\nPress any key to close the program.");
Console.ReadKey();

Wait a sec--why does your game player know when the program ends? What if you had two games, and the user was just exiting this game? Only Main should know when the program ends.


void GenerateNumber()
{
    _number = new Random().Next(1, 100);
}

Make a field for Random and only assign it once. Each time you use it, it reseeds itself so it will really be random. You are using the local clock to seed it here, so it will product the same value if you call it twice in a single second.


In HigherOrLower(int difference), you should just return the result directly from the switch. Don't assign a variable, then return at the end of the method.


throw new System.FormatException(); //Is there a better way of doing this?

Sure there is. Check the input and notify the user, and let them try again. And do it in a dedicated method. Maybe you can modify the stub GetInput() method I gave you into GetInput(int minValue, int maxValue) and make sure it is within the ranges provided. This is a perfect example of extracting similar logic into a more generic function.


int.Parse will throw an exception if the user types a non-number. Use int.TryParse to see if it is a valid number, and make the user input a valid number.


ValidateInput appears to be reading input too... Use a better name.


There appears to be a lot more stuff I don't have time to review. However, this should provide you plenty to chew on for now.

\$\endgroup\$
5
\$\begingroup\$

I think your try-catch blocks are overkill for what you're doing:

In the ValidInput method, using the int.TryParse method would work a lot better:

int ValidateInput()
{
    int guess=0;
    Console.WriteLine("What's your guess?");

    while (guess == 0)
    {
        int.TryParse(Console.ReadLine(), out guess);
        if(guess == 0 || guess > 100)
        {
            Console.WriteLine("Guess a valid number");
        }
    }
    return guess;
}

In your ContinuePlaying method, a switch block would work better, something like this:

while (!validResponse)
{
    int.TryParse(Console.ReadLine(), out response);
    switch (response)
    {
        case 1:
            keepPlaying = true;
            validResponse = true;
            break;
        case 2:
            keepPlaying = false;
            validResponse = true;
            break;
        case 3:
            keepPlaying = true;
            computerPlays = true;
            validResponse = true;
        default:
            Console.WriteLine(invalidEntry);
            response = 0;
            break;
    }

}
\$\endgroup\$

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.