6
\$\begingroup\$

I'm using the code below to save a record in my database. I was thinking that I am creating new instances of the SQL Command object and fear that I might be bloating the system memory and not doing proper garbage collection for unused objects. Can anyone point out which parts of this code I can improve?

By the way, this is the first time I've used SQL Transactions.

Private Sub SaveRecord()
    Dim type As Integer = 0

    Try
        Using dbConn As New SqlConnection(strConnStr)
            Try
                dbConn.Open()

                _params = New List(Of SqlParameter)

                ' Register record
                If formMode = 1 Then
                    dbCmd = dbConn.CreateCommand
                    dbTran = dbConn.BeginTransaction(IsolationLevel.ReadCommitted, "RBT")

                    If OrganismID = -1 Then
                        dbCmd = New SqlCommand("spNewZEntity", dbConn, dbTran)
                        dbCmd.CommandType = CommandType.StoredProcedure
                        AssignZEntityParam()

                        ZEntityID = dbCmd.ExecuteScalar
                        If ZEntityID = Nothing Then Throw New Exception("Failed to Generate ZEntityID")
                    End If

                    dbCmd = New SqlCommand("spNewXEntity", dbConn, dbTran)
                    dbCmd.CommandType = CommandType.StoredProcedure

                    AssignXEntityParam()
                    XEntityID = dbCmd.ExecuteScalar()

                    If XEntityID = Nothing Then Throw New Exception("Failed to Generate XEntityID")

                    For Each AZ In _AZItems
                        If AZ.IDAZ = -1 Then
                            dbCmd = New SqlCommand("spNewAZ", dbConn, dbTran)
                            dbCmd.CommandType = CommandType.StoredProcedure

                            _params.Clear()
                            dbCmd.Parameters.AddWithValue("@AZName", AZ.nameAZ)
                            AZID = dbCmd.ExecuteScalar()

                            If AZID = Nothing Then Throw New Exception("Failed to Generate AZID")
                            type = 1
                        Else
                            type = 2
                        End If

                        dbCmd = New SqlCommand("spNewISO", dbConn, dbTran)
                        dbCmd.CommandType = CommandType.StoredProcedure

                        AssignCreateISOParam(type, AZ)
                        dbCmd.ExecuteNonQuery()
                    Next

                    dbTran.Commit()
                    dbConn.Close()

                    MsgBox("Done.", MsgBoxStyle.Information)
                End If

                flagSaved = True
                Dispose()
            Catch ex As Exception
                CreateLog("SaveRecord", ex.Message, ex)

                Try
                    MsgBox("Attempting to rollback changes to database...", MsgBoxStyle.Information, emParsing)

                    If Not dbTran.Connection Is Nothing Then
                        dbTran.Rollback("RBT")
                        dbTran.Dispose()

                        MsgBox("Rollback successful.", MsgBoxStyle.Information, emParsing)
                    Else
                        MsgBox("Failed to rollback changes. Please contact your system administrator.",
                               MsgBoxStyle.Exclamation, emParsing)
                    End If
                Catch ex2 As SqlException
                    CreateLog("SaveRecord/Rollback Failed", ex2.Message, ex2)
                    MsgBox("Failed to rollback changes. Please contact your system administrator.", MsgBoxStyle.Exclamation, "Rollback Failed")
                End Try
            Finally
                dbCmd.Dispose()
            End Try
        End Using
    Catch ex As Exception
        CreateLog("SaveRecord", ex.Message, ex)
    End Try
End Sub
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Not clear what AssignCreateISOParam(type, AZ) is and why it is above dbCmd.ExecuteNonQuery()

Create the command and parameter(s) outside the loop. In the loop just assign the value and execute.

dbCmd = New SqlCommand("spNewAZ", dbConn, dbTran)
\$\endgroup\$
2
  • \$\begingroup\$ The AssignCreateISOParam assigns the parameters to the dbCmd object. If I'm not mistaken, parameters should be assigned prior to executing the query? Also, when you meant to move the dbCmd = New SQLCommand(...) code, does that mean that the SQL Command object using the spNewAZ SPROC will be reused even with a different set of parameters per loop? \$\endgroup\$ Commented Jan 10, 2017 at 9:50
  • \$\begingroup\$ Answer is only as good as the question \$\endgroup\$
    – paparazzo
    Commented Jan 10, 2017 at 13:33
0
\$\begingroup\$

What should happen if formMode <> 1 ? It seems nothing, So you won't need to create a SqlConnection, open it and create a List(Of SqlParameter).

By checking at the top of the method

If formMode <> 1 Then
    Exit Sub
End if  

You will save one level of indentation which makes your code easier to read.


This

dbCmd = dbConn.CreateCommand  

is superflous because the value of dbCmd is overwritten some lines below. You can safely remove it.


The use of _params by first calling _params = New List(Of SqlParameter) and in a loop calling _params.Clear() doesn't seem to buy you anything. That object isn't used anywhere else in this method.


                    dbCmd = New SqlCommand("spNewZEntity", dbConn, dbTran)
                    dbCmd.CommandType = CommandType.StoredProcedure
                    AssignZEntityParam()

                    ZEntityID = dbCmd.ExecuteScalar
                    If ZEntityID = Nothing Then Throw New Exception("Failed to Generate ZEntityID")  

and

                dbCmd = New SqlCommand("spNewXEntity", dbConn, dbTran)
                dbCmd.CommandType = CommandType.StoredProcedure

                AssignXEntityParam()
                XEntityID = dbCmd.ExecuteScalar()

                If XEntityID = Nothing Then Throw New Exception("Failed to Generate XEntityID")  

should be extracted to well named methods to which you should pass the SqlConnection and SqlTransaction objects. This will help your code regarding readability because it makes the method shorter. In addition this will make the responsibilities of each method more clear.


You are using a lot of class level variables in this method and it seems that this variables will be out of scope after the method has been executed succesful because you call Dispose() at after the transaction had been commited and the connection had been closed.

Calling Dispose() shouldn't be done by the object itself but by the user of that object. See: Should a .Net/C# object call Dispose() on itself?

\$\endgroup\$
1
  • \$\begingroup\$ There's another set of code for a different formMode. I read the attached link. And yes, there's a lot of class level variables in the code (primarily data drawn from a Dataset) that I pass from one sub to another. Are localized (ie: variables limited to a sub) variables a better approach for this? _params is populated by the Assign... subs. The reason why I call Dispose() is because the form closes after the sub successfully creates the record. \$\endgroup\$ Commented Jan 10, 2017 at 9:57

Your Answer

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

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