Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm working on a random number guessing game. I'm done with the code and it's working, but there is a part that I want to make better. I declared an instance of a Guess class I created, and now how to make this part more efficient.

int counter = 0;
do
{             
    myGuess.UserGuess = GetUserGuess(); //read user guess
    if (myGuess.Compair() == "match")
    {
        Console.WriteLine("\n\t Correct!You WIN !");
    }


    else if (myGuess.Compair() == "high")
    {
        if (counter < 3)
            Console.WriteLine("\n\tTry a lower number,");
        else
            Console.WriteLine("\n\tSorry you LOSE !, The right number is " + myGuess.RndNum);

        counter++;
    }

    else if (myGuess.Compair() == "low")
    {
        if (counter < 3)
            Console.WriteLine("\n\tTry a higher number,");
        else
            Console.WriteLine("\n\tSorry you LOSE !, The right number is " + myGuess.RndNum);
        counter++;
    }


} while (myGuess.Compair() != "match" && counter < 4);

For example, I used the same message the same condition twice, which I think is not the best way. Any way to loop better than that?

share|improve this question
add comment

3 Answers 3

up vote 6 down vote accepted

I think a way to make this code generally more efficient and predictable is to have your classes have a more defined role. For example, it seems like you're defining an instance of Guess as myGuess, then externally loading in a random number (RndNum), and the current guess (UserGuess = GetUserGuess()).

You should clearly define the goals for classes, such as:

  • Manage the Game lifecycle.
  • Handle user input, ensuring the user enters only numbers and is properly prompted, etc.

In this way, your code might look more like this psuedo-code:

class GuessingGame
{
    static void Main(string[] args)
    {
        game = new GuessingGame();
        game.run();
    }

    int numGuesses;
    int correctNumber;
    UserInputHandler userInputHandler;

    GuessingGame(numGuesses = 4)
    {
        Random random = new Random();
        this.correctNumber = random.Next(10);
        this.numGuesses = numGuesses;
        this.userInputHandler = new UserInputHandler();
    }

    void run()
    {
        for (int thisGuess = 0; thisGuess < numGuesses; ++thisGuess)
        {
            Console.WriteLine(string.Format("Guess #{0}!\n", thisGuess + 1));
            guess = this.userInputHandler.getGuess();
            if (guess < this.correctNumber)
            {
                Console.WriteLine("WRONG! Try a higher number!");
            }
            else if (guess > this.correctNumber)
            {
                Console.WriteLine("WRONG! Try a lower number!");
            }
            else
            {
                win();
                return();
            }
        }
        lose();
    }

    void win()
    {
        Console.WriteLine("You WIN!  The number was indeed " + this.correctNumber);
    }

    void lose()
    {
        Console.WriteLine("You LOSE!  The number was " + this.correctNumber);
    }
}

// UserInputHandler Source
class UserInputHandler
{
    public int getGuess()
    {
        // Handle user input, and ensure the guesses are actually numbers.
    }
}

The idea is that each class is responsible for encapsulating one piece of functionality around another class. The Main program entry point doesn't care about how the game works, the GuessingGame class doesn't care about interacting with the user at all, while the UserInputHandler class doesn't care about the correct answer, etc.

I've also wrapped the win/loss messages into a single method, so that you're not repeating the victory message.

Some benefits include:

  • Having a clear understanding of the role of each class.
  • Being able to assert that with unit tests easily.
  • Code reuse. You could plug the UserInputClass into a different game that requires simple user input, for example.
  • Collaboration. Properly scoped and documented code means other developers could jump into your project with less friction.

Now this example is a bit much considering that a guessing game is fairly basic, but it's good practice for tackling larger projects.

share|improve this answer
    
Thank you ,that is so insightfull answer :) –  sunflower Mar 2 at 3:09
add comment

You can use a switch statement instead of if else if else if.

You don't need to test if (counter < 3) inside the do loop; instead you could exit the do loop when the count is too high.

Similarly you could print the error message once if/after you exit the do loop unsuccessfully.

So:

for (int counter = 0; counter < 3; ++counter)
{
    myGuess.UserGuess = GetUserGuess(); //read user guess
    switch (myGuess.Compare()) 
    {
        case "match":
            Console.WriteLine("\n\t Correct!You WIN !");
            return true; // tell caller than the guess was successful
        case "high"
            Console.WriteLine("\n\tTry a lower number,");
            break;
        case "low"
            Console.WriteLine("\n\tTry a higher number,");
            break;
        default:
            throw new NotImplementedException(); // unexpected Compare result
    }
}
Console.WriteLine("\n\tSorry you LOSE !, The right number is " + myGuess.RndNum);
return false;
share|improve this answer
1  
I like the switch option. I might also consider using enums instead of string literals as well as passing userGuess into the Compare method rather than a public property setter... –  dreza Mar 2 at 8:52
add comment

@ChrisW has a good answer. The code is clean.

You could also consider having your comparison return an int:

  • 0 for the same
  • -1 if the guess is less than the number
  • 1 if the guess is greater than the number

It would basically be wrapping the CompareTo method for whatever your guess data type is. There are more CompareTo methods, but here are a couple examples:

http://msdn.microsoft.com/en-us/library/y2ky8xsk(v=vs.110).aspx

http://msdn.microsoft.com/en-us/library/fyxd1d26(v=vs.110).aspx

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.