CODEWITHSUNDEEP
asp.netsql-server
Jui Test
Jui Test

Reputation: 2409

SQL Server connection in .net

Here is my code.

public Dataset ReturnDataset()
{
SqlConnection con = new SqlConnection(Proper connectionstring); 
SqlCommand cmd = new SqlCommand(spname,con); 
Dataset ds = new Dataset();
try
{
con.Open(); 
cmd.CommandType = CommandType.StoreProcedure; 
SqlDataAdapter da = new SqlDataAdapter(cmd); 
da.Fill(ds,table); 
return ds;
}
catch (TimeoutException Ex)
            {
                con.Close();
                if (Ex.Message.Contains("Timeout expired"))
                {
                    ds = null;
                    return ds;
                }
                else
                {
                    throw;
                }
            }
            catch (Exception)
            {
                throw;
            }

Does I need to write finally clause to close connection if error occur or not?In first try block I closed connection and then throw exception.Does I need to do same in second block?What will happen If already closed connection and trying to close once again?

Upvotes: 0

Views: 112

Answers (5)

SWeko
SWeko

Reputation: 30932

A better option is to use the using keyword, that does the closing / disposing for you

public Dataset ReturnDataset()
{
  using (SqlConnection con = new SqlConnection(connectionstring))
  using (SqlCommand cmd = new SqlCommand(spname,con))
  {
    Dataset ds = new Dataset();
    try
    {
      con.Open(); 
      cmd.CommandType = CommandType.StoreProcedure; 
      SqlDataAdapter da = new SqlDataAdapter(cmd); 
      da.Fill(ds,table); 
    }
    catch (TimeoutException Ex)
    {
      ds = null;
    }
    return ds;
  }
}

Upvotes: 1

Steve
Steve

Reputation: 216363

Why don't you use the Using Statement that ensures a proper closing and disposing of the disposable objects? I think you don't need to catch the exceptions at this level, just let them bubble up and, if needed take care of them at the upper level

Here you can simply

public Dataset ReturnDataset()
{
    Dataset ds = new Dataset();
    using(SqlConnection con = new SqlConnection(Proper connectionstring))
    using(SqlCommand cmd = new SqlCommand(spname,con))
    {
         con.Open(); 
         cmd.CommandType = CommandType.StoreProcedure; 
         using(SqlDataAdapter da = new SqlDataAdapter(cmd))
         {
             da.Fill(ds,table); 
         }
    }
    return ds;
}

If this code raises an exception the dataset will be never returned and being a local variable will be quickly become eligible for garbage collection

Upvotes: 1

marc_s
marc_s

Reputation: 755451

I would rewrite your code to be something like this:

public DataSet ReturnDataset()
{
    // initialize return value
    DataSet result = null;

    // put SqlConnection and SqlCommand into using () { ....}  blocks to ensure proper disposal    
    using (SqlConnection con = new SqlConnection(Proper connectionstring))
    using (SqlCommand cmd = new SqlCommand(spname, con))
    {
        result = new DataSet();

        try
        {
           con.Open(); 

           // you had a typo: it's a StoredProcedure - not a StoreProcedure
           cmd.CommandType = CommandType.StoredProcedure;   

           SqlDataAdapter da = new SqlDataAdapter(cmd); 
           da.Fill(result, table); 

           con.Close(); 
        }
        catch (TimeoutException Ex)
        {
            if (Ex.Message.Contains("Timeout expired"))
            {
                result = null;
            }
            else
            {
                throw;
            }
        }
    }

    // return the result - null if an error happened, a valid DataSet otherwise
    return result;
}

Points I improved:

  • declare the possible return value at the beginning, return only once at the very end
  • removed unnecessary "catch" on an exception you don't do anything with -> just let it happen
  • put SqlConnection and SqlCommand into using(...) { ... } blocks to ensure proper and speedy disposal

Upvotes: 1

Alberto De Caro
Alberto De Caro

Reputation: 5213

If not strictly necessary I avoid to create a structured catch cascade.

If necessary, I always try to define a catch cascade from the more specific exception to the more generic exception.

I always put any cleanup logic in the finally block:

SqlConnection con = new SqlConnection(Proper connectionstring); 
SqlCommand cmd = new SqlCommand(spname,con); 
Dataset ds = new Dataset();
try
{
  con.Open(); 
  cmd.CommandType = CommandType.StoreProcedure; 
  SqlDataAdapter da = new SqlDataAdapter(cmd); 
  da.Fill(ds,table); 
  return ds;
}
catch(TimeoutException toEx)
{
  //manage or log specific exception
}
catch(Exception ex)
{
  //manage or log generic exception
}
finally
{
  //cleanup
  con.Close();
  ds = null;
}

Upvotes: 1

Zaki
Zaki

Reputation: 5600

It is always good idea to close your connection on finally part of try catch as below, or use using statment and let .net to take care of it :

try{
}
catch{
}
finally{
conn.close();
}

or use using :

using (SqlConnection connection = new SqlConnection(connectionString)) 
{
}

You can close on error too. If you are worried about the state of connection before closing you can check if it is open and close it in finally:

if (conn != null && conn.State == ConnectionState.Open)
{
  conn.close();
}

Upvotes: 1

Related Questions