2
\$\begingroup\$

I have inherited this code from my company. It is acting as a helper class for a database and written in VB.NET until date. It is working properly and almost 5 to 10 commercial packages I am maintaining is written based on this work. I just want to know if it can be improved. It invariably uses Async, and I am not sure about it.

  Public Overloads Function bindData(ByVal qry As String, ByVal param() As MySqlParameter, Optional ByVal retRows As CommandBehavior = CommandBehavior.Default) As DataTable

    Try


        Using d = New MySqlConnection(connectionString)
            Using cmd = d.CreateCommand
                cmd.CommandText = qry
                cmd.CommandType = CommandType.Text

                cmd.Parameters.AddRange(param)
                If d.State = ConnectionState.Closed Then d.Open()
                Dim iResult As IAsyncResult = cmd.BeginExecuteReader(retRows)
                While Not iResult.IsCompleted
                    Threading.Thread.Sleep(100)
                End While

                If iResult.IsCompleted Then
                    mydr = cmd.EndExecuteReader(iResult)

                End If
                Dim dt As New DataTable
                dt.Load(mydr)
                Return dt

            End Using
        End Using
    Catch ex As Exception
        Throw ex
    End Try

End Function
Public Overloads Function bindData(ByVal qry As String, Optional ByVal retRows As CommandBehavior = CommandBehavior.Default) As DataTable

    Try

        Using d = New MySqlConnection(connectionString)
            Using cmd As New MySqlCommand(qry, d)
                If d.State = ConnectionState.Closed Then d.Open()
                cmd.CommandType = CommandType.Text
                Dim iResult As IAsyncResult = cmd.BeginExecuteReader(retRows)
                While Not iResult.IsCompleted
                    Threading.Thread.Sleep(100)
                End While

                If iResult.IsCompleted Then
                    mydr = cmd.EndExecuteReader(iResult)

                End If
                Dim dt As New DataTable
                dt.Load(mydr)
                Return dt
            End Using
        End Using

    Catch ex As Exception
        Throw ex
    End Try

End Function
Public Overloads Function bindProcedure(ByVal spname As String, Optional ByVal retRows As CommandBehavior = CommandBehavior.Default) As DataTable

    Try


        Using d = New MySqlConnection(connectionString)
            Using cmd = d.CreateCommand
                cmd.CommandText = spname
                cmd.CommandType = CommandType.StoredProcedure
                If d.State = ConnectionState.Closed Then d.Open()
                Dim iResult As IAsyncResult = cmd.BeginExecuteReader(retRows)
                While Not iResult.IsCompleted
                    Threading.Thread.Sleep(100)
                End While

                If iResult.IsCompleted Then
                    mydr = cmd.EndExecuteReader(iResult)

                End If
                Dim dt As New DataTable
                dt.Load(mydr)
                Return dt

            End Using
        End Using
    Catch ex As Exception
        Throw ex
    End Try

End Function
Public Overloads Function bindProcedure(ByVal spname As String, ByVal inparam() As MySqlParameter, Optional ByVal retRows As CommandBehavior = CommandBehavior.Default) As DataTable

    Try

        Using d = New MySqlConnection(connectionString)
            Using cmd = d.CreateCommand
                cmd.CommandText = spname
                cmd.CommandType = CommandType.StoredProcedure

                cmd.Parameters.AddRange(inparam)
                If d.State = ConnectionState.Closed Then d.Open()
                Dim iResult As IAsyncResult = cmd.BeginExecuteReader(retRows)
                While Not iResult.IsCompleted
                    Threading.Thread.Sleep(100)
                End While

                If iResult.IsCompleted Then
                    mydr = cmd.EndExecuteReader(iResult)

                End If
                Dim dt As New DataTable
                dt.Load(mydr)
                Return dt

            End Using
        End Using
    Catch ex As Exception
        Throw ex
    End Try

End Function
Public Overloads Function ExecuteQuery(ByVal qry As String, ByVal param() As MySqlParameter) As Boolean
    Dim retval As Boolean = False
    Try


        Using d = New MySqlConnection(connectionString)
            Using cmd = d.CreateCommand
                cmd.CommandText = qry
                cmd.CommandType = CommandType.Text

                cmd.Parameters.AddRange(param)
                If d.State = ConnectionState.Closed Then d.Open()
                Dim iResult As IAsyncResult = cmd.BeginExecuteNonQuery()
                While Not iResult.IsCompleted
                    Threading.Thread.Sleep(100)
                End While

                If iResult.IsCompleted Then
                    If cmd.EndExecuteNonQuery(iResult) > 0 Then
                        retval = True
                    Else
                        retval = False
                    End If

                End If


            End Using
        End Using

    Catch ex As Exception
        Throw ex
        retval = False
    End Try
    Return retval
End Function
Public Overloads Function ExecuteQuery(ByVal qry As String) As Boolean
    Dim retval As Boolean = False
    Try

        Using d = New MySqlConnection(connectionString)
            Using cmd = d.CreateCommand
                cmd.CommandText = qry
                cmd.CommandType = CommandType.Text

                If d.State = ConnectionState.Closed Then d.Open()
                Dim iResult As IAsyncResult = cmd.BeginExecuteNonQuery()
                While Not iResult.IsCompleted
                    Threading.Thread.Sleep(100)
                End While

                If iResult.IsCompleted Then
                    If cmd.EndExecuteNonQuery(iResult) > 0 Then
                        retval = True
                    Else
                        retval = False
                    End If

                End If

            End Using
        End Using

    Catch ex As Exception
        Throw ex
        retval = False
    End Try
    Return retval
End Function
Improve this question
\$\endgroup\$

2 Answers 2

Reset to default
4
\$\begingroup\$

This isn't a Catch block. It's a stack trace rewriter:

Catch ex As Exception
    Throw ex
    retval = False
End Try

The correct way of rethrowing the exception that was caught without losing the original stack trace, is to use the Throw keyword all by itself.

Then, retval = False is dead code - you're throwing an exception, so execution immediately jumps out and the exception bubbles up the stack until it finds another Catch block.

In other words, the whole Try/Catch block is useless here, the only purpose it serves, is to hide where the error is actually coming from. Get rid of it, and handle the actual exception wherever you're actually catching ex.

...which takes me to the next point: the only value you'll ever get for retVal, is True - if it's not True, then you're dealing with an exception. Make it a Sub, not a Function - you don't care what the return value is, just whether it succeeded (happy path) or failed (exception).

This is also quite wrong:

Dim iResult As IAsyncResult = cmd.BeginExecuteNonQuery()
While Not iResult.IsCompleted
    Threading.Thread.Sleep(100)
End While

Don't sleep-wait for an asynchronous task - Await it!

Improve this answer
\$\endgroup\$
3
  • 2
    \$\begingroup\$ There's a good example of await here. \$\endgroup\$
    – RubberDuck
    Apr 29, 2015 at 20:11
  • \$\begingroup\$ thanks for the inputs , currently all the projects are maintained via vs2010 and i think for await i have to add async ctp , which i have to get consent from higher ups. so can you suggest any other method. \$\endgroup\$
    – sansknwoledge
    Apr 30, 2015 at 5:44
  • 1
    \$\begingroup\$ I'd use Task.Run to run asynchronously, and Task.Wait to wait for it to finish. \$\endgroup\$
    – Mathieu Guindon
    Apr 30, 2015 at 14:12
1
\$\begingroup\$

In addition to @Mat's Mug's answer

In its current state you can omit the Try..Cacth completely, because if an exception is thrown you are throwing it (also in a wrong way) anyway. So by omitting the Try..Catch you not only won't loose anything but keep the original stacktrace and reduce horizontal spacing.

Shortening parameter/variable names does not add any value. In fact it reduces readability and therfor reduces maintainability. So do yourself a favour and use descriptive and meaningful names. 

qry -> query
retRows -> behaviour
d -> connection
mydr -> dataReader

Based on the naming guidelines methods should be named using PascalCase casing.

If iResult.IsCompleted Then

this If statement can be omitted because it is following the While Not iResult.IsCompleted loop.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.