2
\$\begingroup\$

I have been studying C#, and I am trying to model the Random number guessing game.

1 User inputs the number of try.
2 Computer generates random number.
3 If numbers doesn't match user lose one try and if they match user gets two bonus try.

Is my program reasonable or is there anything I need to improve?

As player goes to next round, I also want to increase random number range by 100 to increase difficulty.For example, first round 1-100 next round 1-200 and so on. How can I do that? Any help would be appreciated.

 public class GuessGame
 {

    public class Game
    {
        public int attemp { set; get; } = 0; 
        public override string ToString()
        {
           return $"The player's attemp <{attemp}>";
        }
    }
    public static void Test()
    {
        Console.WriteLine("Lets play the Low or High Game");
        var game = new Game();

        game.attemp = InputAttempt();
        PlayGame(game);
    }
    public static void PlayGame(Game game)
    {
        Console.WriteLine(game);
        var comNum = GenerateComNum();
            Console.WriteLine($"This is for test: ComNum is {comNum}");
            do
            {
                var userNum = GenerateUserNum();
                CompareNum(userNum, comNum, game);
                Console.WriteLine(game);

            } while (game.attemp >0 );
     }

    static void CompareNum(int userNum, int comNum,Game game)
    {

        if (userNum < comNum)
        {
            Console.WriteLine("User num is too Low try again");
            Console.WriteLine();
            game.attemp--;
        }
        else if(userNum > comNum)
        {
            Console.WriteLine("User num is too High try again");
            Console.WriteLine();
            game.attemp--;
        }
        else if(userNum == comNum)
        {
            Console.WriteLine("You got the answer!!");
            Console.WriteLine();
            game.attemp += 2;
            PlayGame(game);
        }

    }
    static int GenerateUserNum()
    {
        int userNum;
        Console.Write("Please enter the num btw 1-100\t\t");
        while (!Int32.TryParse(Console.ReadLine(), out userNum))
        {
            Console.WriteLine("Invalid num please input whole num");
        }
        return userNum;
    }
    static int GenerateComNum()
    {  
        Random rnd = new Random();
        var comNum = rnd.Next(1,101);
        return comNum;
    }
    static int InputAttempt()
    {
        var attempt = 0;
        Console.Write("How many Attemps would you like?\t");

        while (!Int32.TryParse(Console.ReadLine(), out attempt))
            {
                Console.WriteLine("Invalid num please input whole num");
            }
        return attempt;
    }
}
\$\endgroup\$

3 Answers 3

1
\$\begingroup\$

1) Avoid using var if you want your code to be more readable.

2) Both CompareNum() and PlayGame() methods should be defined within the Game class as they rely on a game (which means they don't need to have a game as argument anymore)

3) In the CompareNum() method

Console.WriteLine("User num is too Low try again");
Console.WriteLine();

can become

Console.WriteLine("User num is too Low try again\n");

4) In the GenerateComNum() method you can shorten

var comNum = rnd.Next(1, 101);
return comNum;

to

return rnd.Next(1, 101);

and you should avoid creating a new Random() every time you have to generate a random number because it could create unexpected results. You should make a static Random rnd = new Random(); and only use rnd.Next(x, y) whenever you need a random number.

5) In the PlayGame() method you shouldn't have a do ... while loop because if the user chooses to have 0 attempts then he should lose instantly and that's not what will happen with a do ... while loop

6) The usual order for accessors is get then set, you did the opposite : public int attemp { set; get; } = 0; it'll work in both orders, it's just to make things more readable.

7) It's realy unlikely to happen but if the user would win during realy long time, your application could have a System.StackOverflowException because you are calling PlayGame() while it is already running when the user wins which will cause the current one running to wait for the new one to finish and those will stack up and take more and more memory, this is recursion, sorry and check internet if my explanations aren't clear.

To increase the range of possible numbers being generated, you can use a variable to determine what the max generated number is and put this variable as second argument instead of 101 when you do rnd.Next(1, 101);

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

This is not optimal

static int GenerateComNum()
{  
    Random rnd = new Random();
    var comNum = rnd.Next(1,101);
    return comNum;
}

Should not generate rnd on every call. You can end up with duplicate values if the seed has not changed.

This should be better

static Random rnd = new Random()
static int GenerateComNum()
{  
    var comNum = rnd.Next(1,101);
    return comNum;
}

This gives the game away.

Console.WriteLine($"This is for test: ComNum is {comNum}");

I don't like the way you use PlayGame(game); to jump out of the loop. The previous call is still in memory.

You don't reset .attemp on a new Game. In a way this is not working code.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ you forgot to remove the new Random in the function body \$\endgroup\$
    – pm100
    Oct 8, 2018 at 21:56
0
\$\begingroup\$

I don't see the point of an outter class. Read: Public nested classes are a code smell


The interface is chaotic. I can't tell how to use the class. Functionality is illogically scattered. The Game class is and does nothing. The flexibility to vary difficulty is not possible with the code as it is. You should re-write the program.

For example, there are two public methods: Test and PlayGame. You'd think I should call PlayGame but the game object is instantiated in Test. The user is prompted for input and then game is instantiated.


Make a simple user interface

Client code should at most have to instantiate a game object and then call Play, or something like that. All the code is inside this game object.


Write a program outline then write the code around it. This big picture outline could be a single method - the "main" method or the "play" method let's say. Each outline statement could possibly represent separate methods.

Display instructions
Ask how many attempts

While there are attempts left
   prompt for guess
   Check the answer
   correct? jump out. Wrong? loop
end while

Show stats? 
Ask to play again?

Write methods based on what they do. The outline statements are a good guide. A single method that both prompts for an answer and calculates the result is doing two different things - different things, different methods. Do not squish methods into larger lumps of code just because you think they are not the "right size." Goldilocks was a lousy programmer.

Good program structure, focused smaller methods, etc. looks like overkill and a waste of time when building small projects like this. Hear me now and believe me later, you're on the road to perdition if you do otherwise.


Adding difficulty is doable once the code is structured.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks you for your advise. I will program it again. I have one question tho. I was doing another project"Rock-Paper-Scissors" and someone advised me to put variables in other class (codereview.stackexchange.com/questions/205002/…) I am confused on this point. Is there article that would help me understand this? \$\endgroup\$ Oct 9, 2018 at 5:37
  • \$\begingroup\$ ...put variables in other class Asked here, it is impossible for me to understand what this is about. You should ask on that thread. BTW, there are many R.P.S. questions on stack exchange, perhaps those would be helpful. \$\endgroup\$
    – radarbob
    Oct 9, 2018 at 5:51

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.