views:

1864

answers:

8

I have a Database class which contanins the follwing methods:

  • public bool ExecuteUDIQuery(string query) // UDI = Update Delete Insert
  • public bool ExecuteSelectQuery(string query)
  • public bool ExecuteSP(string sp, string[,] parms)
  • public int ExecuteSPReturnValue(string sp, string[,] parms)

The results of the methods are stored in private datasets or datatables. These objects are defined as getters.

There're about 10 classes which use the Database class. Every class creates an object of the class Database. Now i was thinking to make the Database class static. Is this a good idea? If so, why? Of not, why not?

A: 

If you are just executing queries against the DB, then yes make it static. You only have to create an instance if this object needs to keep some sort of state.

Martin Moser
A: 

If you have a static method you need to keep track of instances when you open and close the database.

So what you probably want to do is have a static Method called instance or current instance. And within you create a new instance of your db-class returning it in the static method.

Emil C
Keeping "instances" open is not a good idea. You should close the connection to the DB ASAP.
Inferis
Of course you should close the connection. But if you have a static class for accessing the commands you can manage the databaseconnections within. There is a command for closing reader after they are finished.
Emil C
+3  A: 

If I understand, the database class has some properties that store the result of the query? If so, you cannot make them static, since that is not thread-safe. If the result of a query is stored in these properties, what would happen if a second query would execute right after the first? It would be stored in the same static variable. The same goes for a web application: the result of another user browsing the site would change the results of the first user.

EDIT: To summarize, do NOT make the class static when you store the result of the queries in static variables, especially not when the class is used in a website, as the properties value will be shared amongst all visitors of your website. If 20 visitors do a query at the same time, visitor 1 will see the results of visitor 20's query.

Razzie
I am building a website. So if i understand you correctly, when there are say 20 visitors and they all request a different page (and my db class is static) it goes wrong, right?
Martijn
Yes, you understand correctly. If the class is static, the properties will be shared amongst the application. So, if 20 visitors do a database query, the property will hold the result of the last query, executed by visitor number 20.
Razzie
+4  A: 

In your specific example, I'd advise against making the class static: you're keeping state in the Database class, and by making the class static, that state will be shared amongst all classes using your Database. In your current setup, each Database instance keeps its own state, so there's no problem with Database calls interfering with each other.

If you'd refactor the Database class to return the datasets when doing a method call, you'd be fine making it static: there would be no stateful information left in the Database class.

But since this is not the case: no, don't make the class static.

Inferis
I agree with Inferis. Make a facade class which eases access to database classes could be an alternative.
Statement
+1  A: 

It depends on what kind of database or ORM that you're using. But in my experience it's seemed like a good idea but ended up shafting me. Here's how it did for me in LINQ-to-SQL:

I had a repository class that had a static variable to a data context. It worked at first, but when I had to make many more repository classes I ended up getting bugs as I hacked along. It turns out that data context in LINQ-to-SQL caches up all results and there is no way to refresh them. So if you added a post in a table in one context, it won't show up in other that cached that table. The solution was to remove the static modifier and let the repository create the context in the constructor. Since the repository classes were constructed as they were used, so would a fresh new data context.

You could argue that static variables leaves less footprint in memory, but the footprint for a data context is quite small to begin with and will be garbage collected in the end.

Spoike
+2  A: 

In addition to the others comments about thread safety there is also the issue of paralellization. In your case you won't be able to open several connections to the database at the same time and you won't be able to perform multiple paralell queries, even if thread safety of the results isn't an issue.

So I agree with the others, don't make a static class out of it.

Making the class static may be convenient, but creating new instances of it probably won't be an expensive operation so there probably isn't much to gain performance-wise either.

Edit:
I saw in a comment that you want to use your class on a web site. In that case you REALLY shouldn't do this. With a static database class you will only be able to safely serve one request at any time, and that is not what you want.

Rune Grimstad
A: 

Your methods good for static usage. I think, you have no trouble to convert them to static methods for now.

but later maybe you will need to manage transaction. leaving the transaction management to a class is saves lots of time I think. and this scenario is best fits with a nonstatic class.

Canavar
+1  A: 

Contrary to the answer post. I've built a webframework with a static database access it works great and gives great performance.

You can check out the source code at http://www.codeplex.com/Cubes

Emil C