Reputation: 708
I am a developer that is maintaining and constantly making an application faster and better. I came across a piece of code that uses .Dispose()
, which is inside a using
statement.
Here is the code:
using (IDbCommand cmd = proxy.Connection.CreateCommand())
{
cmd.CommandText = "usp_GetPluginInfo";
cmd.CommandType = CommandType.StoredProcedure;
cmd.Parameters.Add(proxy.Connection.CreateParameter(cmd, "@dealershipId", dealershipId));
cmd.Parameters.Add(proxy.Connection.CreateParameter(cmd, "@pluginId", pluginId));
PluginInfo[] pluginInfos = PopulatePluginsFromCommand(cmd);
result = pluginInfos.First();
cmd.Dispose();
}
Wouldn't the last }
Dispose when the Command is finished? I didn't think that it was necessary to use Dispose()
in this case.
Thanks for all of your help!
Upvotes: 3
Views: 170
Reputation: 68760
You're right, it's not necessary.
A using
statement is translated by the compiler to this:
IDbCommand cmd = proxy.Connection.CreateCommand();
try
{
//...
}
finally
{
if(cmd != null)
((IDisposable)cmd).Dispose();
}
Having said that, calling Dispose
more than once should not be "hurtful" either, as a proper IDisposable implementation is supposed to be idempotent. It is, however, redundant, and you should remove it, since you're cleaning up the code.
Upvotes: 6
Reputation: 3
The cmd.Dispose(); is unnecessary. In a case where Dispose() is not properly implemented or made to throw the System.ObjectDisposedException, this could be an issue
Refer to the code analysis rule below The call to CA2202: Do not dispose objects multiple times
Upvotes: 0
Reputation: 116501
It's redundant and the explicit call to Dispose
is not guaranteed to execute. If you want to call Dispose
explicitly make sure to do it in a finally block.
Upvotes: 0
Reputation: 22814
There is one reason he could've put it there explicitly. If the IDbCommand
isn't the one from System.Data
, and the interface that was made for some reason inherited from IDisposable
but used new
instead of override
, they could have different behaviours. But it's 99.999999999% of the time absolutely redundant.
Upvotes: 0
Reputation: 20157
You are correct! The using
block is syntactic sugar for the following:
IDbCommand cmd = proxy.Connection.CreateCommand();
try
{
cmd.CommandType = CommandType.StoredProcedure;
cmd.Parameters.Add(proxy.Connection.CreateParameter(cmd, "@dealershipId", dealershipId));
cmd.Parameters.Add(proxy.Connection.CreateParameter(cmd, "@pluginId", pluginId));
PluginInfo[] pluginInfos = PopulatePluginsFromCommand(cmd);
result = pluginInfos.First();}
finally
{
if(cmd != null)
{
((IDisposable)cmd).Dispose();
}
}
So yes, the extra cmd.Dispose();
is unnecessary. In most cases, a well-written class will do nothing the second .Dispose()
(using the so-called Disposable Pattern), but sometimes it will cause issues. Get rid of it!
Upvotes: 1
Reputation: 22511
using calls Dispose upon exit of the block, no matter whether the block is run successfully or an exception occurs. In your example, the explicit call to Dispose is redundant.
Upvotes: 0
Reputation: 1430
using
statement always calls IDisposable.Dispose()
method. cmb.Dispose()
code is redundant
Upvotes: 0