Take the 2-minute tour ×

I have 2 methods as below :

internal static SqlDataReader SelectData(string sql)
    using (var sqlConnection = new SqlConnection(Constant.ConnectionString))
        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();


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

share|improve this question
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. –  Tim Schmelter Aug 8 '13 at 14:12
@TimSchmelter .. Thats correct..I edited.. –  Tech Aug 8 '13 at 14:21
Why not use Enterprise Libraries for data access? –  T.S. Aug 8 '13 at 15:05
@T.S. - I'm surprised Enterprise Library is still maintained. It used to be the shiznit, but I haven't heard of it in a long time. –  Greg Aug 8 '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 8 '13 at 20:13

3 Answers 3

up vote 4 down vote accepted

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);

    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.

share|improve this answer
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 8 '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 8 '13 at 14:38
@Hawxby, I'm not sure I follow. –  Michael Perrenoud Aug 8 '13 at 14:39
I've submitted an edit. You can use using statements without braces between each one using() using() using () {} –  Hawxby Aug 8 '13 at 14:44
@Hawxby, nice little syntactic sugar. Thanks for the addition –  Michael Perrenoud Aug 8 '13 at 14:51

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

share|improve this answer
That means, the datareader is blank when I am accessing in AddData method?? –  Tech Aug 8 '13 at 14:15
@Ratan: not blank, It should throw an error as connection is not longer active. –  Claudio Redi Aug 8 '13 at 14:16
ok..point taken. –  Tech Aug 8 '13 at 14:19
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 8 '13 at 14:21

Technically, first method would be correct if you would do


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.

share|improve this answer

Your Answer


