2

I'm looking at a class handling database access for an MVC3/.Net application.

The class is static, and provides nice convenience methods for common DB queries - all sorts of twiddly stuff like "GetColumnBValueForColumnA()", as well as much more complex queries. It's nicely factored/optimized for the given solution and domain.

Seeing the class as static, though, triggered some half forgotten memory about how this might be a bad idea (maybe in the context of multi-threading?), and I can't shake the feeling.

Is it a good idea to keep this kind of class static, or should I be instantiating it for each database call?

Improve this question
5
  • This is very common in the Active Record pattern.. Nothing wrong with it at all! Commented Jan 23, 2012 at 21:22
  • is there any static state? is it meant to implement any kind of abstraction (interface etc)? Commented Jan 23, 2012 at 21:23
  • @MarcGravell These classes don't contain any static state. At some point they instantiate a wrapper around the underlying database connection, as an aide to constructing the relevant queries. Commented Jan 23, 2012 at 21:26
  • @blueberryfields and where do they get the underlying database connection from, if not from static state? Do you pass in the connection as a parameter? Commented Jan 23, 2012 at 21:29
  • I'm still wrapping my head around this code. It looks like the wrapper is instantiated (and creates a DB connection) for each query Commented Jan 23, 2012 at 21:40

2 Answers 2

Reset to default
5

Is it a good idea to keep this kind of class static, or should I be instantiating it for each database call?

If you care about things like weak coupling between the layers of your application, reusability of those layers, unit testing in isolation you shouldn't be doing any of the above. You should be working with abstractions.

If you don't care about those things then static methods are just fine. The only thing to be careful when working with static methods is to design them so that they are reentrant and do not depend on any shared state in order to be thread safe. In all cases make sure to properly dispose all IDisposable resources such as database connections and commands by wrapping them in using statements.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

"working with abstractions" is a bit general - what do you mean when you say that?
@blueberryfields, concretely abstractions in .NET are either interfaces or abstract classes. So basically your controller action should take an interface or an abstract class as constructor argument which will define the contract of all the operations that you are willing to perform. Then you should configure your favorite Dependency Injection tool to inject a specific implementation of this repository or data access contract into the controller. We are now saying that there is a weak coupling between your controller and the data access layer throughout this contract.
2

Is it a good idea to keep this kind of class static, or should I be instantiating it for each database call?

These are not your only two options.

The class should not be static: by making it static you relinquish a few important advantages of object-oriented programming, while gaining practically nothing.

Instead, an instance of it should be provided to your controllers via constructor-based dependency injection. Whether the class is re-instantiated for each request or whether you end up using a singleton is then determined by your DI-binding code. You can change it at the drop of a hat.

Here are a couple of advantages:

  1. Say you want to unit test the method that relies on this data-access class. If you are using an injected service interface rather than calling a static method directly, you can create a Mock object, tell it how to behave, and allow the method you're unit testing to think it's getting real data when in fact you're just feeding it the data you want to test.
  2. Say you end up wanting to cache the results of your data-access calls. You could inject a caching class that wraps the actual class, returning cached results where possible and invoking the real data-access methods when cached values aren't present. None of this would require any changes to your data-access class.

I could go on. Static classes kill your flexibility and have no practical advantage 99% of the time.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.