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?

Answer 1

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

Answer 2

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)) 
  {
    //....
  }
}