Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm trying to find out what is the best way to create a class so I can use it to connect to the SQL Server and be able to rollback everything if a single error exists.

Right now, I'm using a single connection for a procedure. This, sometimes, is causing my code to throw There is already an open DataReader associated with this Command which must be closed first.

I thought of setting up a new connection for every transaction but I don't know if I can rollback everything if I do it that way.

My current "way", if a single error exists, I call sBDCloseConnect(True) and it does a rollback and closes the connection. This is useful because I usually do a lot iteration with multiple sql queries.

Below is the relevant code I use to do this and I would like to know if there is a better way to do this and avoid the error thrown I mentioned previously.

One last note, I don't want a solution like MARS.

Test case

'' Initialize connection to database
sBDInitConnect()
Dim test1 As Integer = Val(sBDReturnQuery("SELECT 1"))
If test1 < 0 Then
    '' Has thrown an error
    sBDCloseConnect(True)
ElseIf test1 > 1 Then
    Using dt As New DataTable
        Dim test2 As Integer = sBDSelectSQL("SELECT name FROM users WHERE idUser=1", dt)
        If test2 < 0 Then
            '' Has thrown an error
            sBDCloseConnect(True)
        ElseIf test2 > 0 Then
            If Not sBDNonQuery("UPDATE users SET name='only a test' WHERE idUser=1") Then
                sBDCloseConnect(True)
            Else
                If Not sBDNonQuery("UPDATE users SET name2='this field does not exist' WHERE idUser=1") Then
                    sBDCloseConnect(True)
                End If
            End If
        End If
    End Using
End If
'' No error
sBDCloseConnect(False)

Class

Public Partial Class componenteDEFAULT
    Inherits System.Web.UI.UserControl

    Private bdConection As Data.SqlClient.sqlConnection
    Private bdTransaction As Data.SqlClient.sqlTransaction
    Public bdCommand As Data.SqlClient.SqlCommand

    Sub sBDInitConnect()
        Try : bdConection.Close() : Catch : End Try
        Try
            bdConection = Nothing
            bdConection = New Data.SqlClient.SqlConnection(bd.ConnectString)
            bdConection.Open()

            bdTransaction = Nothing
            bdTransaction = bdConection.BeginTransaction(IsolationLevel.ReadCommitted)

            bdCommand = New Data.SqlClient.SqlCommand()
            bdCommand.Transaction = bdTransaction
            bdCommand.Connection = bdConection
            bdCommand.CommandType = CommandType.Text
            bdCommand.CommandTimeout = 80
        Catch ex As Exception
            sErro(ex.Message)
        End Try
    End Sub

    Function sBDSelectSQL(ByVal comando As String, ByRef rs As Data.DataTable) As Integer
        If rs Is Nothing Then : rs = New DataTable : Else : rs.Clear() : End If
        Try
            bdCommand.Parameters.Clear()
            bdCommand.CommandText = PrepareStringToDB(comando)
            Using dr As Data.SqlClient.SqlDataReader = bdCommand.ExecuteReader(CommandBehavior.Default)
                rs.Load(dr)
            End Using
            Return rs.Rows.Count
        Catch ex As Exception
            sErro(PrepareStringToDB(comando) & " - " & ex.Message)
            Return -1
        End Try
    End Function

    Function sBDNonQuery(ByVal comando As String) As Boolean
        Try
            bdCommand.Parameters.Clear()
            bdCommand.CommandText = PrepareStringToDB(comando)
            bdCommand.ExecuteNonQuery()
            Return True
        Catch ex As Exception
            sErro(PrepareStringToDB(comando) & " - " & ex.Message)
            Return False
        End Try
    End Function

    Function sBDReturnQuery(ByVal comando As String) As String
        Try
            bdCommand.Parameters.Clear()
            bdCommand.CommandText = PrepareStringToDB(comando)
            Return bdCommand.ExecuteScalar()
        Catch ex As Exception
            sErro(PrepareStringToDB(comando) & " - " & ex.Message)
            Return "-1"
        End Try
    End Function

    Sub sBDCloseConnect(ByVal erro As Boolean)
        If erro Then
            Try : bdTransaction.Rollback() : Catch : End Try
            Try : bdConection.Close() : Catch : End Try
        Else
            Try : bdTransaction.Commit() : Catch : End Try
            Try : bdConection.Close() : Catch : End Try
        End If
    End Sub
End Class
share|improve this question

migrated from stackoverflow.com May 8 at 16:46

This question came from our site for professional and enthusiast programmers.

1  
Don't invent the bicycle. Use Microsoft Enterprise Library Data Block - great wrapper for SqlClient –  T.S. May 7 at 19:03
    
@T.S. For that, I need extra configuration and I need this to be easy and quick to change my existing code (this class is used several times in my application). All I need is to know which way is best, open/close connection on every query (this way, how do I rollback every action?) or the way I have is good enough? –  silentw May 8 at 9:01
1  
Change your database interaction class to implement IDisposable Add a class level boolean 'InDBTransaction'. Set it to true after starting the transaction. Set it to false after committing the transaction. Test in the dispose method and roll back if necessary. –  Mort May 9 at 6:53

2 Answers 2

I read the Text portion of your question and it really sounds like you are trying to do the database's work for it, which is always inefficient, if possible DON'T. Especially when it comes down to rolling back transactions.

in the case where you need to do a bunch of different queries and rollback if something goes wrong, create a stored procedure that calls all these queries.

Use the SQL Database for what it was meant to be used for.

SQL Server is good at querying data and performing operations on data, and it has ways of catching errors and rolling back the entire thing. Go do that, it will be much easier in the long run.

share|improve this answer

I'm trying to find out what is the best way to create a class so I can use it to connect to the SQL Server and be able to rollback everything if a single error exists.

This sounds very much like what a transaction is used for. Looking at the code, you have that part covered... somewhat.

The abstractions are wrong. First thing that hits me is this:

Inherits System.Web.UI.UserControl

Why would data access logic ever need to be entangled with UI controls? What you need is to encapsulate a transaction - a "normal" class can do that, and its implementation can stay off the UI layer's code.

Then there's this:

Private bdConection As Data.SqlClient.sqlConnection
Private bdTransaction As Data.SqlClient.sqlTransaction
Public bdCommand As Data.SqlClient.SqlCommand

The first two would be fine, if the third didn't stick out like a sore thumb: fields should not be Public; exposed fields break encapsulation and open up your code to all kinds of unnecessary issues and edge cases (e.g. what happens when the bdCommand reference is reassigned by external code, in the middle of a transaction?).

One problem, is that SqlConnection, SqlTransaction and SqlCommand types all implement the IDisposable interface, and as such should be as short-lived as possible, and properly disposed: setting a disposable reference to NOTHING does not dispose it. Your class owns these instances, and should be responsible for their disposal... which should happen in the Dispose() method, which doesn't seem to be overridden.

Now, if you went and properly disposed the disposables there, you would run into a bunch of new problems, such as ObjectDisposedException getting thrown all over the place. The reason for that is that you're reusing objects that shouldn't be.

The correct abstraction for encapsulating a transaction, is a unit of work - a simple one could look like this:

Public Interface IUnitOfWork
    Sub Commit()
    Sub Execute(command As SqlCommand)
    Function Select(command As SqlCommand) As DataTable
End Interface

You don't need a Rollback() method there, because if you dispose the unit of work before you commit, you're effectively cancelling all pending changes. I added an Execute method here taking an SqlCommand object, because I don't want to get into repositories, so the idea is to expose a method that can run a command within a transaction's scope.

Public Class UnitOfWork Implements IUnitOfWork, IDisposable

    Private ReadOnly connection As SqlConnection
    Private ReadOnly transaction As SqlTransaction

    Public Sub New(connection As SqlConnection)
        '' assume connection owner already opened the connection (document that!)
        Me.connection = connection
        Me.transaction = Me.connection.BeginTransaction(IsolationLevel.ReadCommitted)
    End Sub

    Public Sub Execute(command As SqlCommand)
        '' assume command owner already set up parameters and cmd text (document that!)
        command.Transaction = Me.Transaction
        command.Connection = Me.connection
        command.ExecuteNonQuery()
    End Sub

    Public Function Select(command As SqlCommand) As DataTable
        '' assume command owner already set up parameters and cmd text (document that!)
        command.Transaction = Me.Transaction
        command.Connection = Me.connection
        Using reader = command.ExecuteReader()
            Dim result As DataTable = New DataTable
            result.Load(reader)
            Return result
        End Using
    End Function

    Public Sub Commit()
        Me.Transaction.Commit()
    End Sub

    Public Sub Dispose()
        Me.Transaction.Dispose()
    End Sub

End Class

You would use it something like this:

Private Sub DoSomething(connection As SqlConnection)
    Using uow = New UnitOfWork(connection)
        Try
            uow.Execute(GetCommandForFoo())
            uow.Execute(GetCommandForBar())
            uow.Commit()
        Catch exception As SqlException
            '' message? log? whatever...
        End Try
    End Using
End Sub

If you need a new transaction, you need a new UnitOfWork instance. But if you're doing things right, you shouldn't need to Commit more than once per request anyway.


Other Points

  • Naming is not following the typical VB.NET conventions. Use PascalCase for object members (methods, etc.), and avoid cryptic prefixes (what's that meaningless s I'm seeing everywhere?)
  • I would not have sBDNonQuery return a Boolean. If that command failed, something is wrong and you want to rollback anyway: just let the exception bubble up instead.
  • Try : bdTransaction.Commit() : Catch : End Try is very, very, very bad: if the transaction can't be committed, you need to know - and at least log something about it. By swallowing any exception that might occur there, you're setting yourself up for some nightmareish debugging later.
  • 4 try/catch blocks in a single method is a code smell all by itself.
  • Where's the last r in erro and sErro?
  • rs is what we used to name our ADODB.Recordset object instances in VB6/VBA. This is .NET, and it's a DataTable. Name accordingly.
  • If you're targeting .NET > 2.0, I would strongly recommend looking into newer data access technologies, which abstract away all the SQL-handling and SqlCommand boilerplate. Start with LINQ-to-SQL, then see what Entity Framework can do for you.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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