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]]

Re: open a db connection in a constructor ?

On Jul 1, 5:38 am, Author <gnewsgr...@gma il.comwrote:

<snip>

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]

Re: open a db connection in a constructor ?

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.space s.live.com/Blog/cns!A68482B9628 A842A!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" <gnewsgroup@gma il.comwrote in message
news:e3477dd1-160a-4f75-a94b-f7f231a0044d@z6 6g2000hsc.googl egroups.com...

>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.
>

[Author]

Re: open a db connection in a constructor ?

On Jul 1, 2:21 am, "Jon Skeet [C# MVP]" <sk...@pobox.co mwrote:

On Jul 1, 5:38 am, Author <gnewsgr...@gma il.comwrote:
>
<snip>
>

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

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 = ConfigurationSe ttings.AppSetti ngs.Item("cntSt ring")
Me.con = New SqlConnection
Me.con.Connecti onString = Me.cntString
Me.cmd = New SqlCommand
Me.cmd.Connecti on = Me.con
Me.con.Open // Look, the db connection is open here.
End Sub
......

[Author]

Re: open a db connection in a constructor ?

On Jul 1, 5:04 am, "sloan" <sl...@ipass.ne twrote:

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.space s.live.com/Blog/cns!A68482B9628 A842A!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]

Re: open a db connection in a constructor ?

On Jul 1, 5:04 am, "sloan" <sl...@ipass.ne twrote:

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.space s.live.com/Blog/cns!A68482B9628 A842A!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.IsN othing(Me.dataR eader) Then
Me.dataReader = Nothing
End If
If Not Information.IsN othing(Me.cmd) Then
Me.cmd = Nothing
End If
If Not Information.IsN othing(Me.con) Then
Me.con.Close
Me.con = Nothing
End If
Catch exception1 As Exception
ProjectData.Set ProjectError(ex ception1)
Dim exception As Exception = exception1
ProjectData.Cle arProjectError
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]]

Re: open a db connection in a constructor ?

On Jul 1, 2:05 pm, Author <gnewsgr...@gma il.comwrote:

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 )]

Re: open a db connection in a constructor ?

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]

Re: open a db connection in a constructor ?

On Jul 1, 10:04 am, "Jon Skeet [C# MVP]" <sk...@pobox.co mwrote:

On Jul 1, 2:05 pm, Author <gnewsgr...@gma il.comwrote:
>

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

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]

Re: open a db connection in a constructor ?

On Jul 1, 10:40 am, "Ignacio Machin ( .NET/ C# MVP )"
<ignacio.mac... @gmail.comwrote :

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

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(m yConnectionStri ng))
using (SqlCommand command = new SqlCommand(mySq lQuery, connection))
using (SqlDataAdapter dataAdapter = new SqlDataAdapter( command))
using (DataSet dataSet = new DataSet())
{
command.Command Type = CommandType.Tex t;
connection.Open ();
dataAdapter.Fil l(dataSet);
}

[Jon Skeet [C# MVP]]

Re: open a db connection in a constructor ?

Author <gnewsgroup@gma il.comwrote:

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.

>
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.

--
Jon Skeet - <skeet@pobox.co m>
Web site: http://www.pobox.com/~skeet
Blog: http://www.msmvps.com/jon_skeet
C# in Depth: http://csharpindepth.com

[Peter]

Re: open a db connection in a constructor ?

sloan wrote:

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.space s.live.com/Blog/cns!A68482B9628 A842A!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 CustomersGetAll Reader()
{
return
Microsoft.Appli cationBlocks.Da ta.SqlHelper.Ex ecuteReader(m_c onnectionStr
ing, this.PROC_CUSTO MERS_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]

Re: open a db connection in a constructor ?


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
OracleDataReade r
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.Clos e() method.
The EnterpriseLibra ry 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
EnterpriseLibra ry.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)DataSet s 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 PresentationLay er. 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" <xdzgor@hotmail .comwrote in message
news:OGntRIC3IH A.1192@TK2MSFTN GP05.phx.gbl...

sloan wrote:
>

>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.space s.live.com/Blog/cns!A68482B9628 A842A!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 CustomersGetAll Reader()
{
return
Microsoft.Appli cationBlocks.Da ta.SqlHelper.Ex ecuteReader(m_c onnectionStr
ing, this.PROC_CUSTO MERS_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

[Peter]

Re: open a db connection in a constructor ?

sloan wrote:

>
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
OracleDataReade r
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.Clos e() method. The
EnterpriseLibra ry 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 EnterpriseLibra ry.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)DataSet s 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 PresentationLay er. 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

[=?ISO-8859-1?Q?Arne_Vajh=F8j?=]

Re: open a db connection in a constructor ?

Author wrote:

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