Number guess generator

[cahid6365]

Hello, Firstly I am a student and am very new to VB.Net. I am trying to create a program that is capable of accepting a number from a user and comparing that with a 10 number array that is generated from non roccuring random numbers ranging from 1 to 100. The issue I am having is my Youve guessed wrong message is continuously showing and will not allow termination of teh message box in order to enter another number. I also am having difficulty getting my program to read the numbers in the array, which is printed in a listbox. My code is somewhat horrendous but any guidance on the errors and any ideas would be very helpful.

Public Class frmMain
    Dim MyNum, YourGuess As Integer
    Const MaxNum = 100, ListSize = 10

    Private Sub btnExit_Click(sender As Object, e As EventArgs) Handles btnExit.Click
        Me.Close()
    End Sub

    Private Sub frmMain_Load(sender As Object, e As EventArgs) Handles MyBase.Load
        txtYourNum.Focus()
        MessageBox.Show("Enter A number from 0 to 99")
    End Sub

    Private Sub btnLottery_Click(sender As Object, e As EventArgs) Handles btnLottery.Click
        Dim Check(MaxNum) As Byte
        For I = 1 To ListSize
            Do
                MyNum = Int(Rnd() * MaxNum) + 1
                If Check(MyNum) = 0 Then
                    ListBox1.Items.Add(MyNum)
                    Check(MyNum) = 1
                    Exit Do
                End If
            Loop
        Next
        Do
            YourGuess = CInt(txtYourNum.Text)
            NewMethod()

            If YourGuess > 100 Then
                MessageBox.Show("I take pity on you")
            End If
        Loop
    End Sub

    Private Sub NewMethod()

        If Not IsNumeric(YourGuess) Then
            MessageBox.Show(YourGuess & " is not numeric. Try again.")
        ElseIf YourGuess < 1 Then
            MessageBox.Show(YourGuess & " is not a valid number. Try Again.")
        ElseIf YourGuess = (ListBox1.Items.Contains(MyNum)) Then
            MessageBox.Show("Well Guessed!")
        ElseIf YourGuess <> (ListBox1.Items.Contains(MyNum)) Then
            MessageBox.Show("Nope. Try again.")
        End If

    End Sub
End Class

[dbasnett]

If you search the forum for

random guess

I'll bet you find a lot of good info.

[si_the_geek]

Think carefully about the ElseIf's here:

        ElseIf YourGuess = (ListBox1.Items.Contains(MyNum)) Then
            MessageBox.Show("Well Guessed!")
        ElseIf YourGuess <> (ListBox1.Items.Contains(MyNum)) Then

First of all the second one doesn't need to be an ElseIf, because you only want to check if the previous one failed... so replace that second ElseIf line with just Else


The first of those ElseIf's doesn't make a lot of sense, and I'm not sure what you intended.

The part ListBox1.Items.Contains(MyNum) checks whether MyNum is in ListBox1, and returns either True or False (based on a quick read, I think it will always be True, because MyNum appears to only be set when adding items to the list).

You are then comparing that True or False value to the integer in YourGuess, which isn't something people tend to do on purpose (and doesn't really make much sense here).

I would guess that you intended one of these:

        ElseIf YourGuess = MyNum Then

        ElseIf ListBox1.Items.Contains(YourGuess) Then

[Sitten Spynne]

Your problem's here:

Do
    YourGuess = CInt(txtYourNum.Text)
    NewMethod()

    If YourGuess > 100 Then
        MessageBox.Show("I take pity on you")
    End If
Loop

Loops have two parts. The part that says "do this", and the part that says "until this happens". You left out the "until this happens". If you don't tell a loop when to stop, it will continue forever. So you need a way to tell your loop how to stop.

I think the best way would be to make NewMethod() return a value. It should be a function. Then its purpose becomes "evaluate the guess, and tell me if it was correct." It might look something like this:

Private Function NewMethod() As Boolean
    If YourGuess = ListBox1.Items.Contains(MyNum) Then
        MessageBox.Show("Well guessed!")
        Return True
    End If

    If Not IsNumeric(YourGuess) Then
        ...
    Else If YourGuess < 1 Then
        ...
    ...
    End If

    Return False

The "Return" keyword ends execution of the Sub/Function it is in. For Functions, it should be followed with the value the function will return. So I put the "True" case at the top. If it is true, the "Return True" part will make the method stop executing and none of the other cases will execute. That's why I only put one "Return False" at the bottom: only one of the other three If statements will match, but they all mean "No, this wasn't a correct guess" so I just have one "Return False".

Then your code up top needs to understand when to stop the loop. Since the function returns a value now, it might look like this:

Dim shouldStop As Boolean = False
Do
    YourGuess = CInt(txtYourNum.Text)
    shouldStop = NewMethod()

    If YourGuess > 100 Then
        ...
    End If

Loop Until shouldStop

Now calling NewMethod returns something, so we store the value in a variable. If it returns True, the loop will stop.

[Shaggy Hiker]

Yeah, this one comes up every year, or so.

Still lots of suggestions:

One thing you need to do is to stop using Rnd and Randomize. That's ancient legacy code, and it doesn't look like you are using it quite right, anyways, as I don't see a call to Randomize. Not sure what Rnd will do in that case. Anyways, in .NET you should be creating one instance of the Random object at form scope, and just using that. The .Next method is easier than Rnd.

Another point is that Check shouldn't be Byte. You may be thinking that's efficient, but it really isn't. I would say that you should just use Integer. Bytes may seem smaller, but they are awkward for the CPU, so they are likely dealt with as 4 or 8 byte items anyways, once in the processor. However, Check should really be Boolean, because that's how you are using it. You are checking for 1 or 0, so you might as well check for True or False, instead.

Third point is that CInt will crash if the user either leaves the textbox blank or enters a string that isn't an integer. The only safe conversion for user input is Integer.TryParse. Better still would be to use a NumericUpDown control rather than a textbox, because the NUD.Value property returns a Decimal, and CInt will safely convert that to an Integer, if you even need to.

Fourth point is that the check for IsNumeric in NewMethod is pointless. If that string wasn't a number, the program would have crashed on the CInt call. You'd have to be calling IsNumeric before you tried to convert the string for that to have any benefit.

The bigger point is that you should put a breakpoint in NewMethod and step through that. Breakpoints and code stepping is something that should be taught the first or second class in programming, but is often left out entirely. That's the tool you need, but whether you know about it is a different matter. If you set a breakpoint on the first If, you'd see that was false, then you'd step to the first ElseIf, which I kind of think is wrong. Why isn't 0 a valid number? You did ask them for a number in a certain range, and you have good sarcasm if they guess 100 times without getting it right. Why not some sarcasm if they entered a number that wasn't in the range you asked for?

Anyways, assuming that the number was 1 or greater, you'd then go to the second ElseIf, and that's where the problem with the code is found. (ListBox1.Items.Contains(MyNum)) will return either True or False, and there is no way that equals whatever number the user guessed. ALL you want to do is see whether or not the listbox contains the number, not compare it to anything.

However, I assume that the listbox is hidden, because it should be pretty doggone easy for a person to guess a number in a list when they can see the list. Therefore, a listbox is not a good thing to be using here. If that's all you've learned, thus far, then go right ahead. What you should be using is a List(of Integer). It's smaller, faster, works better, and would make your earlier loop for filling it a whole lot simpler.


EDIT: Cool, Sitten and I both found problems, but didn't find the same ones. He's right that the loop will never end. I read the problem as being, "why am I seeing the wrong messagebox".

SECOND EDIT: I see that I was REALLY slow, as Si also got in there. Hopefully, I added other value.