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\$

2 Answers 2

Reset to default
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\$
5
  • \$\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\$ Commented Oct 13, 2014 at 11:52
  • \$\begingroup\$ Based on the documentation it isn't needed. If the connection is closed, it will be opened \$\endgroup\$ Commented Oct 13, 2014 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\$ Commented Oct 13, 2014 at 12:10
  • \$\begingroup\$ See: http://programmers.stackexchange.com/a/142068 \$\endgroup\$ Commented Oct 13, 2014 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\$ Commented Oct 13, 2014 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\$
2
  • \$\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\$ Commented Oct 13, 2014 at 11:00
  • \$\begingroup\$ It looks like Heslacher answered to those questions already. I have nothing to add. \$\endgroup\$ Commented Oct 13, 2014 at 12:15

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.