Skip to main content
Code Review

Return to Answer

handle `guess` better
Source Link
Wai Ha Lee
Wai Ha Lee
  • 150
  • 2
  • 10

6. Introduce a variable to represent the state of the user input

Currently you have:

Dim guess As Integer = -1

Do
    Console.Write("Enter a guess between " + MinValue.ToString() + " and " + MaxValue.ToString() + ": ")
    If Not Integer.TryParse(Console.ReadLine(), guess) Then
        guess = -1
    End If
Loop Until guess >= MinValue And guess <= MaxValue

Return guess

but you could instead go for something like:

Dim guess As Integer = -1
Dim validInput As Boolean = false

Do
    Console.Write("Enter a guess between " + MinValue.ToString() + " and " + MaxValue.ToString() + ": ")
    validInput = Integer.TryParse(Console.ReadLine(), guess)
Loop Until validInput And guess >= MinValue And guess <= MaxValue

Return guess

(Apologies if that doesn't build - I'm from C# land).

That way, you don't set the value of guess if it's invalid and the intent is clearer.

7 (maybe). Reuse the same instance of System.Random.

I think you're already doing this - my VB.NET knowledge is weak (I'm fromagain, C# land). The default constructor for Random seeds the generator with a time-dependant seed, so multiple instances created in (very) quick succession will give the same values.

6 (maybe). Reuse the same instance of System.Random.

I think you're already doing this - my VB.NET knowledge is weak (I'm from C# land). The default constructor for Random seeds the generator with a time-dependant seed, so multiple instances created in (very) quick succession will give the same values.

6. Introduce a variable to represent the state of the user input

Currently you have:

Dim guess As Integer = -1

Do
    Console.Write("Enter a guess between " + MinValue.ToString() + " and " + MaxValue.ToString() + ": ")
    If Not Integer.TryParse(Console.ReadLine(), guess) Then
        guess = -1
    End If
Loop Until guess >= MinValue And guess <= MaxValue

Return guess

but you could instead go for something like:

Dim guess As Integer = -1
Dim validInput As Boolean = false

Do
    Console.Write("Enter a guess between " + MinValue.ToString() + " and " + MaxValue.ToString() + ": ")
    validInput = Integer.TryParse(Console.ReadLine(), guess)
Loop Until validInput And guess >= MinValue And guess <= MaxValue

Return guess

(Apologies if that doesn't build - I'm from C# land).

That way, you don't set the value of guess if it's invalid and the intent is clearer.

7 (maybe). Reuse the same instance of System.Random.

I think you're already doing this - my VB.NET knowledge is weak (again, C# land). The default constructor for Random seeds the generator with a time-dependant seed, so multiple instances created in (very) quick succession will give the same values.

I was wrong about the wasted guess
Source Link
Wai Ha Lee
Wai Ha Lee
  • 150
  • 2
  • 10

4. Don't set a default value for guess.

In InputGuess, if a user enters a value which can't be parsed into an integer, guess is set to -1 and it still counts as one of the six guesses. Maybe introduce another while loop to guarantee a valid integer is entered.

5. Use Else.

The conditions are mutually exclusive, so once one is satisfied there's no need to check the others.

65. Consider letting the user specify the min and max values which bound the random number.

76 (maybe). Reuse the same instance of System.Random.

4. Don't set a default value for guess.

In InputGuess, if a user enters a value which can't be parsed into an integer, guess is set to -1 and it still counts as one of the six guesses. Maybe introduce another while loop to guarantee a valid integer is entered.

5. Use Else.

6. Consider letting the user specify the min and max values which bound the random number.

7 (maybe). Reuse the same instance of System.Random.

4. Use Else.

The conditions are mutually exclusive, so once one is satisfied there's no need to check the others.

5. Consider letting the user specify the min and max values which bound the random number.

6 (maybe). Reuse the same instance of System.Random.

Source Link
Wai Ha Lee
Wai Ha Lee
  • 150
  • 2
  • 10

Here are a few suggestions. n.b. I'm on my phone so can't run your code or see all of it on one screen.

1. Don't give boolean members negative names.

If you rename ExitGame() to something like KeepPlaying(), you'll have

Do
    PlayGame()
Loop While KeepPlaying()

instead of

Do
    PlayGame()
Loop Until Not ExitGame()

which (in my opinion) reads better. Once you get more conditions, having e.g. Not Invalid And Not NoValues starts to make it easier to introduce bugs because you need to think about the logic.

2. Consider using Console.ReadKey instead of Console.ReadLine.

Nobody wants to keep typing "yes" and pressing Enter - if you use ReadKey, you can just filter for "y" or "n".

3. Rename Prompt.

Its function is too give feedback, rather than to prompt the user. Maybe call it Respond, or GiveFeedback?

4. Don't set a default value for guess.

In InputGuess, if a user enters a value which can't be parsed into an integer, guess is set to -1 and it still counts as one of the six guesses. Maybe introduce another while loop to guarantee a valid integer is entered.

5. Use Else.

If guess > num Then
    Console.WriteLine("Your guess is too high.")
End If
If guess < num Then
    Console.WriteLine("Your guess is too low.")
End If
If guess = num Then
    Console.WriteLine("You won!")
End If

could be

If guess > num Then
    Console.WriteLine("Your guess is too high.")
Else If guess < num Then
    Console.WriteLine("Your guess is too low.")
Else If guess = num Then
    Console.WriteLine("You won!")
End If

6. Consider letting the user specify the min and max values which bound the random number.

This would 'spice things up' and make the game more interesting (maybe). Use Math.Log(range, 2) as a guide to the number of guesses to allow the user.

7 (maybe). Reuse the same instance of System.Random.

I think you're already doing this - my VB.NET knowledge is weak (I'm from C# land). The default constructor for Random seeds the generator with a time-dependant seed, so multiple instances created in (very) quick succession will give the same values.