Do While Loop doesn't stop looping in VB.net-CodePudding

I'm trying to generate a random number that's not in the database. If the randomly generated number happens to already be in the database, a message box appears saying the number exists. When you click Ok, it generates another number and if it's still in the database, it will repeat the same process. With my code, it keeps showing the message box and generating a number even after it has already generated a number that's not in the database. This is my code:

Private Sub BtnOrder_Click(sender As Object, e As EventArgs) Handles BtnOrder.Click
    Dim rand As New Random
    Dim num As Integer

    num = rand.Next(1, 30)
    TxtOrder.Text = "#"   num.ToString("0000")
    BtnOrder.Enabled = False

    Dim connection As New OleDbConnection("Provider=Microsoft.ACE.OLEDB.12.0;Data Source=Daily Sales.accdb;")
    Dim command As New OleDbCommand("SELECT [Order No] FROM [Table1] WHERE [Order No] = orderno", connection)
    Dim orderParam As New OleDbParameter("orderno", Me.TxtOrder.Text)
    command.Parameters.Add(orderParam)
    command.Connection.Open()
    Dim reader As OleDbDataReader = command.ExecuteReader()
    Do While reader.HasRows = True
        If reader.HasRows = False Then
            Exit Do
        End If
        MessageBox.Show("Order number exists.", "Invalid Order Number")
        num = rand.Next(1, 30)
        TxtOrder.Text = "#"   num.ToString("0000")
    Loop
    command.Connection.Close()
End Sub

CodePudding user response：

I think many things need to be changed. Some are listed in comments on your question, but here they are

You hit the database multiple times.
You don't follow the Disposable pattern which is implemented by both the connection and the command.
You had a loop whose condition looked for rows, then immediately checks for no rows to exit, which can never happen.
You don't actually check that the order number is in the result set.
You only create random numbers from 1 to 29, but if they all exist, the loop will continue forever.
You perform the database interaction on the UI thread.

You may find that using Using blocks to properly dispose of the Disposable objects helps with memory, and moving the query off the UI helps your UI remain responsive. Also, I don't see a need to alert the user that a random number has found a match in the database - just find a proper random number without alerting the user. Lastly, throw an Exception if you can't generate another order number.

Dim lowerInclusive As Integer = 1, upperExclusive As Integer = 30

Private Async Sub BtnOrder_Click(sender As Object, e As EventArgs) Handles BtnOrder.Click
    BtnOrder.Enabled = False
    Try
        Dim nextOrderNumber = Await GetNextOrderNumber()
        TxtOrder.Text = $"#{nextOrderNumber:0000}"
    Catch ex As Exception
        TxtOrder.Text = "N/A"
        MessageBox.Show(ex.Message)
    Finally
        BtnOrder.Enabled = True        
    End Try
End Sub

Private Function GetNextOrderNumber() As Task(Of Integer)
    Return Task.Run(
        Function()
            Dim existingOrderNumbers As New List(Of Integer)()
            Using connection As New OleDbConnection("Provider=Microsoft.ACE.OLEDB.12.0;Data Source=Daily Sales.accdb;")
                connection.Open()
                Using command As New OleDbCommand("SELECT [Order No] FROM [Table1]", connection)
                    Dim reader As OleDbDataReader = command.ExecuteReader()
                    While reader.Read()
                        existingOrderNumbers.Add(reader.GetInt32(0))
                    End While
                End Using
            End Using
            Dim nextOrderNumbers = Enumerable.Range(lowerInclusive, upperExclusive - lowerInclusive).Except(existingOrderNumbers).ToList()
            If Not nextOrderNumbers.Any() Then Throw New Exception("All possible order numbers are already used.")
            Dim rand As New Random()
            Return nextOrderNumbers(rand.Next(0, nextOrderNumbers.Count()))
        End Function)
End Function

Thinking about it again, the new order number never goes into the database. So when does that happen? You may want that to be an atomic operation, if this code can be used by multiple people - multiple threads. You can use a transaction to lock down the table... or perhaps just use a PK ID as part of a hash to generate some random number.

CodePudding user response：

So if you're looking for unique numbers, I suggest you change the DB column to become an IDENTITY column. That way, SQL server takes care of creating unique (increasing) numbers for you.

Query the current highest order number:

SELECT MAX([Order No]) AS OrderNoCurrent FROM Table1

Query the next order number:

SELECT (MAX([Order No]) 1) AS OrderNoNext FROM Table1