Reputation: 1232
Where is my loop evaluating wrong?
I'm writing a Visual Basic program to emulate a Tic Tac Toe game. I have a method that, to my knowledge, should correctly draw the board and fill it with Xs and Os appropriately. However when I test it, it will only draw them in the same row.
"Sub board" draws everything, testing for existing values of "x" and "o" and respectively calls "drawX" or "drawO", passing the coordinates to draw.
Sub Main()
Dim start As String = String.Empty
Dim x As ArrayList = New ArrayList
Dim o As ArrayList = New ArrayList
Dim choice As String = String.Empty
o.Add("1")
o.Add("5")
o.Add("9")
While True
board(x, o)
pause()
End While
End Sub
Sub pause()
Console.WriteLine("Press enter to continue...")
Console.ReadLine()
End Sub
Sub board(x As ArrayList, o As ArrayList)
Console.ForegroundColor = 8
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine("**********************************************************")
Console.WriteLine("**********************************************************")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine("**********************************************************")
Console.WriteLine("**********************************************************")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Console.WriteLine(" ** ** ")
Dim ver As Integer = 1
Dim hor As Integer = 4
Dim vSpace As Integer = 11
Dim hSpace As Integer = 20
Dim vPos As Integer = 0
Dim hPos As Integer = 0
For i As Integer = 1 To 9
If 1 <= i <= 3 Then
vPos = ver
Select Case i
Case 1
hPos = hor
Case 2
hPos = hor + hSpace
Case 3
hPos = hor + (hSpace * 2)
Case Else
End Select
End If
If 4 <= i <= 6 Then
vPos = ver + vSpace
Select Case i
Case 4
hPos = hor
Case 5
hPos = hor + hSpace
Case 6
hPos = hor + (hSpace * 2)
Case Else
End Select
End If
If 7 <= i <= 9 Then
vPos = ver + (vSpace * 2)
Select Case i
Case 7
hPos = hor
Case 8
hPos = hor + hSpace
Case 9
hPos = hor + (hSpace * 2)
Case Else
End Select
End If
Select Case True
Case x.Contains(CStr(i))
drawX(hPos, vPos)
Case o.Contains(CStr(i))
drawO(hPos, vPos)
End Select
Next i
End Sub
Sub drawX(hPos As Integer, vPos As Integer)
Console.ForegroundColor = 6
Console.SetCursorPosition(hPos, vPos)
Console.Write("** **")
Console.SetCursorPosition(hPos, vPos + 1)
Console.Write(" ** **")
Console.SetCursorPosition(hPos, vPos + 2)
Console.Write(" ** **")
Console.SetCursorPosition(hPos, vPos + 3)
Console.Write(" ****")
Console.SetCursorPosition(hPos, vPos + 4)
Console.Write(" ** **")
Console.SetCursorPosition(hPos, vPos + 5)
Console.Write(" ** **")
Console.SetCursorPosition(hPos, vPos + 6)
Console.Write("** **")
End Sub
Sub drawO(hPos As Integer, vPos As Integer)
Console.ForegroundColor = 3
Console.SetCursorPosition(hPos, vPos)
Console.Write(" ******")
Console.SetCursorPosition(hPos, vPos + 1)
Console.Write(" ********")
Console.SetCursorPosition(hPos, vPos + 2)
Console.Write("** **")
Console.SetCursorPosition(hPos, vPos + 3)
Console.Write("** **")
Console.SetCursorPosition(hPos, vPos + 4)
Console.Write("** **")
Console.SetCursorPosition(hPos, vPos + 5)
Console.Write(" ********")
Console.SetCursorPosition(hPos, vPos + 6)
Console.Write(" ******")
End Sub
Upvotes: 1
Views: 56
Answers (3)
Reputation: 117027
While the answers given so far are correct - you should be using AndAlso - there is another way of looking at the issue.
Your entire For i As Integer = 1 To 9 loop can be simply replaced by this:
For Each i In x
drawX(hor + hSpace * ((i - 1) Mod 3), ver + vSpace * ((i - 1) \ 3))
Next
For Each i In o
drawO(hor + hSpace * ((i - 1) Mod 3), ver + vSpace * ((i - 1) \ 3))
Next
This is more directly computing the position of the letter.
I would even go further and rewrite your code like this:
Option Strict On
Module Module1
Sub Main()
Dim x = New List(Of Integer)() From {2, 4, 7}
Dim o = New List(Of Integer)() From {1, 5, 9}
While True
board(x, o)
pause()
End While
End Sub
Sub pause()
Console.WriteLine("Press enter to continue...")
Console.ReadLine()
End Sub
Sub board(x As List(Of Integer), o As List(Of Integer))
Console.ForegroundColor = ConsoleColor.DarkGray
Dim vSpace As Integer = 11
Dim hSpace As Integer = 20
Dim ver As Integer = 1
Dim hor As Integer = 4
For i = 1 To 2
For j = 0 To 3 * (vSpace - 1)
Console.SetCursorPosition(i * hSpace - 2, j)
Console.Write("**")
Next
Next
For j = 1 To 2
Console.SetCursorPosition(0, j * vSpace - 2)
Console.Write("".PadRight(3 * hSpace - 2, "*"c))
Console.SetCursorPosition(0, j * vSpace - 2 + 1)
Console.Write("".PadRight(3 * hSpace - 2, "*"c))
Next
For Each i In x
drawX(hor + hSpace * ((i - 1) Mod 3), ver + vSpace * ((i - 1) \ 3))
Next
For Each i In o
drawO(hor + hSpace * ((i - 1) Mod 3), ver + vSpace * ((i - 1) \ 3))
Next
End Sub
Sub drawX(hPos As Integer, vPos As Integer)
drawLetter(hPos, vPos, ConsoleColor.DarkYellow,
{
"** **",
" ** **",
" ** **",
" ****",
" ** **",
" ** **",
"** **"
})
End Sub
Sub drawO(hPos As Integer, vPos As Integer)
drawLetter(hPos, vPos, ConsoleColor.DarkCyan,
{
" ******",
" ********",
"** **",
"** **",
"** **",
" ********",
" ******"
})
End Sub
Sub drawLetter(hPos As Integer, vPos As Integer, color As ConsoleColor, letter As String())
Console.ForegroundColor = color
For i = 0 To 6
Console.SetCursorPosition(hPos, vPos + i)
Console.Write(letter(i))
Next
End Sub
End Module
Upvotes: 1
Reputation: 415665
The line of code below (and other lines like it) does not do what you think:
If 4 <= i <= 6 Then
What actually happens here is that first just the 4 <= i expression is evaluated. Then the boolean result from that expression is used for the <= 6 comparison. To make this comparison, the boolean must first be converted to an integer. The boolean conversion back to integer results in either a 0 (false) or -1 (true) (reference). Both of those are less than 6, and so this full line of code will always result in True.
You need to write it like this, instead:
If 4 <= i AndAlso i <= 6 Then
BTW, are you sure Option Strict is turned on? Because it should be, and if it were I'd expect a compiler error when you try to compare a boolean to an integer. Don't write code without Option Strict, or at least Option Infer. They help greatly to avoid mistakes like this.
Upvotes: 1
Reputation: 881263
I'd be looking at this type of construct:
If 1 <= i <= 3 Then
I'm pretty certain that would evaluate 1 <= i and then compare the result of that with 3. Since the result of a boolean expression is either -1 for true or 0 for false, that expression would always evaluate as true.
You should instead opt for something like:
If 1 <= i And i <= 3 Then ' could also use AndAlso '
(and ditto for the other similar lines).
Upvotes: 1