The Importance of Closing SQLConnection Objects When Using a SQLDataReader Object
My present agreement is for a large e-commerce company. Their codebase, which has its origins going back to .Net 1.0, took me by surprise to contain a lot of issues that increase odor levels over the last shit I took.
Regardless, and trying to dispel my level of distraction from it, I keep on having fun trying to add in features to either fix other issues or expand on more crap. When I touch on DAL / BLL, the time it takes to fix the above is done. However, I wanted to get a vote of confidence from the experts so that I could get some confidence that we weren't wasting time so customers didn't lose confidence by voting by touching "stuff that works." Of course, unit testing will solve, or at least mitigate, this concern. Perhaps this should also be added to wtf.com?
Public Function GetSizeInfoBySite(ByVal siteID As String) As IList
Dim strSQL As String = "YES INLINE SQL!! :)"
Dim ci As CrapInfo
Dim alAnArrayList As ArrayList
Dim cn As New SqlConnection(ConfigurationSettings.AppSettings("ConnectionString"))
Dim cmd As New SqlCommand(strSQL, cn)
cmd.Parameters.Add(New SqlParameter("@MySiteID", SqlDbType.NVarChar, 2)).Value = siteID
cn.Open()
Dim rs As SqlDataReader = cmd.ExecuteReader(CommandBehavior.CloseConnection)
While rs.Read()
ci = New CategoryInfo(rs("someID"), rs("someName"))
If IsNothing(alAnArrayList) Then
alAnArrayList = New ArrayList
End If
alAnArrayList.Add(ci)
End While
rs.Close()
Return CType(alAnArrayList, IList)
End Function
Does anyone see a problem with this other than the embedded SQL that makes my gut hideous? At least you don't usually wrap this in try / catch / finally that most of us know about since .Net v1.0? Even better, would it be impractical to correct with assertions? Does the SQLDataReader automatically close the actual encapsulation of the connection?
source to share
Nothing wrong with inline sql as long as the user input is parameterized correctly and it looks like it is.
Other than that, yes, you need to close the connections. On a busy website, you can hit your limit and it will cause all kinds of weirdness.
I also noticed that it is still using an arraylist. Since they migrated from .Net 1.0, it's time to update them to generic List<T>
(and avoid calling CType - you should be able to DirectCast ()).
source to share
Definitely get some instructions related to Connection and Reader objects. If there is an exception, they won't be closed until the garbage collector gets close to them.
I usually don't call .Close () when operators are used. Even if the SqlDataReader closes the connection on dispose (check the doco), using a usage around Connection cannot break and stick to the pattern.
If you do this, try / finally is only needed if you need to do exception handling right there. I tend to leave exception handling at higher levels (wrap every UI entry point, library entry points, additional information in an exception), since the stacktrace is usually sufficient for debugging errors.
It doesn't matter that it matters, but if you are refactoring, move the collection initialization outside of the loop. Second, the code returns null if there are no records.
At least SqlParameters are used! Get rid of anything that combines user input with SQL if you find it (SQL Injection attack), no matter how well it is "cleaned up".
source to share
The connection is closed when the reader is closed because it is using the CloseConnection command.
Dim rs As SqlDataReader = cmd.ExecuteReader(CommandBehavior.CloseConnection)
According to MSDN ( http://msdn.microsoft.com/en-us/library/aa326246(VS.71).aspx )
If the SqlDataReader is created with the CommandBehavior set to CloseConnection, closing the SqlDataReader automatically closes the connection.
source to share
In response to some of the great points noted by Joel and Robert, I refactored the method as follows, which performed flawlessly.
Public Function GetSomeInfoByBusObject(ByVal SomeID As String) As IList
Dim strSQL As String = "InLine SQL"
Dim ci As BusObject
Dim list As New GenList(Of BusObject)
Dim cn As New SqlConnection(
ConfigurationSettings.AppSettings("ConnectionString"))
Using cn
Dim cmd As New SqlCommand(strSQL, cn)
Using cmd
cmd.Parameters.Add(New SqlParameter
("@SomeID", SqlDbType.NVarChar, 2)).Value = strSiteID
cn.Open()
Dim result As SqlDataReader = cmd.ExecuteReader(CommandBehavior.CloseConnection)
While result.Read()
ci = New BusObject(rs("id), result("description"))
list.Add(DirectCast(ci, BusObject))
End While
result.Close()
End Using
Return list
End Using
End function
Created a nice little helper class to wrap general details up
Public Class GenList(Of T)
Inherits CollectionBase
Public Function Add(ByVal value As T) As Integer
Return List.Add(value)
End Function
Public Sub Remove(ByVal value As T)
List.Remove(value)
End Sub
Public ReadOnly Property Item(ByVal index As Integer) As T
Get
Return CType(List.Item(index), T)
End Get
End Property
End Class
source to share
Public Function GetSizeInfoBySite(ByVal siteID As String) As IList(Of CategoryInfo)
Dim strSQL As String = "YES INLINE SQL!! :)"
'reference the 2.0 System.Configuration, and add a connection string section to web.config
' <connectionStrings>
' <add name="somename" connectionString="someconnectionstring" />
' </connectionStrings >
Using cn As New SqlConnection(System.Configuration.ConfigurationManager.ConnectionStrings("somename").ConnectionString
Using cmd As New SqlCommand(strSQL, cn)
cmd.Parameters.Add(New SqlParameter("@MySiteID", SqlDbType.NVarChar, 2)).Value = siteID
cn.Open()
Using reader As IDataReader = cmd.ExecuteReader()
Dim records As IList(Of CategoryInfo) = New List(Of CategoryInfo)
'get ordinal col indexes
Dim ordinal_SomeId As Integer = reader.GetOrdinal("someID")
Dim ordinal_SomeName As Integer = reader.GetOrdinal("someName")
While reader.Read()
Dim ci As CategoryInfo = New CategoryInfo(reader.GetInt32(ordinal_SomeId), reader.GetString(ordinal_SomeName))
records.Add(ci)
End While
Return records
End Using
End Using
End Using
End Function
You can try something like above, using statements will handle closing the connection and deleting objects. This is available when the class implements IDisposable. Also create and return your IList from CategoryInfo.
source to share