Asp.Net: Returning a Reader from a Class

Question

Hi all,

I was just wondering about the correct way to return a reader from a class?

My code below works, but I'm unsure if this is correct.

Also. I can't close the the connection in my class method and still access it from my ascx page, is

that OK?

// In my class I have the following method to return the record/reader -- it's a single record in this case.

public SqlDataReader GetPost()
    {
        SqlConnection conn = new SqlConnection(connectionString);
        SqlCommand cmd = new SqlCommand("con_spPost", conn);
        cmd.CommandType = CommandType.StoredProcedure;
        cmd.Parameters.AddWithValue("@blogid", blogid);
        try
        {
            conn.Open();
            return cmd.ExecuteReader();
        }
        finally
        {
          //  conn.Close();
        }
    }

//I then call the GetPost method in my ascx page like so:

protected void Page_Load(object sender, EventArgs e)
{

    //instantiate our class
    MyClass DB = new MyClass();

    //pass in the id of the post we want to view
    DB.PostID = Int32.Parse(Request.QueryString["p"]);

    ///call our GetPost method
    SqlDataReader reader = DB.GetPost();

   //output the result
    reader.Read();
    this.viewpost.InnerHtml = "<span id='post1_CreatedDate'>" + reader["CreatedDate"].ToString() + "</span><br>";
    this.viewpost.InnerHtml += "<span class='blogheads'>" + reader["BlogTitle"].ToString() + "</span><p><p>";
    this.viewpost.InnerHtml += reader["BlogText"].ToString();
    reader.Close();
}

I'd appreciate any comments on my code or tips, thanks.

Melt

Answer 1

Generally speaking it's fine to return a reader from a method, but the reader's consumer needs to take control of all the disposable objects that will be used during the reader's lifetime.

To do this, you'd pass an IDbConnection into the GetPost method, then make sure your caller disposes both the connection and reader. The using keyword is the most convenient way to do this:

protected void Page_Load(object sender, EventArgs e) {

    // Create the DB, get the id, etc.    

    using (IDbConnection connection = new SqlConnection(connectionString))
    using (IDataReader reader = DB.GetPost(connection)) {
        reader.Read();
        this.viewpost.InnerHtml = reader["BlogText"].ToString();
        // finishing doing stuff with the reader  
    }
}

As others have pointed out, this starts to clutter your application's presentation layer with way too much data-access infrastructure - so it isn't appropriate here. Until you find yourself with a performance problem or need to display an unreasonable amount of data, you shouldn't be handling data readers in your presentation layer. Just make DB.GetPost return a string, and encapsulate all the data-access code in there.

Answer 2

Why are you having the web page know about the database at all? Why not abstract away the database knowledge and just return a list or an object with the data from the database? Just seems like a lot of responsibility mixing, and you could make it easier on yourself.

Answer 3

To make sure that the connection is closed, replace the ExecuteReader call with the following:

return cmd.ExecuteReader(CommandBehavior.CloseConnection);

You should also remove te try / finally block.

Also, in your Page_Load handler, you should use a using statement, like this:

using (SqlDataReader reader = DB.GetPost()) {

    //output the result
    reader.Read();
    this.viewpost.InnerHtml = "<span id='post1_CreatedDate'>" + reader["CreatedDate"].ToString() + "</span><br>"
        + "<span class='blogheads'>" + reader["BlogTitle"].ToString() + "</span><p><p>"
        +  reader["BlogText"].ToString();
}

Also, you should check that the SQL query actually returned something, like this:

if (!reader.Read()) {
    Something's wrong
}

Finally, and most important by far, you should escape your HTML to prevent XSS holes by calling Server.HtmlEncode.

For example:

    this.viewpost.InnerHtml = "<span id='post1_CreatedDate'>" + reader["CreatedDate"].ToString() + "</span><br>"
        + "<span class='blogheads'>" + Server.HtmlEncode(reader["BlogTitle"].ToString()) + "</span><p><p>"
        + Server.HtmlEncode(reader["BlogText"].ToString());

Answer 4

There is a problem. Your connection is not being closed. As you know you can't close it within your GetPost because then you won't have the data any more, due to the nature of the DataReader. One way to address this is to include a parameter in your ExecuteReader method like this:

cmd.ExecuteReader(CommandBehavior.CloseConnection)

Then, when your reader is closed, the connection will close.

There is a fundamental problem with having a datareader returned by encapsulated code, in that the connection has to be open through all this, making error handling tricky. Consider (A) using a datatable instead, which is almost as efficient for small sets of data. This way you can close your connection right away in you GetPost method and not worry about it, with very simple error handling. Or (B) Pass the connection into GetPost, so all the Using/Dispose syntax and error handling for the connection is explicit in one place. I would suggest option A.

Answer 5

You really shouldn't be mixing data access with the presentation layer.

Consider returning a typed DataSet, or building business objects and returning those to your control.

Here is a tutorial: http://www.asp.net/learn/data-access/tutorial-01-cs.aspx

Answer 6

This is a very simple architecture. As CSharpAtl has suggested, you could make it more sophisticated. However, this seems to work for you.

One important addition I would make would be to use try-finally blocks. Putting the Close in the finally will ensure that the connection is released even if an exception occurs during processing.

SqlDataReader reader;
try
{
///call our GetPost method
    reader = DB.GetPost();

   //output the result
    reader.Read();
    this.viewpost.InnerHtml = "<span id='post1_CreatedDate'>" + reader["CreatedDate"].ToString() + "</span><br>";
    this.viewpost.InnerHtml += "<span class='blogheads'>" + reader["BlogTitle"].ToString() + "</span><p><p>";
    this.viewpost.InnerHtml += reader["BlogText"].ToString();
}
finally
{
    reader.Close();
}

Answer 7

Thanks for all the great tips, I've decided to continue this thread on a slightly different but related topic here: http://stackoverflow.com/questions/1980887/asp-net-returning-a-dataset-from-a-class

Regards Melt

Answer 8

This article by Dan Whalin might be a good resource for you to read through. It shows the basics of creating an n-layered application. You create a data access component, an entity object, a business layer and a presentation layer. He also uses sql data readers like you're asking about, and he shows a nice way of having a object build helper method.

If you don't like reading article, he also has a pretty good video on the same subject and a code example which you can download and see the different variations of this method of creating data driven applications.

Good luck and hope this helps some.