open a db connection in a constructor ?

[Author]

a .net 1.1 app has a class whose constructor opens a db connection to
sql svr two thousand. this class has more than a dozen of methods.
most of them don't do db stuff.

I am wondering if this design is going to be a problem, bcoz each time
this class is instantiated, a db conn is open. The worst thing is that
I haven't seen anywhere in the code the db conn is closed.

I write about this bcoz I see that this app leaves more than one
hundred open connections in the db, and the app complains that the
conn pool is full.

I think first of all, each connection needs to be closed. secondly,
I don't think it is a good idea to open a db conn in the constructor
of a class, unless each and every method of the class needs to visit
the db. I would like to hear you OO gurus' opinion. thank you.

-- typed up from my cell phone.

 

[Jon Skeet [C# MVP]]

I think first of all, each connection needs to be closed. secondly,
I don't think it is a good idea to open a db conn in the constructor
of a class, unless each and every method of the class needs to visit
the db. I would like to hear you OO gurus' opinion. thank you.

Sounds about right. I almost never end up with a db connection as a
field - it's alost always always a local variable, introduced in a
"using" statement (and therefore automatically closed). If it *is* in
a field, the type implements IDisposable and disposes the connection
when the object itself is disposed.

Jon

 

[sloan]

Looks like another example of a developer being too clever for their own
good.

never closing a db connection = HORRIBLE.

opening a db connection in a construction , my strong personal opinion is
that this is not a good practice, even if you close them.

my advice (which is pretty typical I believe)
OPEN LATE
USE Quickly
Close SOON

...........

You can check this 1.1 example for the idea of a Controller/Manager class,
which seperates calls to the DAL from the business objects.
http://sholliday.spaces.live.com/Blog/cns!A68482B9628A842A!139.entry


But outside of the 1 other miscue (which is not closing your IDataReader's
btw), you've listed 2 of the 3 worst ways to code up db calls IMHO.

...
"app complains that the conn pool is full"
Yep. That is exactly the "feature" the previous developer has coded for
you.

 

[Author]

Sounds about right. I almost never end up with a db connection as a
field - it's alost always always a local variable, introduced in a
"using" statement (and therefore automatically closed). If it *is* in
a field, the type implements IDisposable and disposes the connection
when the object itself is disposed.

Jon

Thank you very much. Yes, the db connection object was/is defined as
a private field of the class. Now, regarding your last statement

<quote>
If it *is* in a field, the type implements IDisposable and disposes
the connection when the object itself is disposed.
</quote>

How is this different from not implementing IDisposable but do
explicitly dispose/close the connection?

BTW, below is the constructor of the class (written in VB) I've been
talking about.

... ...
Private con As SqlConnection

Public Sub New()
Me.cntString = ConfigurationSettings.AppSettings.Item("cntString")
Me.con = New SqlConnection
Me.con.ConnectionString = Me.cntString
Me.cmd = New SqlCommand
Me.cmd.Connection = Me.con
Me.con.Open // Look, the db connection is open here.
End Sub
......

 

[Author]

Looks like another example of a developer being too clever for their own
good.

never closing a db connection = HORRIBLE.

opening a db connection in a construction , my strong personal opinion is
that this is not a good practice, even if you close them.

my advice (which is pretty typical I believe)
OPEN LATE
USE Quickly
Close SOON

..........

You can check this 1.1 example for the idea of a Controller/Manager class,
which seperates calls to the DAL from the business objects.http://sholliday.spaces.live.com/Blog/cns!A68482B9628A842A!139.entry

But outside of the 1 other miscue (which is not closing your IDataReader's
btw), you've listed 2 of the 3 worst ways to code up db calls IMHO.

..
"app complains that the conn pool is full"
Yep.  That is exactly the "feature" the previous developer has coded for
you.

Thank you for the reference, I'll definitely take a look.

I whole-heartedly agree with your principles.

OPEN LATE
USE Quickly
Close SOON

I would like to add one more to it, but I am not sure if it is
considered a good practice. That is, retrieve as much data as needed
in smallest possible number of db trips.

For example, if I know that I will populate 3 different controls on a
web page with data from 3 different queries, shouldn't I execute these
3 queries in a batch and put the result sets in a DataSet object,
close the connection and then only work with the in-memory dataset?
Or should I make 3 different db trips, each of which only executes 1
query?

 

[Author]

Looks like another example of a developer being too clever for their own
good.

never closing a db connection = HORRIBLE.

opening a db connection in a construction , my strong personal opinion is
that this is not a good practice, even if you close them.

my advice (which is pretty typical I believe)
OPEN LATE
USE Quickly
Close SOON

..........

You can check this 1.1 example for the idea of a Controller/Manager class,
which seperates calls to the DAL from the business objects.http://sholliday.spaces.live.com/Blog/cns!A68482B9628A842A!139.entry

But outside of the 1 other miscue (which is not closing your IDataReader's
btw), you've listed 2 of the 3 worst ways to code up db calls IMHO.

..
"app complains that the conn pool is full"
Yep.  That is exactly the "feature" the previous developer has coded for
you.

I just looked into the compiled dll (because I don't have the source
code of the class) using Reflector.Net, and I did find that the class
overrides the Finalize method of the ultimate ancestor Object, like
so:

Protected Overrides Sub Finalize()
Try
MyBase.Finalize
If Not Information.IsNothing(Me.dataReader) Then
Me.dataReader = Nothing
End If
If Not Information.IsNothing(Me.cmd) Then
Me.cmd = Nothing
End If
If Not Information.IsNothing(Me.con) Then
Me.con.Close
Me.con = Nothing
End If
Catch exception1 As Exception
ProjectData.SetProjectError(exception1)
Dim exception As Exception = exception1
ProjectData.ClearProjectError
End Try
End Sub

So, it does attempts to close the connection in this Finalize method.
However, if the garbage collector isn't quick enough to collect the
instantiated object of this class, the connection won't be closed in a
timely manner, thus making the application shaky.

 

[Jon Skeet [C# MVP]]

Thank you very much.  Yes, the db connection object was/is defined as
a private field of the class.  Now, regarding your last statement

<quote>
If it *is* in a field, the type implements IDisposable and disposes
the connection when the object itself is disposed.
</quote>

How is this different from not implementing IDisposable but do
explicitly dispose/close the connection?

If the database connection is in a field, I'd expect it to stay open
while the object is usable. What's the point of keeping it around if
it's going to be closed? On the other hand, I suppose that's sort of
what LINQ to SQL does... but in that case it shouldn't be opened in
the constructor.

Jon

 

[Ignacio Machin ( .NET/ C# MVP )]

Sounds about right. I almost never end up with a db connection as a
field - it's alost always always a local variable, introduced in a
"using" statement (and therefore automatically closed). If it *is* in
a field, the type implements IDisposable and disposes the connection
when the object itself is disposed.

Jon

I prefer the first option, the connection is declared inside the
method surrounded in a using statement. This assure you that the
conenction will be closed ASAP

 

[Author]

If the database connection is in a field, I'd expect it to stay open
while the object is usable. What's the point of keeping it around if
it's going to be closed? On the other hand, I suppose that's sort of
what LINQ to SQL does... but in that case it shouldn't be opened in
the constructor.

Jon

So, even if we have a class that implements IDisposable, we still need
to close the connection, right?

In other words, it is *not* the case that once a class implements
IDisposable, then the db connection wherein is automatically closed
when the object of the class is disposed. Correct?

 

[Author]

I prefer the first option, the connection is declared inside the
method surrounded in a using statement. This assure you that the
conenction will be closed ASAP

Talking about the using statement block, we normally have a quite a
few disposable objects to cooperate in doing db work. For example,
SqlConnection, SqlCommand, SqlDataAdapter or SqlDataReader, DataSet
etc.

So, is it pretty common to do the following with all 4 using
statements?

using (SqlConnection connection = new
SqlConnection(myConnectionString))
using (SqlCommand command = new SqlCommand(mySqlQuery, connection))
using (SqlDataAdapter dataAdapter = new SqlDataAdapter(command))
using (DataSet dataSet = new DataSet())
{
command.CommandType = CommandType.Text;
connection.Open();
dataAdapter.Fill(dataSet);
}

 

[Jon Skeet [C# MVP]]

So, even if we have a class that implements IDisposable, we still need
to close the connection, right?

In other words, it is *not* the case that once a class implements
IDisposable, then the db connection wherein is automatically closed
when the object of the class is disposed. Correct?

You would make the Dispose method close the connection.

 

[Peter]

You can check this 1.1 example for the idea of a Controller/Manager
class, which seperates calls to the DAL from the business objects.
http://sholliday.spaces.live.com/Blog/cns!A68482B9628A842A!139.entry

Hi - I have looked at the above link, and I have a couple of questions
about his use of IDataReader.

For example, he has a Data-Access-Layer class like this:
public class CustomerData
{
...
public IDataReader CustomersGetAllReader()
{
return
Microsoft.ApplicationBlocks.Data.SqlHelper.ExecuteReader(m_connectionStr
ing, this.PROC_CUSTOMERS_GET_ALL, null);
}
...
}

Is it good practice to return an IDataReader like this?

One thing is, a "data-reader" is a database-centric object isn't it? So
if a business-layer class calls this data-layer class you suddenly have
database specific things being returned up to the business layer.

Also, does an IDataReader hold a connection open to the database?


Thanks,
Peter

 

[sloan]

Well, it is the DataAccessLayer, which talks to a database.

You notice I return an IDataReader, not a specific one. Thus I can swap it
out to a
SqlDataReader
OracleDataReader
MySqlDataReader
OleDBDataReader
and it doesn't affect the business layer code at at all.

See my other blog entry "multiple rdbms with the factory pattern" if you
want to see how to have the DAL support multiple databases in a non hacky
fashion.

Back to your question.
The IDataReader NEEDS TO BE CLOSED AFTER ITS CONSUMED. Thus you'll see my
BAL calling the dataReader.Close() method.
The EnterpriseLibrary and/or the DAAB 2.0 sets up a automatic "close the
connection when the datareader is closed" call for you.
But you need to use/consume the IDataReader at some point. If you close it
in the DAL, it becomes worthless to you in the BAL.
This open IDataReader from the DAL is the only hole that the
EnterpriseLibrary.Data (or DAAB 2.0 ) cannot protect you from. You gotta
handle that yourself.

I think its a good practice. Since I've went almost 100% BusinessObjects,
the IDataReader gives you the best performance.
That code is tweaked to give the best performance. I even use the
"Layout"'s to have hard ordinal constants to avoid a look up penalty like
int i = sqlDataReader["EmpID"]; //
As in I don't incur the penalty of the EmpID lookup. Ordinal numbers
(0,1,2,etc) have been faster for lookup even back in ADO days.
But my method with the Layout....gives you readability as well. I thank an
old colleague for this idea.

You could return back (strong)DataSets from the DAL. There is nothing wrong
about that. But DataSets are slightly less performance abled then an
IDataReader.
Just google "DataReader, DataSet Performance".

.........

I suggest that you CAN return IDataReaders to the BAL. Let the BAL
use/consume them ... as fast as it can...and close them.
I do NOT like the BAL exposing IDataReaders to the PresentationLayer. Why?
Your page developers are more likely to forget to close them.
And it's better to give them the BusinessObjects and BusinessObject
collections to work with I believe.

...

 

[Peter]

Well, it is the DataAccessLayer, which talks to a database.

You notice I return an IDataReader, not a specific one. Thus I can
swap it out to a SqlDataReader
OracleDataReader
MySqlDataReader
OleDBDataReader
and it doesn't affect the business layer code at at all.

See my other blog entry "multiple rdbms with the factory pattern" if
you want to see how to have the DAL support multiple databases in a
non hacky fashion.

Back to your question.
The IDataReader NEEDS TO BE CLOSED AFTER ITS CONSUMED. Thus you'll
see my BAL calling the dataReader.Close() method. The
EnterpriseLibrary and/or the DAAB 2.0 sets up a automatic "close the
connection when the datareader is closed" call for you. But you need
to use/consume the IDataReader at some point. If you close it in the
DAL, it becomes worthless to you in the BAL. This open IDataReader
from the DAL is the only hole that the EnterpriseLibrary.Data (or
DAAB 2.0 ) cannot protect you from. You gotta handle that yourself.

I think its a good practice. Since I've went almost 100%
BusinessObjects, the IDataReader gives you the best performance.
That code is tweaked to give the best performance. I even use the
"Layout"'s to have hard ordinal constants to avoid a look up penalty
like int i = sqlDataReader["EmpID"]; // As in I don't incur the
penalty of the EmpID lookup. Ordinal numbers (0,1,2,etc) have been
faster for lookup even back in ADO days. But my method with the
Layout....gives you readability as well. I thank an old colleague
for this idea.

You could return back (strong)DataSets from the DAL. There is
nothing wrong about that. But DataSets are slightly less performance
abled then an IDataReader. Just google "DataReader, DataSet
Performance".

........

I suggest that you CAN return IDataReaders to the BAL. Let the BAL
use/consume them ... as fast as it can...and close them. I do NOT
like the BAL exposing IDataReaders to the PresentationLayer. Why?
Your page developers are more likely to forget to close them. And
it's better to give them the BusinessObjects and BusinessObject
collections to work with I believe.

Ok. I've always looked at it from the point of view that the Data
Access Layer communicates with the database (or whatever the datasource
is), and it is only in this layer that database specific classes and
interfaces are used. I consider IDataReader and the concrete
implementations you mention to be database specific, and they have
therefore no business showing up in the Business Layer.

I would usually transform the IDataReader into specific business data
classes in the Data Access Layer, and return them up to the Business
Layer.

I can see you do it differently - transforming the database classes to
business data classes in the Business Layer.

I am still not convinced it is a good idea to rely on the Business
Layer to close the connections/readers opened in the Data Access
Layer...


/Peter

 

[sloan]

You can do that (avoid IDataReader in the BAL). But then you need a "glue"
assembly where you put the business objects.

The glue.dll would have
Customer
CustomerCollection
Order
OrderCollection
(etc,etc)

Thus
Presentation references BAL and glue.
BAL references DAL and glue.
DAL references the glue assembly.

Then you DAL can return those objects to the BAL. Since it references the
glue assembly, it know about them and can return them.

If you stick the objects inside of the DAL, then your Pres layer has to
reference Data. (boo).

...

Obviously (and sometimes I do this in very small projects), you could have
everything in one assembly, and pick different namespaces...if you can
follow the rules.
However, with junior developers, they sometimes don't follow the rules, and
thus I like things in assemblies.....so they can't break the rules.

If you follow my blog and go to the "bird's eye view" article at
microsoft.com....the article discusses the "glue" assembly. They call it a
"common assembly" or something like that.

Instead of the glue assembly, I prefer my approach.

However, I would never say the glue method is wrong. In fact, it would be
preferred in some scenarios.

Well, it is the DataAccessLayer, which talks to a database.

You notice I return an IDataReader, not a specific one. Thus I can
swap it out to a SqlDataReader
OracleDataReader
MySqlDataReader
OleDBDataReader
and it doesn't affect the business layer code at at all.

See my other blog entry "multiple rdbms with the factory pattern" if
you want to see how to have the DAL support multiple databases in a
non hacky fashion.

Back to your question.
The IDataReader NEEDS TO BE CLOSED AFTER ITS CONSUMED. Thus you'll
see my BAL calling the dataReader.Close() method. The
EnterpriseLibrary and/or the DAAB 2.0 sets up a automatic "close the
connection when the datareader is closed" call for you. But you need
to use/consume the IDataReader at some point. If you close it in the
DAL, it becomes worthless to you in the BAL. This open IDataReader
from the DAL is the only hole that the EnterpriseLibrary.Data (or
DAAB 2.0 ) cannot protect you from. You gotta handle that yourself.

I think its a good practice. Since I've went almost 100%
BusinessObjects, the IDataReader gives you the best performance.
That code is tweaked to give the best performance. I even use the
"Layout"'s to have hard ordinal constants to avoid a look up penalty
like int i = sqlDataReader["EmpID"]; // As in I don't incur the
penalty of the EmpID lookup. Ordinal numbers (0,1,2,etc) have been
faster for lookup even back in ADO days. But my method with the
Layout....gives you readability as well. I thank an old colleague
for this idea.

You could return back (strong)DataSets from the DAL. There is
nothing wrong about that. But DataSets are slightly less performance
abled then an IDataReader. Just google "DataReader, DataSet
Performance".

........

I suggest that you CAN return IDataReaders to the BAL. Let the BAL
use/consume them ... as fast as it can...and close them. I do NOT
like the BAL exposing IDataReaders to the PresentationLayer. Why?
Your page developers are more likely to forget to close them. And
it's better to give them the BusinessObjects and BusinessObject
collections to work with I believe.

Ok. I've always looked at it from the point of view that the Data
Access Layer communicates with the database (or whatever the datasource
is), and it is only in this layer that database specific classes and
interfaces are used. I consider IDataReader and the concrete
implementations you mention to be database specific, and they have
therefore no business showing up in the Business Layer.

I would usually transform the IDataReader into specific business data
classes in the Data Access Layer, and return them up to the Business
Layer.

I can see you do it differently - transforming the database classes to
business data classes in the Business Layer.

I am still not convinced it is a good idea to rely on the Business
Layer to close the connections/readers opened in the Data Access
Layer...


/Peter

 

[Arne Vajhøj]

a .net 1.1 app has a class whose constructor opens a db connection to
sql svr two thousand. this class has more than a dozen of methods.
most of them don't do db stuff.

I am wondering if this design is going to be a problem, bcoz each time
this class is instantiated, a db conn is open. The worst thing is that
I haven't seen anywhere in the code the db conn is closed.

I write about this bcoz I see that this app leaves more than one
hundred open connections in the db, and the app complains that the
conn pool is full.

I think first of all, each connection needs to be closed. secondly,
I don't think it is a good idea to open a db conn in the constructor
of a class, unless each and every method of the class needs to visit
the db. I would like to hear you OO gurus' opinion. thank you.

The open + do + close paradigm by others is the way to go.

If you keep connections open, then your app will not scale well,
because you will run out of connections at the database server.

As another general rule try not to do too much in the constructor. A
constructor is expected to bring the object in a consistent state. It
is not expected to do a lot of things besides that.

Arne

 

[Arne Vajhøj]

Well, it is the DataAccessLayer, which talks to a database.

You notice I return an IDataReader, not a specific one. Thus I can swap it
out to a
SqlDataReader
OracleDataReader
MySqlDataReader
OleDBDataReader
and it doesn't affect the business layer code at at all.

True. But you can only switch database - you can not switch to
XML files or some custom native stuff.

Return a collection of a custom class is a level higher at
the encapsulation scale.

See my other blog entry "multiple rdbms with the factory pattern" if you
want to see how to have the DAL support multiple databases in a non hacky
fashion.

That comes with .NET 2.0 ...

Arne

 

[Peter]

True. But you can only switch database - you can not switch to
XML files or some custom native stuff.

This is correct of course, but I thought the bigger "problem" was that
the data layer relies on the business layer to close the connection to
the database.

However, this is also recommended practice from Microsoft. For example,
at
http://msdn.microsoft.com/en-us/library/ms978496.aspx
one can read:

<quote>
Returning Data Readers As Outputs

The advantage of this option is as follows:

Performance. There is a performance benefit when you need to render
data quickly and you can deploy your Data Access Logic Component with
the presentation tier code.

The disadvantage of this option is as follows:

Remoting. It is inadvisable to use data readers in remoting scenarios,
because of the potential for client applications to hold the database
connection open for lengthy periods.
</quote>



/Peter

 

[Arne Vajhøj]

This is correct of course, but I thought the bigger "problem" was that
the data layer relies on the business layer to close the connection to
the database.

Not in my opinion.

However, this is also recommended practice from Microsoft. For example,
at
http://msdn.microsoft.com/en-us/library/ms978496.aspx
one can read:

<quote>
Returning Data Readers As Outputs

The advantage of this option is as follows:

Performance. There is a performance benefit when you need to render
data quickly and you can deploy your Data Access Logic Component with
the presentation tier code.

The disadvantage of this option is as follows:

Remoting. It is inadvisable to use data readers in remoting scenarios,
because of the potential for client applications to hold the database
connection open for lengthy periods.
</quote>

I disagree with it being best practice for the reasons mentioned
previously.

I agree that data readers perform well.

I am not quite sure about their remoting remark. Data readers
are not serializable, so they can not be send to the client. And
it is not obvious to me why the BLL-DAL-DB-DAL-BLL trip should
take longer depending on whether the PL is remote or local.

Arne

 

[Peter]

Not in my opinion.

In your opinion is it a "problem" at all, or is it ok to do this?

What I don't like about it is that (1) the connection to the database
is now open until the business layer closes it; and (2) the business
layer has to have knowledge of the data layer (and data source) to be
able to extract the data, either by use of column names or indices -
meaning the business layer has to have knowledge of the sql which
extracted the data in the first place, and my opinion is that this
knowledge belongs in the data layer, not the business layer.

But it *is* recommeded by Microsoft - so it could be it is a good way
to get data. Seems pointless to have a business layer though.