Writing multiple if tags that compare two variables

Question

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);
            }
        }
    }

Answer 1

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 ;)

Answer 2

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)");

Answer 3

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);
}