views:

702

answers:

4
public class db
{
    public static string connectionString =
           WebConfigurationManager.ConnectionStrings["connectString"].ConnectionString;
    public static SqlConnection OpenConnection() 
    {
        SqlConnection connection = new SqlConnection(connectionString);
        connection.Open();
        return connection;
    }
}

I see code like this and it screams WRONG! It's for an ASP.NET (2.0). I understand it to be wrong.

For one you shouldn't open the SqlConnection and return it and two why would you make a static SqlConnection? Won't that create problems if multiple people are trying use it at the same time?

+7  A: 

What is static is OpenConnection() the Method which returns the connection. A new connection gets returned each time however (the assumption is that caller will be in charge of disposing of this connection object when appropriate).

In other words, the db class shown is not a singleton at all. The confusion may arise from the fact that one does not need to instantiate an instance of db in order to use its OpenConnection() method (since it is static), and the rest of the code may contain multiple snippets like

myConn = db.OpenConnection();
-- then do something with myConn
mjv
That said, a globally accessible static method for getting a SQL connection hardly seems like a good way of doing things.
ColinD
Agreed, ColinD, the validity of this "pattern" depends on the context in which it is used. On the one end it may make sense to provide a utility-type method to free the bulk of of the logic in a particular module to having to figure out the location of the connection string (and btw, also to have error handling etc...). On the other hand it may be more judicious to either share connections or to name the "db" class something more specific like "CustomerLeadsDb" or "OnlineBooksDatabase" etc.
mjv
+2  A: 

I'm not 100% sure what the question is, but to answer this

Won't that create problems if multiple people are trying use it at the same time?

no, there wouldn't be problems because each call to OpenConnection() constructs a new SqlConnection instance. That doesn't mean the code isn't garbage for other reasons. I at least hope calls to this method look something like this:

using(var conn = db.OpenConnection())
{
  // stuff
}
gWiz
Thanks, you're right. I copied and pasted the code, but I have seen things like this where the actual SqlConnection object is created as a static member or the class is declared static.
geekydevjoe
Yeah, a static SqlConnection object is pretty yucky. Hopefully it was 'private static', and/or all access to it was synchronized across threads (e.g. with a lock() block).
gWiz
A: 

You can't just say "Screams of being wrong" and then apply the term "Singleton" incorrectly. Why is a single static connection a poor approach? It you do not recreate it each time, then your entire application can share the connection. Do you need to create, open and close for each frigg'n sql call, that screams of stupid. Either way ADO should manage this.

vince
A: 

Connection objects must be scoped to the method that executes the transaction. Sharing the same connection object between threads will corrupt the connection pool. See http://social.msdn.microsoft.com/Forums/en-US/adodotnetdataproviders/thread/74a5db87-c63f-4386-b317-e848dedb2cd9

bbzippo