A Dispose Gone Bad

I'm going through some VB.NET source code and I keep seeing a common bad occurrence.  Below is a slightly modified example of the code I keep seeing.

Code Snippet

119.  
120. Dim SqlDataAdapter As New SqlClient.SqlDataAdapter
121. Dim sqlCommand As New SqlClient.SqlCommand
122. Dim dt_dataadapter As New DataSet
123.  
124. 'Set Parameter   
125. With sqlCommand
126.     .Connection = dataConnection
127.     .CommandType = CommandType.StoredProcedure
128.     .CommandText = "blah blah"
129.     .Parameters.Add("@ID", SqlDbType.Int, 8, "ID").Value = 0
130. End With
131. SqlDataAdapter.SelectCommand = sqlCommand
132. SqlDataAdapter.Fill(dt_dataadapter)
133.  
134. If dt_dataadapter Is Nothing Then
135.     dt_dataadapter.Dispose()
136.     SqlDataAdapter.Dispose()
137.     sqlCommand.Dispose()
138.     MessageBox.Show("Failed retrieving 'blah blah'.")
139.     Exit Sub
140. End If
141. If dt_dataadapter.Tables.Count = 0 Then
142.     dt_dataadapter.Dispose()
143.     SqlDataAdapter.Dispose()
144.     sqlCommand.Dispose()
145.     MessageBox.Show("Failed retrieving 'blah blah'.")
146.     Exit Sub
147. End If
148. If dt_dataadapter.Tables(0).Rows.Count = 0 Then
149.     dt_dataadapter.Dispose()
150.     SqlDataAdapter.Dispose()
151.     sqlCommand.Dispose()
152.     MessageBox.Show("Failed retrieving 'blah blah'.")
153.     Exit Sub
154. End If

This same format is being used in multiple locations.  There's probably a lot your seeing wrong with this right now, but if not then that's why you should read the rest of this article.  Let me go through this step by step. 

The developer is using objects that use the interface IDispose.  The second thing you'll notice is there is not one try-catch in this entire section.  If the connection mysteriously disappears the program will terminate immediately.  The end user will have no idea what's going on.  Crash!

Now let's say it gets past that.  Maybe he/she finally put a try-catch around the Fill() function.  The dataset has an exception, but the catch just says a message and it continues on.  If the dt_dataadapter is nothing it then goes and disposes of the dt_dataadapter.  You can't dispose nothing.  So now this results in the application crashing.

Let's choose one more scenario.  Let's say this function is called and is surrounded in a Try-Catch.  Now the application doesn't crash, however the data adapter and the command is never released from memory.  Now you have a memory crisis on your hands.  If this happens enough then the application will crash.

The appropriate and best way to avoid everything that has happened above is by using the Using statement.  The example below is about 1,000 times better, easier to read, and this won't cause any memory leakage.

Code Snippet

158.  
159. Using Command As New SqlClient.SqlCommand("blah, blah", dataConnection)
160.     Command.Parameters.AddWithValue("@ID", 0)
161.  
162.     Using Adapter As New SqlClient.SqlDataAdapter(Command)
163.         Using Data As New DataSet
164.             Try
165.                 Adapter.Fill(Data)
166.             Catch ex As Exception
167.                 MessageBox.Show("Failed retrieving 'blah blah'.")
168.                 Exit Sub
169.             End Try
170.  
171.             If Not Data Is Nothing AndAlso Data.Tables.Count > 0 AndAlso Data.Tables(0).Rows.Count > 0 Then
172.                 ' Do something with the data here.
173.             Else
174.                 MessageBox.Show("Failed retrieving 'blah blah'.")
175.                 Exit Sub
176.             End If
177.         End Using
178.     End Using
179. End Using

This works because whenever this method is completed it will automatically call the dispose for each object that uses the Using statement.

If you see anything wrong, or have any questions, please leave a message.