OO Architecture Question

[Mike Hofer]

In a large application I'm working on (ASP.NET 1.1, VS2003), we have a
base class that wraps stored procedures. Essentially, this base class
(StoredProcedureBase) encapsulates the code to set up the connection,
transaction, command and parameters required to invoke a stored
procedure on our SQL Server database. It provides helper methods that
simplify the process of invoking the stored procedure so that our data
access classes can make the call like this:

Public Function GetBuildings(ByVal siteId As Integer) As
BuildingCollection

Dim buildings As BuildingCollection
Dim connection As SqlConnection

Try
connection = ConnectionGenerator.GetOpenConnection()
buildings = GetBuildingsProcedure.Execute(siteId, connection)
Finally
Disposer.DisposeOf(connection)
End Try

Return buildings

End Function

The stored procedure classes derive from StoredProcedureBase and look
like this:

Public NotInheritable Class GetBuildingsProcedure
Inherits StoredProcedureBase

Private Sub New(ByVal siteId As Integer, ByVal connection As
SqlConnection)
MyBase.New("GetBuildings", connection)
AddParameter("@SiteID", SqlDbType.Int).Value = siteId
End Sub

Public Shared Function Execute(ByVal siteId As Integer, ByVal
connection As SqlConnection)
ValidateOpenConnection(connection)
If siteId <= 0 Then Throw New
ArgumentOutOfRangeException("siteId")
Dim procedure As GetBuildingsProcedure
Dim buildings As BuildingCollection
Dim building As Building
Dim reader As SqlDataReader
Try
procedure = New GetBuildingsProcedure(siteId, connection)
reader = procedure.ExecuteReader()
buildings = New BuildingCollection
Do While reader.Read()
building = New Building()
Store(reader("ID"), -1, building.ID)
Store(reader("AcceptsAdmissions"), False,
building.AcceptsAdmissions)
Store(reader("Description"), String.Empty,
building.Description)
Store(reader("Name"), String.Empty, building.Name)
buildings.Add(building)
Loop
Finally
Disposer.DisposeOf(reader, procedure)
End Try
Return buildings
End Function
End Class

What we like about this model is that it provides *one* place to
invoke the stored procedure, and that it's got really high functional
cohesion. What worries me, however, is that it appears to be tightly
coupled to the Building class. However, it takes very little code to
invoke the stored procedure, and the result set is type-safe.

But the tight coupling is bugging me. It's this annoying little voice
nagging at me in the back of my head. The previous versions of this
class weren't this typesafe, but they weren't so tightly coupled
either. But with that lack of coupling came a lot of code. Refactoring
the code to reduce code complexity resulted in this model; it's only
after looking at the model that we realized we were now tightly
coupled.

However, I'm wondering if the tight coupling isn't worth the reduction
in code duplication. My gut tells me that it is, simply from a
reduction in code complexity and the corresponding ease of
maintenance.

So here are my questions to you folks:

1. Would you do it this way, or would you suggest something else?
2. Do you see any problems with this model?
3. Is the tight coupling worth the trade-off for the reduction in code
complexity and ease of maintenance?
4. Am I missing other concerns that should be taken into account?

I work in a vaccuum here; I am the lone developer at my company, so I
don't have any peer developers to bounce ideas off of. So this is a
desperate attempt to seek input from others with experience. Any
guidance you folks can provide would be greatly appreciated.

Thanks in advance,
Mike

 

[Peter Duniho]

[...]
So here are my questions to you folks:

1. Would you do it this way, or would you suggest something else?
2. Do you see any problems with this model?
3. Is the tight coupling worth the trade-off for the reduction in code
complexity and ease of maintenance?
4. Am I missing other concerns that should be taken into account?

My apologies in advance for not answering the questions in a 1-to-1
manner.

My first thought is that I think you might be worrying about the wrong
thing. Having a class closely coupled to some specific aspect of the data
doesn't seem like a bad idea to me, especially for a derived class. After
all, if you had a base class Shape and a bunch of derived classes, one of
which was Circle, I don't think you'd worry about the Circle class being
tightly coupled to the data required to describe a Circle. If a class is
to represent something, then at some point there's got to be some coupling.

One thing that does seem odd to me about your design is that is sounds as
though your classes that involve stored procedures "know" a lot about
database connections, access, etc. I would expect to either see a
separate class that is more specific to the database and which the stored
procedures class uses, or to see *only* a database class that also happens
to know about procedures.

Of course, I am not seeing the whole class structure and I may be reading
too much into your description of the class hierarchy. But that's my
impression upon reading that description.

Another thing is that your derived class seems to be "upside down". That
is, it has a public static method that does work, and a private
constructor called by that public static method. Clients of the class
never actually instantiate the class (only slightly odd), nor do they ever
get an instance of the class (very odd). I see no point in having a class
that can be instantiated (as opposed to being purely static) if the users
of the class never actually get an instance of it.

I see a couple of alternate approaches that might work better than the
current design in this respect:

* Make the constructor public, and the Execute method non-static. The
user of the class would create an instance of it, and then call the
Execute method on that instance.

* Make the class purely static. Rather than creating an instance of
the class in the Execute method, create an instance of the base class,
initializing it as necessary for the specific situation (as in, do what
the private constructor of the derived class does now) and use that
directly.

Of course, for all I know the base class could be suitably designed as a
purely static class as well. If that's the case, then no instantiating of
any class needs to occur.

As a general rule: use a static class when the class doesn't need to
maintain any state from one function call to another; use a non-static
class if it does. There's nothing about your derived class that appears
to me to require maintaining any state outside the Execute method, so it
can be static.

Take everything in this post, as well as any other replies, with a grain
of salt. Only you can say for sure what's really the best design,
especially given the other already-implemented parts of the application.

Pete

 

[Peter Duniho]

[...]
* Make the class purely static. Rather than creating an instance
of the class in the Execute method, create an instance of the base
class, initializing it as necessary for the specific situation (as in,
do what the private constructor of the derived class does now) and use
that directly.

I should clarify: in this alternate design, there would not necessarily
be any reason for the buildings-specific class to be derived from the base
StoredProcedureBase class. It would be a stand-along static class, unless
you decide that the base class can also be static, in which case it might
still make sense to derive from that class.

Pete

 

[Mike Hofer]

[...]
So here are my questions to you folks:

1. Would you do it this way, or would you suggest something else?
2. Do you see any problems with this model?
3. Is the tight coupling worth the trade-off for the reduction in code
complexity and ease of maintenance?
4. Am I missing other concerns that should be taken into account?

My apologies in advance for not answering the questions in a 1-to-1
manner.

My first thought is that I think you might be worrying about the wrong
thing. Having a class closely coupled to some specific aspect of the data
doesn't seem like a bad idea to me, especially for a derived class. After
all, if you had a base class Shape and a bunch of derived classes, one of
which was Circle, I don't think you'd worry about the Circle class being
tightly coupled to the data required to describe a Circle. If a class is
to represent something, then at some point there's got to be some coupling.

That hadn't occurred to me, but you're right. (Sometimes, I get so
blinded by the details that I forget about some of the more common
sense issues.) It makes sense that this class, which invokes a stored
procedure to retrieve buildings, would know what a Building looks
like. The coupling between data model classes and the stored procedure
classes to retrieve them from the databases is likely an acceptable
coupling (provided that it doesn't break the private data barrier).

One thing that does seem odd to me about your design is that is sounds as
though your classes that involve stored procedures "know" a lot about
database connections, access, etc. I would expect to either see a
separate class that is more specific to the database and which the stored
procedures class uses, or to see *only* a database class that also happens
to know about procedures.

Of course, I am not seeing the whole class structure and I may be reading
too much into your description of the class hierarchy. But that's my
impression upon reading that description.

Actually, they don't. The database connections are handled by a
separate class, ConnectionGenerator (a factory). Further, the
ConnectionGenerator class doesn't know the settings; those are stored
in the application's configuration file, and passed to the
ConnectionGeerator during startup. GetOpenConnection() is one override
that uses the default settings to return an open database connection
to the caller. The retrieval of a database connection always takes
place *outside* of the stored procedure class, nominally in in a data
access class (BuildingDAC, for instance). The stored procedure classes
themselves always take either a SqlConnection or a SqlTransaction as
an argument to the Execute method, which is then forwarded to the base
class constructor so that the internal Command object can be properly
configured.

Another thing is that your derived class seems to be "upside down". That
is, it has a public static method that does work, and a private
constructor called by that public static method. Clients of the class
never actually instantiate the class (only slightly odd), nor do they ever
get an instance of the class (very odd). I see no point in having a class
that can be instantiated (as opposed to being purely static) if the users
of the class never actually get an instance of it.

I see a couple of alternate approaches that might work better than the
current design in this respect:

* Make the constructor public, and the Execute method non-static. The
user of the class would create an instance of it, and then call the
Execute method on that instance.

* Make the class purely static. Rather than creating an instance of
the class in the Execute method, create an instance of the base class,
initializing it as necessary for the specific situation (as in, do what
the private constructor of the derived class does now) and use that
directly.

Of course, for all I know the base class could be suitably designed as a
purely static class as well. If that's the case, then no instantiating of
any class needs to occur.

As a general rule: use a static class when the class doesn't need to
maintain any state from one function call to another; use a non-static
class if it does. There's nothing about your derived class that appears
to me to require maintaining any state outside the Execute method, so it
can be static.

Now that's a valid concern. Here's where I was coming from.

I noticed early on that .NET doesn't provide a standard object to
simplify access to stored procedures. But the steps to invoke one are
typically the same (shown here for retrieving a set of data with a
data reader):

1.) Create a connection
2.) Create a command
3.) Create a parameters
4.) Assign the SQL statement
5.) Set the commandtype
6.) Execute the command and get the data reader
7.) Do your work with the reader
8.) Dispose of the reader
9.) Dispose of the command
10.) Dispose of the connection

That's why I wrote the class to wrap this stuff. And it made a huge
difference. I didn't have to rewrite that same, monotonous code for
stored procedures everywhere in my code. And if a parameter changed in
a sproc, it was a lot easier to propagate the changes. But I digress.

The first version of my stored procedure base class didn't use the
Private Constructor/Public Execute model; instead, it used
instantiable stored procedure classes, with one property for every
parameter. In that model, however, you had to write a lot of code to
represent a stored procedure. And the code to invoke the stored
procedure was bloated as well.

For instance, the DAC layer function looked like this:

Public Function GetBuildings(ByVal siteId As Integer) As
BuildingCollection

Dim buildings As BuildingCollection
Dim building As Building
Dim connection As SqlConnection
Dim procedure As GetBuildingsProcedure

Try
connection = ConnectionGenerator.GetOpenConnection()
procedure = New GetBuildingsProcedure(connection)
procedure.SiteId = siteId
reader = procedure.ExecuteQuery()
buildings = New BuildingCollection
Do While reader.Read()
bulding = New Building
Store(reader("AcceptsBuilding"), False,
building.AcceptsAdmissions)
Store(reader("Description"), False, building.Description)
Store(reader("ID"), -1, building.ID)
Store(reader("SiteId"), -1, building.SiteId)
Store(reader("Name"), String.Empty, building.Name)
buildings.Add(building)
Loop
Finally
Disposer.DisposeOf(connection)
End Try

Return buildings

End Function

(This is a poor example, since this stored procedure only takes one
parameter. We have some stored procedures that take a lot of
parameters, especially those that update data in the database.) The
stored procedure class would then simply pass the parameters through
to the Parameters collection on the internal Command object in the
base class.

Through refactoring, I found that there were several occasions where I
wanted to invoke the same procedure from several different DAC
classes. (This has proven to be the case for many stored procedures in
the system.) The steps required to invoke the procedure were the same.
The logical place to put the code was in the StoredProcedure class
itself.

Now here's the interesting bit: *Not all stored procedure classes in
this system use the newer model.* To resolve the issue, a new base
class was written (StoredProcedureBase) that derives from the older
stored procedure base class (the incorrectly named StoredProcedure).
Both classes are abstract (marked MustInherit in VB). The newer stored
procedure classes inherit from StoredProcedureBase. The older stored
procedure classes still use the older StoredProcedure class, and must
be instantiated.

However, when it comes time to add new stored procedures to the
system, it takes a hell of a lot less time to add them using the newer
model. There's a lot less code to write, simply because I don't have
to write a property for each parameter, nor do I have to write code to
populate the properties from within the DAC layer. The Execute method
validates the parameters and throws exceptions if any of the
parameters are invalid.

So yes, it's a class. The base class (StoredProcedure) maintains
state, but the derived classes (StoredProcedureBase and
GetBuildingProcedure) do not. But the class abstracts away the details
of the setup of the call. The static (Shared) Execute method invokes
the private constructor to ensure that the stored procedure class is
created correctly, and then disposed of properly. That, right there,
is a huge boon: it ensures that Dispose is called on the stored
procedure and that the internal Commmand is properly garbage
collected.

It's a long, messy story. I just wanted to make sure that the class
instances were properly created, that the parameters were set up
right, that the Execute method returned typesafe values, and that the
internal objects were always garbage collected.

Take everything in this post, as well as any other replies, with a grain
of salt. Only you can say for sure what's really the best design,
especially given the other already-implemented parts of the application.

Pete

Thanks very much for your feedback! It's appreciated!

Mike

 

[Jeff Jarrell]

mike,

I have actually done a LOT of stuff around this sort of model. Aside from
having database neutral factories for building the connection, command, and
parm objects I use wrote a CodeSmith template to generate the wrapper code.
It is always the same structuure and it always works. If the parms or
result set changes, I regen the wrapper. If the concept in the wrapper can
change, I try and keep it set so I regen all of the wrappers in batch mode.

jeff

[...]
So here are my questions to you folks:

1. Would you do it this way, or would you suggest something else?
2. Do you see any problems with this model?
3. Is the tight coupling worth the trade-off for the reduction in code
complexity and ease of maintenance?
4. Am I missing other concerns that should be taken into account?

My apologies in advance for not answering the questions in a 1-to-1
manner.

My first thought is that I think you might be worrying about the wrong
thing. Having a class closely coupled to some specific aspect of the
data
doesn't seem like a bad idea to me, especially for a derived class.
After
all, if you had a base class Shape and a bunch of derived classes, one of
which was Circle, I don't think you'd worry about the Circle class being
tightly coupled to the data required to describe a Circle. If a class is
to represent something, then at some point there's got to be some
coupling.

That hadn't occurred to me, but you're right. (Sometimes, I get so
blinded by the details that I forget about some of the more common
sense issues.) It makes sense that this class, which invokes a stored
procedure to retrieve buildings, would know what a Building looks
like. The coupling between data model classes and the stored procedure
classes to retrieve them from the databases is likely an acceptable
coupling (provided that it doesn't break the private data barrier).

One thing that does seem odd to me about your design is that is sounds as
though your classes that involve stored procedures "know" a lot about
database connections, access, etc. I would expect to either see a
separate class that is more specific to the database and which the stored
procedures class uses, or to see *only* a database class that also
happens
to know about procedures.

Of course, I am not seeing the whole class structure and I may be reading
too much into your description of the class hierarchy. But that's my
impression upon reading that description.

Actually, they don't. The database connections are handled by a
separate class, ConnectionGenerator (a factory). Further, the
ConnectionGenerator class doesn't know the settings; those are stored
in the application's configuration file, and passed to the
ConnectionGeerator during startup. GetOpenConnection() is one override
that uses the default settings to return an open database connection
to the caller. The retrieval of a database connection always takes
place *outside* of the stored procedure class, nominally in in a data
access class (BuildingDAC, for instance). The stored procedure classes
themselves always take either a SqlConnection or a SqlTransaction as
an argument to the Execute method, which is then forwarded to the base
class constructor so that the internal Command object can be properly
configured.

Another thing is that your derived class seems to be "upside down". That
is, it has a public static method that does work, and a private
constructor called by that public static method. Clients of the class
never actually instantiate the class (only slightly odd), nor do they
ever
get an instance of the class (very odd). I see no point in having a
class
that can be instantiated (as opposed to being purely static) if the users
of the class never actually get an instance of it.

I see a couple of alternate approaches that might work better than the
current design in this respect:

* Make the constructor public, and the Execute method non-static.
The
user of the class would create an instance of it, and then call the
Execute method on that instance.

* Make the class purely static. Rather than creating an instance of
the class in the Execute method, create an instance of the base class,
initializing it as necessary for the specific situation (as in, do what
the private constructor of the derived class does now) and use that
directly.

Of course, for all I know the base class could be suitably designed as a
purely static class as well. If that's the case, then no instantiating
of
any class needs to occur.

As a general rule: use a static class when the class doesn't need to
maintain any state from one function call to another; use a non-static
class if it does. There's nothing about your derived class that appears
to me to require maintaining any state outside the Execute method, so it
can be static.

Now that's a valid concern. Here's where I was coming from.

I noticed early on that .NET doesn't provide a standard object to
simplify access to stored procedures. But the steps to invoke one are
typically the same (shown here for retrieving a set of data with a
data reader):

1.) Create a connection
2.) Create a command
3.) Create a parameters
4.) Assign the SQL statement
5.) Set the commandtype
6.) Execute the command and get the data reader
7.) Do your work with the reader
8.) Dispose of the reader
9.) Dispose of the command
10.) Dispose of the connection

That's why I wrote the class to wrap this stuff. And it made a huge
difference. I didn't have to rewrite that same, monotonous code for
stored procedures everywhere in my code. And if a parameter changed in
a sproc, it was a lot easier to propagate the changes. But I digress.

The first version of my stored procedure base class didn't use the
Private Constructor/Public Execute model; instead, it used
instantiable stored procedure classes, with one property for every
parameter. In that model, however, you had to write a lot of code to
represent a stored procedure. And the code to invoke the stored
procedure was bloated as well.

For instance, the DAC layer function looked like this:

Public Function GetBuildings(ByVal siteId As Integer) As
BuildingCollection

Dim buildings As BuildingCollection
Dim building As Building
Dim connection As SqlConnection
Dim procedure As GetBuildingsProcedure

Try
connection = ConnectionGenerator.GetOpenConnection()
procedure = New GetBuildingsProcedure(connection)
procedure.SiteId = siteId
reader = procedure.ExecuteQuery()
buildings = New BuildingCollection
Do While reader.Read()
bulding = New Building
Store(reader("AcceptsBuilding"), False,
building.AcceptsAdmissions)
Store(reader("Description"), False, building.Description)
Store(reader("ID"), -1, building.ID)
Store(reader("SiteId"), -1, building.SiteId)
Store(reader("Name"), String.Empty, building.Name)
buildings.Add(building)
Loop
Finally
Disposer.DisposeOf(connection)
End Try

Return buildings

End Function

(This is a poor example, since this stored procedure only takes one
parameter. We have some stored procedures that take a lot of
parameters, especially those that update data in the database.) The
stored procedure class would then simply pass the parameters through
to the Parameters collection on the internal Command object in the
base class.

Through refactoring, I found that there were several occasions where I
wanted to invoke the same procedure from several different DAC
classes. (This has proven to be the case for many stored procedures in
the system.) The steps required to invoke the procedure were the same.
The logical place to put the code was in the StoredProcedure class
itself.

Now here's the interesting bit: *Not all stored procedure classes in
this system use the newer model.* To resolve the issue, a new base
class was written (StoredProcedureBase) that derives from the older
stored procedure base class (the incorrectly named StoredProcedure).
Both classes are abstract (marked MustInherit in VB). The newer stored
procedure classes inherit from StoredProcedureBase. The older stored
procedure classes still use the older StoredProcedure class, and must
be instantiated.

However, when it comes time to add new stored procedures to the
system, it takes a hell of a lot less time to add them using the newer
model. There's a lot less code to write, simply because I don't have
to write a property for each parameter, nor do I have to write code to
populate the properties from within the DAC layer. The Execute method
validates the parameters and throws exceptions if any of the
parameters are invalid.

So yes, it's a class. The base class (StoredProcedure) maintains
state, but the derived classes (StoredProcedureBase and
GetBuildingProcedure) do not. But the class abstracts away the details
of the setup of the call. The static (Shared) Execute method invokes
the private constructor to ensure that the stored procedure class is
created correctly, and then disposed of properly. That, right there,
is a huge boon: it ensures that Dispose is called on the stored
procedure and that the internal Commmand is properly garbage
collected.

It's a long, messy story. I just wanted to make sure that the class
instances were properly created, that the parameters were set up
right, that the Execute method returned typesafe values, and that the
internal objects were always garbage collected.

Take everything in this post, as well as any other replies, with a grain
of salt. Only you can say for sure what's really the best design,
especially given the other already-implemented parts of the application.

Pete

Thanks very much for your feedback! It's appreciated!

Mike

 

[John Wright]

To solve the issue here I wrote a generic DAL class using the
System.Data.Common namespace and the factory pattern. The class uses a
parameter collection and will call any stored procedure on the database
regardless of the backend (SQL Server, Oracle, Text Files, Access, ODBC
database...). We hit a few snags in the Oracle arena trying to get a cursor
object back (similiar to calling a stored proc in SQL Server that returns a
data table), but we found a way around this that works quite well. Dropping
the DAL into any program allows the developer to wrap it with their own
functions to type variables but leaves it open enough to switch databases on
the fly (I have a program that has to access both SQL Server and Oracle. It
connects to both on the fly without any problem.) So far it was worked
beautifully on both WinForms and ASP.NET.

Maybe the Common namespace is something to explore.

John

mike,

I have actually done a LOT of stuff around this sort of model. Aside from
having database neutral factories for building the connection, command,
and parm objects I use wrote a CodeSmith template to generate the wrapper
code. It is always the same structuure and it always works. If the parms
or result set changes, I regen the wrapper. If the concept in the wrapper
can change, I try and keep it set so I regen all of the wrappers in batch
mode.

jeff

[...]
So here are my questions to you folks:

1. Would you do it this way, or would you suggest something else?
2. Do you see any problems with this model?
3. Is the tight coupling worth the trade-off for the reduction in code
complexity and ease of maintenance?
4. Am I missing other concerns that should be taken into account?

My apologies in advance for not answering the questions in a 1-to-1
manner.

My first thought is that I think you might be worrying about the wrong
thing. Having a class closely coupled to some specific aspect of the
data
doesn't seem like a bad idea to me, especially for a derived class.
After
all, if you had a base class Shape and a bunch of derived classes, one
of
which was Circle, I don't think you'd worry about the Circle class being
tightly coupled to the data required to describe a Circle. If a class
is
to represent something, then at some point there's got to be some
coupling.

That hadn't occurred to me, but you're right. (Sometimes, I get so
blinded by the details that I forget about some of the more common
sense issues.) It makes sense that this class, which invokes a stored
procedure to retrieve buildings, would know what a Building looks
like. The coupling between data model classes and the stored procedure
classes to retrieve them from the databases is likely an acceptable
coupling (provided that it doesn't break the private data barrier).

One thing that does seem odd to me about your design is that is sounds
as
though your classes that involve stored procedures "know" a lot about
database connections, access, etc. I would expect to either see a
separate class that is more specific to the database and which the
stored
procedures class uses, or to see *only* a database class that also
happens
to know about procedures.

Of course, I am not seeing the whole class structure and I may be
reading
too much into your description of the class hierarchy. But that's my
impression upon reading that description.

Actually, they don't. The database connections are handled by a
separate class, ConnectionGenerator (a factory). Further, the
ConnectionGenerator class doesn't know the settings; those are stored
in the application's configuration file, and passed to the
ConnectionGeerator during startup. GetOpenConnection() is one override
that uses the default settings to return an open database connection
to the caller. The retrieval of a database connection always takes
place *outside* of the stored procedure class, nominally in in a data
access class (BuildingDAC, for instance). The stored procedure classes
themselves always take either a SqlConnection or a SqlTransaction as
an argument to the Execute method, which is then forwarded to the base
class constructor so that the internal Command object can be properly
configured.

Another thing is that your derived class seems to be "upside down".
That
is, it has a public static method that does work, and a private
constructor called by that public static method. Clients of the class
never actually instantiate the class (only slightly odd), nor do they
ever
get an instance of the class (very odd). I see no point in having a
class
that can be instantiated (as opposed to being purely static) if the
users
of the class never actually get an instance of it.

I see a couple of alternate approaches that might work better than the
current design in this respect:

* Make the constructor public, and the Execute method non-static.
The
user of the class would create an instance of it, and then call the
Execute method on that instance.

* Make the class purely static. Rather than creating an instance
of
the class in the Execute method, create an instance of the base class,
initializing it as necessary for the specific situation (as in, do what
the private constructor of the derived class does now) and use that
directly.

Of course, for all I know the base class could be suitably designed as a
purely static class as well. If that's the case, then no instantiating
of
any class needs to occur.

As a general rule: use a static class when the class doesn't need to
maintain any state from one function call to another; use a non-static
class if it does. There's nothing about your derived class that appears
to me to require maintaining any state outside the Execute method, so it
can be static.

Now that's a valid concern. Here's where I was coming from.

I noticed early on that .NET doesn't provide a standard object to
simplify access to stored procedures. But the steps to invoke one are
typically the same (shown here for retrieving a set of data with a
data reader):

1.) Create a connection
2.) Create a command
3.) Create a parameters
4.) Assign the SQL statement
5.) Set the commandtype
6.) Execute the command and get the data reader
7.) Do your work with the reader
8.) Dispose of the reader
9.) Dispose of the command
10.) Dispose of the connection

That's why I wrote the class to wrap this stuff. And it made a huge
difference. I didn't have to rewrite that same, monotonous code for
stored procedures everywhere in my code. And if a parameter changed in
a sproc, it was a lot easier to propagate the changes. But I digress.

The first version of my stored procedure base class didn't use the
Private Constructor/Public Execute model; instead, it used
instantiable stored procedure classes, with one property for every
parameter. In that model, however, you had to write a lot of code to
represent a stored procedure. And the code to invoke the stored
procedure was bloated as well.

For instance, the DAC layer function looked like this:

Public Function GetBuildings(ByVal siteId As Integer) As
BuildingCollection

Dim buildings As BuildingCollection
Dim building As Building
Dim connection As SqlConnection
Dim procedure As GetBuildingsProcedure

Try
connection = ConnectionGenerator.GetOpenConnection()
procedure = New GetBuildingsProcedure(connection)
procedure.SiteId = siteId
reader = procedure.ExecuteQuery()
buildings = New BuildingCollection
Do While reader.Read()
bulding = New Building
Store(reader("AcceptsBuilding"), False,
building.AcceptsAdmissions)
Store(reader("Description"), False, building.Description)
Store(reader("ID"), -1, building.ID)
Store(reader("SiteId"), -1, building.SiteId)
Store(reader("Name"), String.Empty, building.Name)
buildings.Add(building)
Loop
Finally
Disposer.DisposeOf(connection)
End Try

Return buildings

End Function

(This is a poor example, since this stored procedure only takes one
parameter. We have some stored procedures that take a lot of
parameters, especially those that update data in the database.) The
stored procedure class would then simply pass the parameters through
to the Parameters collection on the internal Command object in the
base class.

Through refactoring, I found that there were several occasions where I
wanted to invoke the same procedure from several different DAC
classes. (This has proven to be the case for many stored procedures in
the system.) The steps required to invoke the procedure were the same.
The logical place to put the code was in the StoredProcedure class
itself.

Now here's the interesting bit: *Not all stored procedure classes in
this system use the newer model.* To resolve the issue, a new base
class was written (StoredProcedureBase) that derives from the older
stored procedure base class (the incorrectly named StoredProcedure).
Both classes are abstract (marked MustInherit in VB). The newer stored
procedure classes inherit from StoredProcedureBase. The older stored
procedure classes still use the older StoredProcedure class, and must
be instantiated.

However, when it comes time to add new stored procedures to the
system, it takes a hell of a lot less time to add them using the newer
model. There's a lot less code to write, simply because I don't have
to write a property for each parameter, nor do I have to write code to
populate the properties from within the DAC layer. The Execute method
validates the parameters and throws exceptions if any of the
parameters are invalid.

So yes, it's a class. The base class (StoredProcedure) maintains
state, but the derived classes (StoredProcedureBase and
GetBuildingProcedure) do not. But the class abstracts away the details
of the setup of the call. The static (Shared) Execute method invokes
the private constructor to ensure that the stored procedure class is
created correctly, and then disposed of properly. That, right there,
is a huge boon: it ensures that Dispose is called on the stored
procedure and that the internal Commmand is properly garbage
collected.

It's a long, messy story. I just wanted to make sure that the class
instances were properly created, that the parameters were set up
right, that the Execute method returned typesafe values, and that the
internal objects were always garbage collected.

Take everything in this post, as well as any other replies, with a grain
of salt. Only you can say for sure what's really the best design,
especially given the other already-implemented parts of the application.

Pete

Thanks very much for your feedback! It's appreciated!

Mike

 

[sloan]

Here are my thoughts:

If you're a lone developer, then don't reinvent the wheel.

Go get either the "Data Access Application Block 2.0" (sql server specific)
or the
EnterpriseLibrary.Data ( factory setup with support for Sql Server, Oracle )


If you go here:
http://sholliday.spaces.live.com/blog/
and find this entry:
5/24/2006
Custom Objects/Collections and Tiered Development

You can see how I'm taking IDataReaders, and making them into objects, even
hierarchy ones ( Customers with Orders ).

I also describe the approach here:
http://www.codeproject.com/useritem...rumid=322471&exp=0&select=1954453#xx1954453xx

If you need to populate a Circle (and its a Shape), you still gotta write a
query/stored procedure that gets back Circle data.
Either that, or you'll have 1,000 if statements in your idatareader to
object code.
(if type in db is a circle, then set these properties )
(if type in db is a rectangle, hten set these properties)

I wouldn't do that. I'd have a "deserializer" (as in my blog code) specific
to each object.

Like I said, I wouldn't reinvent the wheel. The EnterpriseLibrary.Data is
something you can trust, and trust people alot smarter than you and me
combined help put together.

I like my approach (which has been fine tuned over hte last 3 years, and
with help from a guy who helped me understand how to write code that didn't
care about whether your backend db was sql server, oracle, access or any
other datastore).

 

[Peter Duniho]

[...]
So yes, it's a class. The base class (StoredProcedure) maintains
state, but the derived classes (StoredProcedureBase and
GetBuildingProcedure) do not. But the class abstracts away the details
of the setup of the call. The static (Shared) Execute method invokes
the private constructor to ensure that the stored procedure class is
created correctly, and then disposed of properly. That, right there,
is a huge boon: it ensures that Dispose is called on the stored
procedure and that the internal Commmand is properly garbage
collected.

Well, that brings be me back to my previous suggestion:

It sounds as though your object-specific classes aren't actually instances
of the stored procedure base class. They are users of that class. No
state is stored in the derived class and the callers never see the
instance you do make. So it seems to me it makes more sense to not bother
deriving the object-specific classes from the stored procedure class, and
instead just have them be static classes that user the base stored
procedure class in their static method.

If it ever becomes the case that the object-specific classes *do* need to
store state, then it's practically certain that will mean that users of
the class will at that point need to instantiate the class and keep the
instance reference. So it's not as though your current design won't
require you to revisit each and every use of the class anyway if you make
that change.

There's nothing technically wrong with the way you're doing it now, but it
does seem to be a departure from usual OOP design, and that sort of thing
can make code harder to maintain later on (as someone, perhaps even you,
looks at it and scratches their head wondering "why'd it get written like
this?" .

Anyway, that's about all I have in the way of potentially useful input. I
don't feel strongly that the current design is wrong...it's just a
suggestion about how it might be improved to be a little confusing.

Pete

 

[Mike Hofer]

Here are my thoughts:

If you're a lone developer, then don't reinvent the wheel.

Go get either the "Data Access Application Block 2.0" (sql server specific)
or the
EnterpriseLibrary.Data ( factory setup with support for Sql Server, Oracle )

If you go here:http://sholliday.spaces.live.com/blog/
and find this entry:
5/24/2006
Custom Objects/Collections and Tiered Development

You can see how I'm taking IDataReaders, and making them into objects, even
hierarchy ones ( Customers with Orders ).

I also describe the approach here:http://www.codeproject.com/useritems/BusinessObjectHelper.asp?df=100&...

If you need to populate a Circle (and its a Shape), you still gotta write a
query/stored procedure that gets back Circle data.
Either that, or you'll have 1,000 if statements in your idatareader to
object code.
(if type in db is a circle, then set these properties )
(if type in db is a rectangle, hten set these properties)

I wouldn't do that. I'd have a "deserializer" (as in my blog code) specific
to each object.

Like I said, I wouldn't reinvent the wheel. The EnterpriseLibrary.Data is
something you can trust, and trust people alot smarter than you and me
combined help put together.

I like my approach (which has been fine tuned over hte last 3 years, and
with help from a guy who helped me understand how to write code that didn't
care about whether your backend db was sql server, oracle, access or any
other datastore).






















- Show quoted text -

Hiya sloan,

Thanks for the pointer. I actually looked at both of these solutions.
Unfortunately, the vast majority of the code in question was generated
by another tool. There are 600 or so classes generated to do *exactly*
what you're describing. That is, get the data from the stored
procedure, shuffle it into a typesafe object (or typesafe collection),
etc. Essentially, every time the database schema changed, the tool
would regenerate the stored procecure classes, the data model classes,
and the data access classes so that the code stayed in synch with the
database. (It used base classes for the generated code, and provided
stub derived classes for you to provide extensions.)

Now, I don't mind the typesafe nature of the classes. Far from it. I
*like* the typesafety. I also *like* the fact that I don't have
DataSets all over my application. (Let's not get into that; suffice it
to say that I think a DataSet is overkill when you know exactly what
you want out of the database.)

Anyway, the application is now two years old, and the amount of
generated code makes a change of architecture on that scale
impractical. Inserting a derived class into the inheritance chain
wasn't, and it solved a good number of problems. The code is fast,
typesafe, and very efficient at garbage collection.

Now, future projects, built from scratch...that's a different story.
But those applications will likely be done in .NET 2.0 anyway.

Mike

 

[Mike Hofer]

[...]
So yes, it's a class. The base class (StoredProcedure) maintains
state, but the derived classes (StoredProcedureBase and
GetBuildingProcedure) do not. But the class abstracts away the details
of the setup of the call. The static (Shared) Execute method invokes
the private constructor to ensure that the stored procedure class is
created correctly, and then disposed of properly. That, right there,
is a huge boon: it ensures that Dispose is called on the stored
procedure and that the internal Commmand is properly garbage
collected.

Well, that brings be me back to my previous suggestion:

It sounds as though your object-specific classes aren't actually instances
of the stored procedure base class. They are users of that class. No
state is stored in the derived class and the callers never see the
instance you do make. So it seems to me it makes more sense to not bother
deriving the object-specific classes from the stored procedure class, and
instead just have them be static classes that user the base stored
procedure class in their static method.

If it ever becomes the case that the object-specific classes *do* need to
store state, then it's practically certain that will mean that users of
the class will at that point need to instantiate the class and keep the
instance reference. So it's not as though your current design won't
require you to revisit each and every use of the class anyway if you make
that change.

There's nothing technically wrong with the way you're doing it now, but it
does seem to be a departure from usual OOP design, and that sort of thing
can make code harder to maintain later on (as someone, perhaps even you,
looks at it and scratches their head wondering "why'd it get written like
this?" .

Anyway, that's about all I have in the way of potentially useful input. I
don't feel strongly that the current design is wrong...it's just a
suggestion about how it might be improved to be a little confusing.

Pete

I think you may be missing some points about the way the objects are
designed. It's nothing major, just probably not obvious.

The interface on the StoredProcedureBase class looks like this:

Public MustInherit Class StoredProcedureBase
Inherits StoredProcedure

Protected Sub New(ByVal procedureName As String, ByVal connection
As SqlConnection)
Protected Sub New(ByVal procedureName As String, ByVal transaction
As SqlTransaction)

Protected Shadows Function ExecuteNonQuery() As Integer
Protected Shadows Function ExecuteReader() As SqlDataReader
Protected Shadows Function ExecuteScalar() As Object

End Class

The interface on the StoredProcedure class (which was generated) looks
like this:

Public MustInherit Class StoredProcedure
Protected Sub New(ByVal procedureName As String)
Protected Sub New(ByVal procedureName As String, ByVal
connectionString As String)
Protected Sub New(ByVal procedureName As String, ByVal connection
As SqlConnection)
Protected Sub New()

Protected Function AddParameter(ByVal name As String, ByVal [type]
As SqlDbType) As SqlParameter
Protected Function AddParameter(ByVal name As String, ByVal [type]
As SqlDbType, ByVal value As Object) As SqlParameter
Protected Function AddParameter(ByVal name As String, ByVal [type]
As SqlDbType, ByVal size As Integer) As SqlParameter
Protected Function AddParameter(ByVal name As String, ByVal [type]
As SqlDbType, ByVal direction As ParameterDirection) As SqlParameter
Protected Function AddParameter(ByVal name As String, ByVal [type]
As SqlDbType, ByVal size As Integer, ByVal value As Object) As
SqlParameter

Private Shared m_defaultConnectionString As String
Public Shared Property DefaultConnectionString() As String

Protected Property IsDisposed() As Boolean

Public Overridable Sub Dispose() Implements IDisposable.Dispose

Public Function ExecuteNonQuery() As Integer
Public Function ExecuteReader() As SqlDataReader
Public Function ExecuteScalar() As Object

Protected ReadOnly Property Parameters() As SqlParameterCollection

Public Property Transaction() As SqlTransaction

End class

Internally, StoredProcedure maintains a reference to a SqlCommand and
a SqlConnection. These are the pieces of state that are maintained by
instances of derived classes. When a class is derived from
StoredProcedureBase, it does, indeed, inherit these pieces of state
data. The fact that the caller never directly instantiates them is
neither here nor there; the instantiation is abstracted away in order
to ensure that the Dispose method is invoked.

I had a number of issues with the generated StoredProcedure class.
First, it provided constructors that allowed you to call it without
providing a connection or a transaction. The result was that one was
*automatically* created for you, which was *highly* inefficient. I
wanted to lock down the constructors.

Second, the Dispose method in the generated StoredProcedure class was
buggy. It involved fairly erroneous assumptions about whether or not
it was safe to dispose of the connection. Sometimes it disposed of it
prematurely; other times, it didn't dispose of it at all. The newer
class resolves the issue by demanding that a connection or a
transaction be provided. Connections are never automagically created.

Anyway, there's not really any big hangup. The class does maintain
state; it just maintains it for an extremely short period of time. I
wondered periodically if the whole thing wasn't overengineered. But
every time I think about dispensing with the class, I keep thinking
about how much duplicate code I'd be writing again. And then I come
right back to the DRY principle, and realize I'd refactor myself right
back to where I am (or something very close to it).

This is the kind of thing that happens when developers work alone,
without the benefit of peers to consult.

And yeah, I'm bitter about it.
Mike