SqlDataAdapter with using keyword

Question

Is this the following code healthy? Or I don't need to use the using keyword as the SqlDataAdapter will handle closing the connection?

public static DataSet Fetch(string sp, SqlParameter [] prm)
{
    using (SqlConnection con = new SqlConnection(ConStr))
    {
        using (SqlCommand cmd = con.CreateCommand())
        {
            cmd.CommandType = CommandType.StoredProcedure;
            cmd.CommandText = sp;
            cmd.Parameters.AddRange(prm);

            using (SqlDataAdapter dta = new SqlDataAdapter(cmd))
            {
                DataSet dst = new DataSet();
                dta.Fill(dst);

                return dst;
            }
        }
    }
}

---

---

@MarkGravell I need a suggestions here, I am really looking to use DataReader, but I was looking all the time to use the using keyword to ensure closing the connections. Where with DataReader we can not use it because it will close the connection if we want to return the DataReader back to some method. So do you think the following technique is fine with DataReader and the using keyword:

public static SqlDataReader Fetch(string sp, SqlParameter [] prm)
{
    SqlCommand cmd = new SqlConnection(ConStr).CreateCommand();
    cmd.CommandType = CommandType.StoredProcedure;
    cmd.CommandText = sp;
    cmd.Parameters.AddRange(prm);
    cmd.Connection.Open();

    return cmd.ExecuteReader(CommandBehavior.CloseConnection);
}

using (SqlDataReader dtrPrize = Sql.Fetch("SelectPrize", new SqlParameter[] { new SqlParameter("id", id) }))
{
    dtrPrize.Read();

    Prize prize = new Prize();
    prize.id = (int)dtrPrize[dtrPrize.GetOrdinal("id")];
    prize.artitle = (string)dtrPrize[dtrPrize.GetOrdinal("artitle")];
    prize.entitle = (string)dtrPrize[dtrPrize.GetOrdinal("entitle")];
    prize.ardetail = (string)dtrPrize[dtrPrize.GetOrdinal("ardetail")];
    prize.endetail = (string)dtrPrize[dtrPrize.GetOrdinal("endetail")];
    prize.image = (string)dtrPrize[dtrPrize.GetOrdinal("image")];
    prize.theme = (string)dtrPrize[dtrPrize.GetOrdinal("theme")];
    prize.price = (int)dtrPrize[dtrPrize.GetOrdinal("price")];
    prize.audience = (int)dtrPrize[dtrPrize.GetOrdinal("audience")];
    prize.type = (byte)dtrPrize[dtrPrize.GetOrdinal("type")];
    prize.status = (byte)dtrPrize[dtrPrize.GetOrdinal("status")];
    prize.voucher = (string)dtrPrize[dtrPrize.GetOrdinal("voucher")];
    prize.supplierid = (int)dtrPrize[dtrPrize.GetOrdinal("supplierid")];
    prize.created = (DateTime)dtrPrize[dtrPrize.GetOrdinal("created")];
    prize.updated = (DateTime)dtrPrize[dtrPrize.GetOrdinal("updated")];

    return prize;
}

Plosive

The code is fine. The Connection will be opened/closed implicitely in DataAdapter.Fill. – Venetis

Answer 1

Healthy-ish; personally I'd say the unhealthy bit is the bit where it makes use of DataSet and DataAdapter, but that is perhaps just my personal bias.

Yes, you should dispose the adapter etc here (which is what the using does for you, obviously).

As a trivial pointless tidy, you can stack the usings - just makes it a little less verbose:

using (SqlConnection con = new SqlConnection(ConStr))
using (SqlCommand cmd = con.CreateCommand())
{

Poorly

@MarkGravell Could you explaine, why the adapter should be disposed also? Is SqlConnection dispose not enought? – Jocund

@voo Because it implements IDisposable, and we're done with it. That is reason enough. Anything beyond that is going into implementation details, which we should avoid. As a consumer, all it needs to come down to is: "does it implement IDisposable? am I done with it?" – Poorly

@MarkGravell as you mentioned the DataAdapter and DataSet might be unhealthy, do you mean its better to use DataReader, map it to an object, close the connection and finally return the model object rather than returning the DataSet? Whats your recommendations over here? – Plosive

@user2155873 I do indeed strongly lead towards a regular object model here (rather than DataSet). How you interact with that is up to you: there are a myriad of ORM tools available. Personally I use a lot of "dapper", but that might just be personal bias. – Poorly

Answer 2

It will be enough to leave just the first using (the one on the Connection) because disposing the connection will dispose everything you need disposed.

However, there is no harm disposing everything, just a bit more code.

Kylix