8
\$\begingroup\$

I am a beginner in C# coding, and I was trying to compare two int variables:

void CompareNumber() {
    int oneNumber;
    int secondNumber;

    if (oneNumber > secondNumber)
        {
        DoSomething();
        }

    else if (oneNumber < secondnumber)
        {
        DoSomethingElse();
        }

    else if (oneNumber == secondnumber)
         {
         DoSomethingDifferent();
         }
}

While this does work, it looks kinda messy, especially because I compare variables in this manner many times. Is there a more concise way of doing this, making it look neater? (Except for omitting the curly brackets)

Here is my actual code for a simple game that thinks of a number, and you need to guess what it said:

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

namespace GuessName
    {
    class Program
        {
        static void Main(string[] args)
            {
            int previousNumber = 0;
            AskNumber(previousNumber);
            }

        static void AskNumber(int previousNumber)
            {
            Console.Clear();

            int numberTyped;
            int randomNumber = new Random().Next(1, 11);

            while (previousNumber == randomNumber)
                randomNumber = new Random().Next(1, 11);

            Console.WriteLine("I am thinking of a number between 1 and 10. What do you think it is?");


            if (int.TryParse(Console.ReadLine(), out numberTyped) == true)
                CheckNumber(numberTyped, randomNumber);
            else
                InvalidNumber();
            }

        static void CheckNumber(int numberTyped, int randomNumber)
            {
            if (numberTyped < 0 || numberTyped > 10)
                {
                InvalidNumber();
                }
            else if (numberTyped > randomNumber)
                {
                Console.Clear();
                Console.WriteLine("Sorry, but your number is bigger than what I thought");
                Console.WriteLine("Try again, just type what you think:");
                }
            else if (numberTyped < randomNumber)
                {
                Console.Clear();
                Console.WriteLine("Sorry, but your number is smaller than what I thought");
                Console.WriteLine("Try again, just type what you think:");
                }
            else
                {
                Console.Clear();
                WonGame(randomNumber);
                }
            if (int.TryParse(Console.ReadLine(), out numberTyped) == true)
                CheckNumber(numberTyped, randomNumber);
            else
                InvalidNumber();
            }

        static void WonGame(int randomNumber)
            {
            Console.WriteLine("Great job, You did it!");
            Console.WriteLine("Would you like to try again? (y) or would you like to quit? (n)");

            string wantToPlay = Console.ReadLine();
            if (wantToPlay == "y")
                AskNumber(randomNumber);
            else
                Environment.Exit(0);
            }

        static void InvalidNumber()
            {
            Console.Clear();

            Console.WriteLine("Your number is invalid. Would you like to try again? (y)");
            Console.WriteLine("Or would you like to quit? (n)");

            string wantToPlay = Console.ReadLine();
            if (wantToPlay == "y")
                AskNumber(0);
            else
                Environment.Exit(0);
            }
        }
    }
\$\endgroup\$
0

3 Answers 3

12
\$\begingroup\$

Indentation?

At first I thought it was a mere copy/paste error, but the pattern is throughout the code you submitted, so I have to mention it: it's not the braces you indent, it's what the braces encompass!

    static void Main(string[] args)
        {
        int previousNumber = 0;
        AskNumber(previousNumber);
        }

Should be:

    static void Main(string[] args)
    {
        int previousNumber = 0;
        AskNumber(previousNumber);
    }

Curly braces define a scope. In C#, convention is to place the scope-opening brace on the next line, so this:

void CompareNumber() {

Should be:

void CompareNumber() 
{

Speaking of braces:

Is there a more concise way of doing this, making it look neater? (Except for omitting the curly brackets)

Omitting the curly braces does not make code look neater. Proof being this very confusing snippet:

        if (wantToPlay == "y")
            AskNumber(0);
        else
            Environment.Exit(0);
        }

I had to scan the entire method twice to figure out that the last } was in fact closing the scope of the method.

By opposition:

        if (wantToPlay == "y")
        {
            AskNumber(0);
        }
        else
        {
            Environment.Exit(0);
        }
    }

Is brutally in-your-face crystal-clear. Notice the position of the last brace, which closes the method scope. The readability problem stems from the extraneous indentation of scope-delimiting braces - this doesn't suffer the exact same issue:

        if (wantToPlay == "y")
            AskNumber(0);
        else
            Environment.Exit(0);
    }

You should always delimit scopes with braces. if/else statements denote a scope - that scope wants its braces.

Same with loop constructs:

        while (previousNumber == randomNumber)
            randomNumber = new Random().Next(1, 11);

Should be:

        while (previousNumber == randomNumber)
        {
            randomNumber = new Random().Next(1, 11);
        }

The Good

You use descriptive naming. This is often understated - descriptive names are your best friend. They reduce bug-proneness of your code all by themselves, and make code easier and more enjoyable to read. Since programming is 80% reading and 20% writing, enjoyable reading means enjoyable programming.

Good job!


The Bad

The syntax for an if condition goes if ([bool-expression]), where [bool-expression] is any expression that evaluates to true or false. Can you spot the redundancy here?

if (int.TryParse(Console.ReadLine(), out numberTyped) == true)

That's right. TryParse returns a bool that's a perfectly valid [bool-expression] - in fact, anytime you're comparing a Boolean value (or expression) to a Boolean constant for the sake of getting a Boolean expression, you are needlessly repeating yourself... redundantly.

Just do this instead:

if (int.TryParse(Console.ReadLine(), out numberTyped))

Method names should start with a verb. Always. void InvalidNumber() is a bad name, because it doesn't say anything about what it actually does - it doesn't return a number, and doesn't intake one either. Without looking at how it's implemented, it's pretty confusing.


Console applications exit when the Main method executes to its closing brace. I have never used Environment.Exit(0) in any application I've written - in fact, this call is like a "red button", a forceful, ugly way to kill your application (and throw the body in a container in the back alley) rather than letting it gracefully exit through the front door.

You're going to have to restructure things up in order to make that happen though.


CheckNumber shouldn't have the 0 and 10 hard-coded like this; there should be constants defined for the lower and upper bounds, and the AskNumber method should be using them as well - that way if you ever need to change the 10 to a 100, there's only 1 place you'll need to change it.

AskNumber shouldn't be creating a new Random() every time. There should be one single instance of Random that the program uses whenever it needs a random number.

There's a lot more to say about this code, I'll let other reviewers chip in ;)

\$\endgroup\$
7
  • \$\begingroup\$ When you talk about CheckNumber and AskNumber, I am kinda confused there. What do you mean? \$\endgroup\$ Jan 9, 2015 at 4:33
  • \$\begingroup\$ randomNumber = new Random().Next(1, 11); appears twice in AskNumber, and in CheckNumber you have if (numberTyped < 0 || numberTyped > 10) - that 0-10 and the 1-11's are the same concept: it's the range of valid values. Make a private readonly int MinValue = 1 and a private readonly int MaxValue = 10. Then you have randomNumber = new Random().Next(MinValue, MaxValue + 1); and if (numberTyped < MinValue || numberTyped > MaxValue), and only one character to add to the code if you want to make it 1-100 instead of 1-10. \$\endgroup\$ Jan 9, 2015 at 4:42
  • \$\begingroup\$ The reason newRandom() appears twice is a failsafe to stop the game from accidently picking the same number again. What it does is randomly pick a number, and keeps changing that until it is different from the previous number (hence the while(previousNumber == randomNumber)). The second one, however, makes perfect sense. I will change that right now :) Thanks SO much for your help, my code looks much better now. I still need to figure out how to make it go back to Main() to end properly, instead of using Environment.Exit(0) \$\endgroup\$ Jan 9, 2015 at 7:41
  • \$\begingroup\$ @Mat'sMug, why readonly int instead of const int? \$\endgroup\$
    – Brian S
    Jan 9, 2015 at 16:11
  • 1
    \$\begingroup\$ @AhkamNihardeen, Your "failsafe" is going to do the opposite of what you think. If the two new Random()s occur close enough together in time, the seed is going to be the same and they'll produce the same series of numbers. If you have a single instance of Random as Matt suggests, you'll get a sequence of random numbers as you actually want. \$\endgroup\$
    – Brian S
    Jan 9, 2015 at 16:13
5
\$\begingroup\$

I don't see anything wrong with the comparison in the example you showed. To me, at least, it looks like exactly what you need. I think that perhaps your issue with it comes from all of the repetition in your real code.

    else if (numberTyped > randomNumber)
        {
        Console.Clear();
        Console.WriteLine("Sorry, but your number is bigger than what I thought");
        Console.WriteLine("Try again, just type what you think:");
        }
    else if (numberTyped < randomNumber)
        {
        Console.Clear();
        Console.WriteLine("Sorry, but your number is smaller than what I thought");
        Console.WriteLine("Try again, just type what you think:");
        }

These two cases are very similar. You can extract a method here using String.Format.

private void WriteWrongGuessMessage(string comparison)
{
    Console.Clear();
    Console.WriteLine(String.Format("Sorry, but your number is {0} than what I thought.", comparison);
    Console.WriteLine("Try again, just type what you think:");
}

And then call it like this.

        if (numberTyped < 0 || numberTyped > 10)
        {
            InvalidNumber();
        }
        else if (numberTyped > randomNumber)
        {
            WriteWrongGuessMessage("bigger");
        }
        else if (numberTyped < randomNumber)
        {
            WriteWrongGuessMessage("smaller");
        }
        else
        {
            Console.Clear();
            WonGame(randomNumber);
        }

In general, I think this is pretty good for a novice. Good names, more or less neat (minus the indentation that another answer has already mentioned).

You copy pasted some code though. Don't do that. A wise Mug once said, "Anytime your fingers reach for Ctrl + C your brain should stop them and write a method instead." I might be paraphrasing a bit, but it's true. Don't repeat yourself. Both InvalidNumber and WonGame have this code.

    string wantToPlay = Console.ReadLine();
    if (wantToPlay == "y")
        AskNumber(randomNumber);
    else
        Environment.Exit(0);
    }

Go ahead and extract it into a private method of it's own.

I have a minor concern about that snippet though. You ask the user to "n" to quit, but in reality, anything but "y" will exit the program. I'd consider that to be a bug.

    Console.WriteLine("Your number is invalid. Would you like to try again? (y)");
    Console.WriteLine("Or would you like to quit? (n)");
\$\endgroup\$
5
  • \$\begingroup\$ Thank you SO Much. This stackexchange page is SO much better and has a nicer community when compared to others. Thanks again. I am just wondering, is it ok if I make a bunch of methods to do this? Or is that excessive methods? \$\endgroup\$ Jan 9, 2015 at 2:42
  • \$\begingroup\$ You're welcome. You'll have to pardon me though. A "bunch of methods" to do what @AhkamNihardeen? \$\endgroup\$
    – RubberDuck
    Jan 9, 2015 at 2:47
  • \$\begingroup\$ What I mean is if instead of doing a long code in Main(), should I make a bunch of methods that specificallly do different things? \$\endgroup\$ Jan 9, 2015 at 4:20
  • \$\begingroup\$ Yes! Absolutely. You've already done this to a point. Your Main method is actually only 2 lines. =;)- \$\endgroup\$
    – RubberDuck
    Jan 9, 2015 at 10:36
  • \$\begingroup\$ IKR ;) So Long Main. \$\endgroup\$ Jan 9, 2015 at 11:33
5
\$\begingroup\$

For getting numeric values from the user, I usually create a helper function, something like:

static int GetBoundedIntFromUser(string prompt, int minValue, int maxValue)
{
    if (minValue >= maxValue) throw new ArgumentException("minValue must be less than maxValue");

    int input;

    while (true)
    {
        if (prompt != null) Console.Write(prompt);
        if (int.TryParse(Console.ReadLine(), out input)
            && input >= minValue && input <= maxValue)
            break;
        Console.WriteLine("Sorry, {0} is not a valid number between {1} and {2}.",
            input, minValue, maxValue);
    }

    return input;
}

Also, it looks like you're creating a new Random() for each new game, which isn't necessary. Instead, consider declaring a Random with 'global' scope, and initialize it only once per session:

class RandomNumberGame
{
    static Random rnd;

    static void Main(string[] args)
    {    
        rnd = new Random();
        PlayRandomNumberGame();
    }

    // Other methods here ...
}

You could also simplify your game code (and remove spaghetti-like code where many of your methods are calling each other) by using some do loops in your main game method. For example, you can use an outer loop that runs each time the player wants to play another game, and an inner one that runs each time the get an incorrect answer:

static void PlayRandomNumberGame()
{
    do
    {
        Console.Clear();
        var randomNumber = rnd.Next(1, 11);
        Console.WriteLine("Welcome to the Random Number game!");
        Console.WriteLine("I'm thinking of a number between 1 and 10. What do you think it is?");
        int userInput;

        do
        {
            userInput = GetBoundedIntFromUser("(Enter a number from 1 to 10): ", 1, 10);
        } while (!CheckForWiner(userInput, randomNumber));

    } while (PlayAnother());
}

Of course, this would requires that the CheckForWinner and PlayAnother methods return a Boolean:

static bool CheckForWinner(int userEntry, int actualNumber)
{
    if (userEntry == actualNumber)
    {
        Console.WriteLine("Great job, You guessed it!");
        return true;
    }

    Console.WriteLine((userEntry > actualNumber)
        ? "Sorry, {0} is bigger than the one I'm thinking of."
        : "Sorry, {0} is smaller than the one I'm thinking of.", userEntry);

    return false;
}

static bool PlayAnother()
{
    Console.Write("Would you like to play again (y/n)? ");
    var input = Console.ReadKey();

    return (input.Key == ConsoleKey.Y);
}
\$\endgroup\$
5
  • \$\begingroup\$ when loading a new game, it does the Random() code again. I also put a failsafe in it accidnetly gets the wrong code again, in the while tag. \$\endgroup\$ Jan 9, 2015 at 4:20
  • \$\begingroup\$ @Ahkam the point is that you don't need a new Random() instance every time you need random.Next() method call result. And you don't need two instances to implement that "fail-safe" either. \$\endgroup\$ Jan 9, 2015 at 12:30
  • \$\begingroup\$ Then how would you do it? \$\endgroup\$ Jan 9, 2015 at 12:34
  • \$\begingroup\$ This answer shows how you can reuse the same Random object to generate all numbers. \$\endgroup\$ Jan 9, 2015 at 14:45
  • \$\begingroup\$ Oh, sure @Mat'sMug. On to it now :) \$\endgroup\$ Jan 10, 2015 at 0:18

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.