Reputation: 7638
I am trying to code a simple Tic tac toe with 9 buttons . When i click any of buttons i am shown a message box with a WIN dialog . Though this message box is meant to show only when a WIN CASE occurs . The problem is it that it is not comparing Button.Text values correctly.
Initially All button's TEXT property is empty .
On click i am setting their TEXT to "x" or "0" as the case maybe .
This is suppose to work correctly but cant figure why it is not comparing the newly assigned TEXT of clicked
button with other Button's Text.
Here is my code
Public Class Form1
Dim count As Int32 = 0
Private Sub Form1_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
End Sub
Sub checkwin()
If Button1.Text = Button2.Text And Button2.Text = Button3.Text Then
MessageBox.Show(Button1.Text & "wins")
Call reset()
ElseIf Button4.Text = Button5.Text And Button5.Text = Button6.Text Then
MessageBox.Show(Button4.Text & "wins")
Call reset()
ElseIf Button7.Text = Button8.Text And Button8.Text = Button9.Text Then
MessageBox.Show(Button7.Text & "wins")
Call reset()
ElseIf Button1.Text = Button4.Text And Button4.Text = Button7.Text Then
MessageBox.Show(Button1.Text & "wins")
Call reset()
ElseIf Button2.Text = Button5.Text And Button5.Text = Button8.Text Then
MessageBox.Show(Button2.Text & "wins")
Call reset()
ElseIf Button3.Text = Button6.Text And Button6.Text = Button9.Text Then
MessageBox.Show(Button3.Text & "wins")
Call reset()
ElseIf Button1.Text = Button5.Text And Button5.Text = Button9.Text Then
MessageBox.Show(Button1.Text & "wins")
Call reset()
ElseIf Button3.Text = Button5.Text And Button5.Text = Button7.Text Then
MessageBox.Show(Button3.Text & "wins")
Call reset()
End If
End Sub
Sub reset()
For Each b as Button in Controls
b.Text = " "
b.Enabled = True
Next
End Sub
Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click, Button2.Click, Button3.Click, Button4.Click, Button5.Click, Button6.Click, Button7.Click, Button8.Click, Button9.Click
If sender Is Button1 Then
count = count + 1
If count Mod 2 = 0 Then
Button1.Text = "0"
Else
Button1.Text = "x"
End If
Button1.Enabled = False
Call checkwin()
End If
If sender Is Button2 Then
count = count + 1
If count Mod 2 = 0 Then
Button2.Text = "0"
Else
Button2.Text = "x"
End If
Button2.Enabled = False
Call checkwin()
End If
If sender Is Button3 Then
count = count + 1
If count Mod 2 = 0 Then
Button3.Text = "0"
Else
Button3.Text = "x"
End If
Button3.Enabled = False
Call checkwin()
End If
If sender Is Button4 Then
count = count + 1
If count Mod 2 = 0 Then
Button4.Text = "0"
Else
Button4.Text = "x"
End If
Button4.Enabled = False
Call checkwin()
End If
If sender Is Button5 Then
count = count + 1
If count Mod 2 = 0 Then
Button5.Text = "0"
Else
Button5.Text = "x"
End If
Button5.Enabled = False
Call checkwin()
End If
If sender Is Button6 Then
count = count + 1
If count Mod 2 = 0 Then
Button6.Text = "0"
Else
Button6.Text = "x"
End If
Button6.Enabled = False
Call checkwin()
End If
If sender Is Button7 Then
count = count + 1
If count Mod 2 = 0 Then
Button7.Text = "0"
Else
Button7.Text = "x"
End If
Button7.Enabled = False
Call checkwin()
End If
If sender Is Button8 Then
count = count + 1
If count Mod 2 = 0 Then
Button8.Text = "0"
Else
Button8.Text = "x"
End If
Button8.Enabled = False
Call checkwin()
End If
If sender Is Button9 Then
count = count + 1
If count Mod 2 = 0 Then
Button9.Text = "0"
Else
Button9.Text = "x"
End If
Button9.Enabled = False
Call checkwin()
End If
End Sub
End Class
Upvotes: 1
Views: 1476
Reputation: 1780
You're not eliminating unused buttons in the checkWin
sub's If
statements. You should also use short-circuiting comparisons to avoid unnecessary processing. So, the first If
statement would look like this:
If Not Button1.Enabled AndAlso Not Button2.Enabled AndAlso Not Button3.Enabled AndAlso _
(Button1.Text = Button2.Text) AndAlso (Button2.Text = Button3.Text) Then
...
ElseIf Not Button4.Enabled ... 'etc.
Oh and all those If
blocks in your button click event are unnecessary. Just use the sender object, like so:
Private Sub Button1_Click(sender As System.Object, e As System.EventArgs) _
Handles Button1.Click, Button2.Click, Button3.Click, Button4.Click, Button5.Click,
Button6.Click, Button7.Click, Button8.Click, Button9.Click
sender.Text = If(clickCount Mod 2 = 0, "X", "O")
sender.Enabled = False
clickCount += 1
CheckWin()
End Sub
Edit (requested in comments)
To avoid the repetitive comparison code in the CheckWin sub, you can farm out the work to a function. That way the comparison code is only written once and consequently far less prone to errors and much easier to read. Something like this is what I would do:
Private Sub CheckWin()
If LineWins(Button1, Button2, Button3) OrElse _
LineWins(Button4, Button5, Button6) OrElse _
LineWins(Button7, Button8, Button9) OrElse _
LineWins(Button1, Button4, Button7) OrElse _
LineWins(Button2, Button5, Button8) OrElse _
LineWins(Button3, Button6, Button9) OrElse _
LineWins(Button1, Button5, Button9) OrElse _
LineWins(Button3, Button5, Button7) Then ResetGame()
End Sub
Private Function LineWins(b1 As Button, b2 As Button, b3 As Button) As Boolean
LineWins = CBool(Not b1.Enabled AndAlso Not b2.Enabled AndAlso Not b3.Enabled _
AndAlso (b1.Text = b2.Text) AndAlso (b2.Text = b3.Text))
If LineWins Then MessageBox.Show(b1.Text & " wins")
End Function
By the way, you may notice I've renamed "reset" to "ResetGame" – I recommend you do this as Reset
is already assigned to a .NET FileSystem
method.
Upvotes: 3