Database connection approach

Question

We have recently started working in ASP.NET and starting a simple CRUD Web application in ASP.Net. Here is the approach we are following to connect to DB.

Connection.cs

public class Connection
{
    public SqlConnection con = new SqlConnection(ConfigurationManager.ConnectionStrings["CWConnectionString"].ConnectionString);
    private readonly log4net.ILog logger = log4net.LogManager.GetLogger("Connection.cs");

    public Connection()
    {
        //
        // TODO: Add constructor logic here
        //
    }

    public bool CheckOpenConnection()
    {
        try
        {
            if (con.State == ConnectionState.Open)
            {
                con.Close();
                return true;
            }
            else
            { return false; }
        }
        catch (Exception Ex)
        {
            logger.Error("Error in CheckOpenConnection : " + Ex.Message);
            return false;
        }
    }

    public void OpenConnection()
    {
        try
        {
            if (CheckOpenConnection() == false)
                con.Open();
        }
        catch (Exception Ex)
        {
            logger.Error("Error in OpenConnection : " + Ex.Message);            
        }
    }

    public void CloseConnection()
    {
        try
        {
            if (CheckOpenConnection() == true)
                con.Open();
        }
        catch (Exception Ex)
        {
            logger.Error("Error in CloseConnection : " + Ex.Message);
        }
    }
}

Then we use some methods like this in another cs file

Dal_authentication.cs

public DataTable AuthenticateUser(string UserName, string Password)
{
    try
    {

        DataTable results = new DataTable();
        Connection cn = new Connection();
        cn.OpenConnection();
        string sql_SP = "uspAuthenticateLogin";

        using (SqlCommand cmd = new SqlCommand(sql_SP, cn.con))
        {
            cmd.CommandType = CommandType.StoredProcedure;

            cmd.Parameters.Add(new SqlParameter("@UserName", SqlDbType.NVarChar, 50));
            cmd.Parameters["@UserName"].Value = UserName;

            cmd.Parameters.Add(new SqlParameter("@Password", SqlDbType.NVarChar, 50));
            cmd.Parameters["@Password"].Value = Password;


            SqlDataAdapter da = new SqlDataAdapter(cmd);
            da.Fill(results);
        }
        cn.CloseConnection();

        return results;
    }
    catch (Exception Ex)
    {
        //logger.Error("AuthenticateUser : " + Ex.Message);
        return null;
    }
}

Is there any flaw in this approach? Is it the right way to connect to DB? Are there any possible improvements?

Panagiotis Kanavos · Answer 1 · 2014-05-13 11:32:18Z

up vote 5 down vote

As @chrfin mentioned, this type of question is best suited for CodeReview.

This is the wrong way to use connections. Keeping a connection open indefinetely is bad because it accumulates locks, transactions remain open,it wastes server resources due to blocking and forces you to use more connections than you actually need.

ADO.NET (and ADO before it, way back to the late 90s) support connection pooling so that whenever you close a connection, it terminates any existing transactions, frees up all locks and remains dormant until the next time you need a connection with the exact same string. This way, a couple of connections can handle commands from multiple classes.

The difference in speed and scalability can be orders of magnitude because you drastically reduce blocking issues:

The proper way to connect is something like this:

using(var con=new SqlConnection(myConnectionString))
using(var cmd=new SqlCommand(myCommand,con))
{
    cmd.Parameters.Add ...
    ...
    con.Open();

    SqlDataAdapter da = new SqlDataAdapter(cmd);
    da.Fill(results);

}

You don't need to close the connection explicitly because it's closed automatically when execution exits the using block, even if an exception occurs.

A better solution would be to use an ORM like Entity Framework or NHibernate to handle connections and map the results to objects.

You can find numerous tutorials on using Entity Framework with ASP.NET MVC at Microsoft's ASP.NET site

answered May 13 at 11:32

Panagiotis Kanavos
1962

It looks pretty good. So we can put the connection string in some global constant in this case right? – Akash_s May 13 at 11:37

It sounds like you need a full ADO.NET tutorial. This can't be posted on a Q&A site like SO. – Panagiotis Kanavos May 13 at 11:39

Just fyi, the recommended place for your connection string would be in the web.config file under the section <connectionStrings/> – mdisibio May 13 at 15:54

@PanagiotisKanavos Your answer is well written, correct and concise, but imho I think the ORM recommendation clouds the issue, which is simply a question about correct connection management in a simple ado.net method. – mdisibio May 13 at 15:58

@mdisibio You'll note that the original code does read the string from web.config but the author still asks about where to put it. The real question is more like "how does data access work in ASP.NET", and all modern tutorials use ORMs. – Panagiotis Kanavos May 14 at 7:04

add comment

pbjork · Answer 2 · 2014-05-13 11:32:07Z

If you run CheckOpenConnection() on a connection that is already open, you will get an error with this code.

Your code closes the connection if it is open, and then return true - as if it was open. This will result in an error. Don't close the connection if your method called CheckOpenConnection(), it's confusing and hard to maintain in the long run!

public bool CheckOpenConnection()
{
    try
    {
        if (con.State == ConnectionState.Open)
        {
            //con.Close(); don't close!
            return true;
        }
        else
        { return false; }
    }
    catch (Exception Ex)
    {
        logger.Error("Error in CheckOpenConnection : " + Ex.Message);
        return false;
    }
}

public void OpenConnection()
{
    try
    {
        if (CheckOpenConnection() == false)
            con.Open();
    }
    catch (Exception Ex)
    {
        logger.Error("Error in OpenConnection : " + Ex.Message);            
    }

}

Consider using a using statement for the connection as well, i.e.

using(SqlConnection con = new SqlConnection(connectionString))
{
  con.Open();
  using(SqlCommand cmd = new SqlCommand(query, con)) 
  {
    //....
  }
}

Better yet, just don't use the custom Connection class, ADO.NET already provides support for proper reuse and disposal — Panagiotis Kanavos, May 13 at 11:33

asked	1 month ago
viewed	44 times
active	1 month ago

current community

your communities

more stack exchange communities

Database connection approach

migrated from stackoverflow.com May 15 at 15:35

2 Answers

Your Answer

Hot Network Questions

current community

your communities

more stack exchange communities

Database connection approach

migrated from stackoverflow.com May 15 at 15:35

2 Answers

Your Answer

Sign up or log in

Post as a guest

Related

Hot Network Questions