:'-(
-
I'm fairly certain the person that made this code did not consider such things. As an example:
Dim oRs As New DataSet
' ...not so many lines later...
oRs = New DataSetAn instance is created, then shortly after another instance is created. And I really don't like when the parens aren't put after calling a constructor. :|
Whaddyaknow Thats a straight copy from MSDN
-
I'm fairly certain the person that made this code did not consider such things. As an example:
Dim oRs As New DataSet
' ...not so many lines later...
oRs = New DataSetAn instance is created, then shortly after another instance is created. And I really don't like when the parens aren't put after calling a constructor. :|
-
The following code is copy/pasted verbatim from a live production environment (the entire project/solution is on the production file system, side-by-side with the executing code):
Private Function GrabDataSet(ByVal sSQL As String) As DataSet
Try
Dim oDAdapt As SqlClient.SqlDataAdapter
Dim oRs As New DataSet
Dim cmdSQL As String = sSQL
'oConn.Open()
oRs = New DataSet
oDAdapt = New SqlClient.SqlDataAdapter(cmdSQL, oConn)
oDAdapt.Fill(oRs)
Return oRs
oRs.Dispose()
oDAdapt.Dispose()
'oConn.Close()
Catch ex As Exception
'MsgBox(ex.Message, MsgBoxStyle.OKOnly, "Database Error")
End Try
End FunctionFound this in a web service I'm upgrading. It's hard to find anything NOT wrong with this. :((
I am trying to learn better development practices so I would gratified if somebody could confirm what I see as well as show me the ideal replacement for this code. Issues with the code 1) Sending in a SQL statement seems to be invitation for SQL injection 2) Exception handling seems to have been done exclusively so that the end user never sees a problem and nothing else 3) The Dispose statements seem to be useless . Now is it good practice to open and close database connections , I have been reading elsewhere about using the Singleton pattern for database connections which in some cases do leave the connection open. Why is it essential to use dispose for the objects that were declared inside the function ? We know that they will be returned to the GC as soon as the function exits.
-
I am trying to learn better development practices so I would gratified if somebody could confirm what I see as well as show me the ideal replacement for this code. Issues with the code 1) Sending in a SQL statement seems to be invitation for SQL injection 2) Exception handling seems to have been done exclusively so that the end user never sees a problem and nothing else 3) The Dispose statements seem to be useless . Now is it good practice to open and close database connections , I have been reading elsewhere about using the Singleton pattern for database connections which in some cases do leave the connection open. Why is it essential to use dispose for the objects that were declared inside the function ? We know that they will be returned to the GC as soon as the function exits.
Regarding the issues, there are a few more, like initializing a variable only to re-initialize a few lines later, and others. In .NET especially, there is connection pooling for database connections, meaning that even after you close a connection, the connection is kept alive by the framework, and next time you open the connection, the existing connection will be reused. No comment on the Singleton pattern. It is considered a best practice to always call the
Disposemethod on a Class that implements IDisposable. This is why there is ausing (resource) { }
construct built into the language. Classes usually implement
IDisposableto indicate that they are using external resources which need to be released once you are finished with the class. In .NET, once a method has completed (returned), any instances created that have no other references can be collected by the Garbage Collector (GC). However, because the GC decides to clean the memory in its own time, this can sometimes mean that instances remain alive for longer than necessary. For example, on a computer with a lot of RAM, the GC may not run for a long time, as there is no issue with Memory. Another issue with relying on the GC is that the GC will not call theDisposemethod on an instance, as it doesn't know anything aboutIDisposable. What it does is call theFinalizemethod (known as the Finalizer), which all classes inherit fromObject. However, the way in which GC calls the Finalizer means that the instance has to be kept alive for longer than absolutely necessary, at minimum until the next GC collection. Additionally, not all classes necessarily implement a Finalizer. If a class used external resources, such as a file or a database connection and doesn't release the resource, the external resources may remain inaccessible even after the .NET application closes. -
The following code is copy/pasted verbatim from a live production environment (the entire project/solution is on the production file system, side-by-side with the executing code):
Private Function GrabDataSet(ByVal sSQL As String) As DataSet
Try
Dim oDAdapt As SqlClient.SqlDataAdapter
Dim oRs As New DataSet
Dim cmdSQL As String = sSQL
'oConn.Open()
oRs = New DataSet
oDAdapt = New SqlClient.SqlDataAdapter(cmdSQL, oConn)
oDAdapt.Fill(oRs)
Return oRs
oRs.Dispose()
oDAdapt.Dispose()
'oConn.Close()
Catch ex As Exception
'MsgBox(ex.Message, MsgBoxStyle.OKOnly, "Database Error")
End Try
End FunctionFound this in a web service I'm upgrading. It's hard to find anything NOT wrong with this. :((
-
I am trying to learn better development practices so I would gratified if somebody could confirm what I see as well as show me the ideal replacement for this code. Issues with the code 1) Sending in a SQL statement seems to be invitation for SQL injection 2) Exception handling seems to have been done exclusively so that the end user never sees a problem and nothing else 3) The Dispose statements seem to be useless . Now is it good practice to open and close database connections , I have been reading elsewhere about using the Singleton pattern for database connections which in some cases do leave the connection open. Why is it essential to use dispose for the objects that were declared inside the function ? We know that they will be returned to the GC as soon as the function exits.
-
The following code is copy/pasted verbatim from a live production environment (the entire project/solution is on the production file system, side-by-side with the executing code):
Private Function GrabDataSet(ByVal sSQL As String) As DataSet
Try
Dim oDAdapt As SqlClient.SqlDataAdapter
Dim oRs As New DataSet
Dim cmdSQL As String = sSQL
'oConn.Open()
oRs = New DataSet
oDAdapt = New SqlClient.SqlDataAdapter(cmdSQL, oConn)
oDAdapt.Fill(oRs)
Return oRs
oRs.Dispose()
oDAdapt.Dispose()
'oConn.Close()
Catch ex As Exception
'MsgBox(ex.Message, MsgBoxStyle.OKOnly, "Database Error")
End Try
End FunctionFound this in a web service I'm upgrading. It's hard to find anything NOT wrong with this. :((
Maybe the Republicans are correct when they say "Outsourcing is good for the economy." It keeps all of us employed rewriting all that crap code!
A guide to posting questions on CodeProject[^]
Dave Kreskowiak -
Regarding the issues, there are a few more, like initializing a variable only to re-initialize a few lines later, and others. In .NET especially, there is connection pooling for database connections, meaning that even after you close a connection, the connection is kept alive by the framework, and next time you open the connection, the existing connection will be reused. No comment on the Singleton pattern. It is considered a best practice to always call the
Disposemethod on a Class that implements IDisposable. This is why there is ausing (resource) { }
construct built into the language. Classes usually implement
IDisposableto indicate that they are using external resources which need to be released once you are finished with the class. In .NET, once a method has completed (returned), any instances created that have no other references can be collected by the Garbage Collector (GC). However, because the GC decides to clean the memory in its own time, this can sometimes mean that instances remain alive for longer than necessary. For example, on a computer with a lot of RAM, the GC may not run for a long time, as there is no issue with Memory. Another issue with relying on the GC is that the GC will not call theDisposemethod on an instance, as it doesn't know anything aboutIDisposable. What it does is call theFinalizemethod (known as the Finalizer), which all classes inherit fromObject. However, the way in which GC calls the Finalizer means that the instance has to be kept alive for longer than absolutely necessary, at minimum until the next GC collection. Additionally, not all classes necessarily implement a Finalizer. If a class used external resources, such as a file or a database connection and doesn't release the resource, the external resources may remain inaccessible even after the .NET application closes.Thank you for your comments. I had no idea that GC uses some form of Lazy scheduling. Any pointers on documentation that would guide me towards the accepted standards for .Net programming?
-
The following code is copy/pasted verbatim from a live production environment (the entire project/solution is on the production file system, side-by-side with the executing code):
Private Function GrabDataSet(ByVal sSQL As String) As DataSet
Try
Dim oDAdapt As SqlClient.SqlDataAdapter
Dim oRs As New DataSet
Dim cmdSQL As String = sSQL
'oConn.Open()
oRs = New DataSet
oDAdapt = New SqlClient.SqlDataAdapter(cmdSQL, oConn)
oDAdapt.Fill(oRs)
Return oRs
oRs.Dispose()
oDAdapt.Dispose()
'oConn.Close()
Catch ex As Exception
'MsgBox(ex.Message, MsgBoxStyle.OKOnly, "Database Error")
End Try
End FunctionFound this in a web service I'm upgrading. It's hard to find anything NOT wrong with this. :((
-
Just tested it. VB doesn't remove them but it doesn't add them neither so if you type them yourself they'll stay there. :)
VB only requires parens when it's necessary to add parameters. Otherwise, you can slip by without them.
XAlan Burkhart
-
Regarding the issues, there are a few more, like initializing a variable only to re-initialize a few lines later, and others. In .NET especially, there is connection pooling for database connections, meaning that even after you close a connection, the connection is kept alive by the framework, and next time you open the connection, the existing connection will be reused. No comment on the Singleton pattern. It is considered a best practice to always call the
Disposemethod on a Class that implements IDisposable. This is why there is ausing (resource) { }
construct built into the language. Classes usually implement
IDisposableto indicate that they are using external resources which need to be released once you are finished with the class. In .NET, once a method has completed (returned), any instances created that have no other references can be collected by the Garbage Collector (GC). However, because the GC decides to clean the memory in its own time, this can sometimes mean that instances remain alive for longer than necessary. For example, on a computer with a lot of RAM, the GC may not run for a long time, as there is no issue with Memory. Another issue with relying on the GC is that the GC will not call theDisposemethod on an instance, as it doesn't know anything aboutIDisposable. What it does is call theFinalizemethod (known as the Finalizer), which all classes inherit fromObject. However, the way in which GC calls the Finalizer means that the instance has to be kept alive for longer than absolutely necessary, at minimum until the next GC collection. Additionally, not all classes necessarily implement a Finalizer. If a class used external resources, such as a file or a database connection and doesn't release the resource, the external resources may remain inaccessible even after the .NET application closes.Generally an excellent post, however this:
Schmuli wrote:
If a class used external resources, such as a file or a database connection and doesn't release the resource, the external resources may remain inaccessible even after the .NET application closes.
... isn't true on a decent operating system (and Windows qualifies these days), because when the process ends all its resource ties will be killed by the OS. But yes, relying on the GC to clean up after you is a bad plan, because there's no guarantee that the GC will run immediately, or even ever.
-
Generally an excellent post, however this:
Schmuli wrote:
If a class used external resources, such as a file or a database connection and doesn't release the resource, the external resources may remain inaccessible even after the .NET application closes.
... isn't true on a decent operating system (and Windows qualifies these days), because when the process ends all its resource ties will be killed by the OS. But yes, relying on the GC to clean up after you is a bad plan, because there's no guarantee that the GC will run immediately, or even ever.
BobJanova wrote:
... isn't true on a decent operating system (and Windows qualifies these days), because when the process ends all its resource ties will be killed by the OS.
Actually, he was right in certain cases. The example I'm thinking of is an Oracle connection. If the connection isn't disposed, even if the underlying application dies, the connection may be left open. This was a real pain point for us on a project that we had to interface with. It took me and the client DBA ganging up on the project lead before he ensured the connections were closed. All of a sudden, 300-400 active connections were reduced to less than a handful.
*pre-emptive celebratory nipple tassle jiggle* - Sean Ewington
"Mind bleach! Send me mind bleach!" - Nagy Vilmos
CodeStash - Online Snippet Management | My blog | MoXAML PowerToys | Mole 2010 - debugging made easier
-
Regarding the issues, there are a few more, like initializing a variable only to re-initialize a few lines later, and others. In .NET especially, there is connection pooling for database connections, meaning that even after you close a connection, the connection is kept alive by the framework, and next time you open the connection, the existing connection will be reused. No comment on the Singleton pattern. It is considered a best practice to always call the
Disposemethod on a Class that implements IDisposable. This is why there is ausing (resource) { }
construct built into the language. Classes usually implement
IDisposableto indicate that they are using external resources which need to be released once you are finished with the class. In .NET, once a method has completed (returned), any instances created that have no other references can be collected by the Garbage Collector (GC). However, because the GC decides to clean the memory in its own time, this can sometimes mean that instances remain alive for longer than necessary. For example, on a computer with a lot of RAM, the GC may not run for a long time, as there is no issue with Memory. Another issue with relying on the GC is that the GC will not call theDisposemethod on an instance, as it doesn't know anything aboutIDisposable. What it does is call theFinalizemethod (known as the Finalizer), which all classes inherit fromObject. However, the way in which GC calls the Finalizer means that the instance has to be kept alive for longer than absolutely necessary, at minimum until the next GC collection. Additionally, not all classes necessarily implement a Finalizer. If a class used external resources, such as a file or a database connection and doesn't release the resource, the external resources may remain inaccessible even after the .NET application closes.Relying on the GC to clean up after classes that implements IDisposable may be dangerous - in some cases such a class may never be garbage collected and thus in practice lead to memory leaks. Consider a class that e.g., when instantiated, subscribes to an event from .NET's SystemEvents class (a GUI application might e.g. subscribe to UserPreferenceChanged). Because these events are static, the delegate have a reference to the class that lasts for the entire lifetime of the application, i.e. it will prevent the class from being GC'ed until it unsubscribes from the event. I.e. the user of the class will have to tell it when it no longer needs to subscribe to such an event - the obvious way to do that is by calling Dispose. Now, you could argue that not all classes that implements Dispose suffer from this, but how would you know (and know that a future version of the class wouldn't require it either)? And should you know - is it not the purpose of encapsulation to ensure that you don't have to know about implementation details like this? Due to this, in my opinion is should not only be best practice, but required that you call Dispose on a class that implements IDisposable.
-
BobJanova wrote:
... isn't true on a decent operating system (and Windows qualifies these days), because when the process ends all its resource ties will be killed by the OS.
Actually, he was right in certain cases. The example I'm thinking of is an Oracle connection. If the connection isn't disposed, even if the underlying application dies, the connection may be left open. This was a real pain point for us on a project that we had to interface with. It took me and the client DBA ganging up on the project lead before he ensured the connections were closed. All of a sudden, 300-400 active connections were reduced to less than a handful.
*pre-emptive celebratory nipple tassle jiggle* - Sean Ewington
"Mind bleach! Send me mind bleach!" - Nagy Vilmos
CodeStash - Online Snippet Management | My blog | MoXAML PowerToys | Mole 2010 - debugging made easier
Are you sure the process was being killed? This sounds like it might have been a web app and IIS would have been the process in that case, so you'd be relying on the GC to come along and help you. File handles, open sockets and properly registered graphical resources are definitely released when a process ends under Windows. Obviously, it's still good practice to close your resources manually because you don't want to make the user close your application to free them!
-
Are you sure the process was being killed? This sounds like it might have been a web app and IIS would have been the process in that case, so you'd be relying on the GC to come along and help you. File handles, open sockets and properly registered graphical resources are definitely released when a process ends under Windows. Obviously, it's still good practice to close your resources manually because you don't want to make the user close your application to free them!
BobJanova wrote:
Obviously, it's still good practice to close your resources manually because you don't want to make the user close your application to free them!
It is, it was and always will be. Unless you are a member of the reformed Java religion, who do not believe in managing memory and resources. :)
At least artificial intelligence already is superior to natural stupidity
-
The following code is copy/pasted verbatim from a live production environment (the entire project/solution is on the production file system, side-by-side with the executing code):
Private Function GrabDataSet(ByVal sSQL As String) As DataSet
Try
Dim oDAdapt As SqlClient.SqlDataAdapter
Dim oRs As New DataSet
Dim cmdSQL As String = sSQL
'oConn.Open()
oRs = New DataSet
oDAdapt = New SqlClient.SqlDataAdapter(cmdSQL, oConn)
oDAdapt.Fill(oRs)
Return oRs
oRs.Dispose()
oDAdapt.Dispose()
'oConn.Close()
Catch ex As Exception
'MsgBox(ex.Message, MsgBoxStyle.OKOnly, "Database Error")
End Try
End FunctionFound this in a web service I'm upgrading. It's hard to find anything NOT wrong with this. :((
-
Someone may have already pointed this out, I've not read all the replies yet, but the first thing I noticed is that the lines that dispose the dataset and dataadapter will never execute.
Indeed. That is one of the many things wrong with this code.
-
Thank you for your comments. I had no idea that GC uses some form of Lazy scheduling. Any pointers on documentation that would guide me towards the accepted standards for .Net programming?
Accepted standards for programming should theoretically be the same for most languages, such as using recognized Design Patterns. This can depend on the type of environment you are working in, which can be desktop, web, mobile, cloud, etc. There are plenty of blogs and sites out there that discuss these issues. For more information on .NET Garbage Collection and IDisposable/Finalizers, you can check out the following links: Garbage Collection[^] - Covers Garbage Collection in .NET, with details about the how it works. Cleaning Up Unmanaged Resources[^] - Discusses Disposing and Finalizing in .NET.
-
The following code is copy/pasted verbatim from a live production environment (the entire project/solution is on the production file system, side-by-side with the executing code):
Private Function GrabDataSet(ByVal sSQL As String) As DataSet
Try
Dim oDAdapt As SqlClient.SqlDataAdapter
Dim oRs As New DataSet
Dim cmdSQL As String = sSQL
'oConn.Open()
oRs = New DataSet
oDAdapt = New SqlClient.SqlDataAdapter(cmdSQL, oConn)
oDAdapt.Fill(oRs)
Return oRs
oRs.Dispose()
oDAdapt.Dispose()
'oConn.Close()
Catch ex As Exception
'MsgBox(ex.Message, MsgBoxStyle.OKOnly, "Database Error")
End Try
End FunctionFound this in a web service I'm upgrading. It's hard to find anything NOT wrong with this. :((
-
Accepted standards for programming should theoretically be the same for most languages, such as using recognized Design Patterns. This can depend on the type of environment you are working in, which can be desktop, web, mobile, cloud, etc. There are plenty of blogs and sites out there that discuss these issues. For more information on .NET Garbage Collection and IDisposable/Finalizers, you can check out the following links: Garbage Collection[^] - Covers Garbage Collection in .NET, with details about the how it works. Cleaning Up Unmanaged Resources[^] - Discusses Disposing and Finalizing in .NET.
Thank you . Unfortunately the web seems to be a rather disparate source of information with more biases than useful information so I did not have much luck .I just finished the Heads First Patterns book and now I got the GoF book and intend to read it soon. Thanks for the info re Garbage collection