2
\$\begingroup\$

I am quite new to C# and to the ASP.NET programming. When maintaining the legacy code, I have found rather terrible implementation of the method that is used on many places to get the DataTable object with the data filled by the SQL command.

Here is my first approach to rewrite it. Does using work with the return as shown below? Should the da and ds be released explicitly somehow? If ds is released, is the returned DataTable reference counted so that it will not be deleted?

public DataTable getdata2(string connection_string_id, string sqlcmd)
{
    string connstring = ConfigurationManager
        .ConnectionStrings[connection_string_id].ConnectionString;

    // Open the connection and return the first table.
    // A single one should be there, only.
    using (SqlConnection con = new SqlConnection(connstring))
    {
        SqlDataAdapter da = new SqlDataAdapter(sqlcmd, con);
        DataSet ds = new DataSet();
        da.Fill(ds);
        Debug.Assert(ds.Tables.Count == 1);
        return ds.Tables[0];
    }
}
\$\endgroup\$
4
\$\begingroup\$

Like janos already stated it is ok to return out of an using block.

Naming

Based on the naming guidelines, method names should be named using PascalCase casing. So getData2 should become GetData2 which should be still renamed to a more meaningful name.
Also input parameters should be named using camelCase casing, so connection_string_id should become connectionStringId.

If you are sure, that there will only be 1 table, you can also use the overloaded Fill() method which takes a DataTable as input parameter.
You could also stack the usings, but this is a matter of taste.

using (SqlConnection con = new SqlConnection(connstring))
using (SqlDataAdapter adapter = new SqlDataAdapter(sqlcmd, con))
{
    con.Open();
    DataTable dataTable = new DataTable();
    adapter.Fill(dataTable);
    return dataTable ;
}

In my opinion, your code does a little bit to much.

  1. Retrieving the ConnectionString
  2. Query the database

So let us add a method to retrieve the ConnectionString, which is then passed to the Getdata2() method.

private String GetConnectionString(String connectionStringId)
{
    return ConfigurationManager
        .ConnectionStrings[connectionStringId].ConnectionString;
}  

public DataTable GetData2(String connectionString, String sqlCommand)
{

    using (SqlConnection connection = new SqlConnection(connectionString))
    using (SqlDataAdapter adapter = new SqlDataAdapter(sqlCommand, connection))
    {
        con.Open();
        DataTable dataTable = new DataTable();
        adapter.Fill(dataTable);
        return dataTable ;
    }
}

To answer when to use using: use it if the related object implements the IDisposable interface.

\$\endgroup\$
  • \$\begingroup\$ Thanks. Is the con.Open() necessary or will it be done by the adapter anyway? Concerning the connectionStringId, the application actually uses only two connection strings. I was already thinking about making the connection object the class members and avoid the first argument at all. But I have to be slow with C# as I do not know it that much. \$\endgroup\$ – pepr Oct 13 '14 at 11:52
  • \$\begingroup\$ Based on the documentation it isn't needed. If the connection is closed, it will be opened \$\endgroup\$ – Heslacher Oct 13 '14 at 11:53
  • \$\begingroup\$ Thanks for the info about con.Open() -- I did not noticed that in the doc at first (IDbConnection). So basically, if I want to reuse the open connection, I should pass the open connection object. Otherwise, leaving the con.Open() out makes the code one line shorter. Am I right? \$\endgroup\$ – pepr Oct 13 '14 at 12:10
  • \$\begingroup\$ See: http://programmers.stackexchange.com/a/142068 \$\endgroup\$ – Heslacher Oct 13 '14 at 12:15
  • \$\begingroup\$ Thanks again! I am going to write a DbUtil class with the knowledge obtained here. Then I will post it for review and will place the link here. \$\endgroup\$ – pepr Oct 13 '14 at 12:54
3
\$\begingroup\$

Yes, it's ok to return from the middle of a using block. See this and this related discussions.

No need to release the DataSet, but you should wrap the SqlDataAdapter in using, something like this:

using (SqlConnection con = new SqlConnection(connstring))
{
    con.Open();
    using (SqlDataAdapter adapter = new SqlDataAdapter(sqlcmd, con))
    {
        DataSet ds = new DataSet();
        adapter.Fill(ds);
        Debug.Assert(ds.Tables.Count == 1);
        return ds.Tables[0];
    }
}
\$\endgroup\$
  • \$\begingroup\$ When the using should be used and when not? For example, here msdn.microsoft.com/en-us/library/bh8kx08z%28v=vs.110%29.aspx the using is used only for the connection and not for the adapter. What would happen to ds after the enclosing brace (say the return was not there)? Also, is the con.Open(); necessary? \$\endgroup\$ – pepr Oct 13 '14 at 11:00
  • \$\begingroup\$ It looks like Heslacher answered to those questions already. I have nothing to add. \$\endgroup\$ – janos Oct 13 '14 at 12:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.