How do I properly use a SqlDataReader?

Question

I have 2 methods as below :

internal static SqlDataReader SelectData(string sql)
{
    using (var sqlConnection = new SqlConnection(Constant.ConnectionString))
    {
        sqlConnection.Open();
        var sqlCommand = new SqlCommand(sql, sqlConnection);
        var dataReader = sqlCommand.ExecuteReader();
        return dataReader;
    }
}

============

And using this method as :

var dataReader = SelectData(---some sql ---);

private void AddData(dataReader)
{
    while (dataReader.Read())
    {
        Employee e = new Employee();
        e.FirstNamei = dataReader["Name"].ToString();
    }

    dataReader.Close();
}

I know we can merge this two method, but I am looking at better way write this, OR this can cause some problem??

You mention that you are using the first method in the second but that is not true. Both methods seem to be unrelated to each other, so your question is still unclear. Btw, [**never** use empty `catch`-blocks](http://stackoverflow.com/a/1234364/284240). — Tim Schmelter, Aug 08 '13 at 14:12
@T.S. - I'm surprised [Enterprise Library](http://msdn.microsoft.com/en-us/library/ff648951.aspx) is still maintained. It used to be the shiznit, but I haven't heard of it in a long time. — Greg, Aug 08 '13 at 20:09
Not just maintained. It had new version recently. It works faster than EF and has caching capabilities. I think, it worth implementing than create connections on every page. — T.S., Aug 08 '13 at 20:13

score 6 · Accepted Answer · edited Aug 08 '13 at 14:45

6

Actually you are in fact leaving yourself open a bit. You really want to write it like this:

using (SqlConnection cnn = new SqlConnection(cnnString))
using (SqlCommand cmd = new SqlCommand(sql, cnn))
{
    // use parameters in your SQL statement too, so you can do this
    // and protect yourself from SQL injection, so for example
    // SELECT * FROM table WHERE field1 = @parm1
    cmd.Parameters.AddWithValue("@parm1", val1);

    cnn.Open();
    using (SqlDataReader r = cmd.ExecuteReader())
    {

    }
}

because you need to make sure these objects get disposed. Further, by going this direction you don't need dataReader.Close(). It will get called when it gets automatically disposed by the using statement.

Now, wrap that collection of statements inside a try...catch and you're in business.

edited Aug 08 '13 at 14:45

Hawxby

2,746
21
29

answered Aug 08 '13 at 14:10

Mike Perrenoud

66,820
29
157
232

I was just trying to refactor it..so that I can use a method like `SelectData` everywhere..What you gave is a great working piece of code.. – Tech Aug 08 '13 at 14:19
For the sake of clarity the first set of braces is unnecessary. using(SqlConnection cnn = n.....) using(SqlCommand cmd = n.....) { // Code } Just saves on so much nesting. Using statements can go back to back – Hawxby Aug 08 '13 at 14:38
@Hawxby, I'm not sure I follow. – Mike Perrenoud Aug 08 '13 at 14:39
I've submitted an edit. You can use using statements without braces between each one using() using() using () {} – Hawxby Aug 08 '13 at 14:44
@Hawxby, nice little syntactic sugar. Thanks for the addition – Mike Perrenoud Aug 08 '13 at 14:51

Claudio Redi · Answer 2 · 2013-08-08T14:15:30.463

2

A couple of things

1) Since you're closing your connection on SelectData, dataReader should blow up on AddData as it requires an open connection

2) AddData shouldn't close dataReader as he didn't open it.

3) Maybe you're hiding some code but I don't see that you use Employee instance created on AddData

edited Aug 08 '13 at 14:15

answered Aug 08 '13 at 14:10

Claudio Redi

67,454
15
130
155

That means, the datareader is blank when I am accessing in AddData method?? – Tech Aug 08 '13 at 14:15
@Ratan: not blank, It should throw an error as connection is not longer active. – Claudio Redi Aug 08 '13 at 14:16
IS there some place, where I can see some ideal way to write such type of methods..Because for each type of `Employee e = new Employee();` I need to write the whole thing again and again.. – Tech Aug 08 '13 at 14:21

score 0 · Answer 3 · answered Aug 08 '13 at 14:30

Technically, first method would be correct if you would do

    sqlCommand.ExecuteReader(CommandBehavior.CloseConnection);

Then, your client would close the reader and your connection will be closed as well.

Second example would also be correct if you didn't close the reader inside of it. There is no criminal in passing the reader to a method just to iterate it. but it has to be controlled from where it was created. How you open and dispose of it - this is different question.

How do I properly use a SqlDataReader?

3 Answers3