Asp.Net: Returning a DataSet from a Class

I've decided to start another thread based on the responses I got in this thread: Asp.Net: Returning a Reader from a Class

I was returning a reader, but members have suggested I'd be better off returning a Dataset instead and also try to seperate the data access tier from the presentation tier.

This is what I have so far: //my class methods

   public DataSet GetSuppliers()
    {
        SqlConnection conn = new SqlConnection(connectionString);
        SqlCommand cmd = new SqlCommand("con_spSuppliersList", conn);
        cmd.CommandType = CommandType.StoredProcedure;
        cmd.Parameters.AddWithValue("@blogid", HttpContext.Current.Request.QueryString["p"]);

        return FillDataSet(cmd, "SuppliersList");
    }

    //my FillDataSet method

    private DataSet FillDataSet(SqlCommand cmd, string tableName)
    {
     SqlConnection conn = new SqlConnection(connectionString);

     cmd.Connection = conn;
     SqlDataAdapter adapter = new SqlDataAdapter(cmd);

     DataSet ds = new DataSet();
         try
         {
             conn.Open();
             adapter.Fill(ds, tableName);
         }
         finally
         {
             conn.Close();
         }
     return ds;

    }

// on my ascx page I call the method like so:

protected void Page_Load(object sender, EventArgs e)
{

    //instantiate our class开发者_开发百科
    MyClass DB = new MyClass();

    // grab the table of data
    DataTable dt = DB.GetSuppliers().Tables["SuppliersList"];

    //loop through the results
    foreach (DataRow row in dt.Rows)
    { 
        this.supplierslist.InnerHtml += Server.HtmlEncode(row["Address"].ToString()) + "<br/>";
        this.supplierslist.InnerHtml += "<b>Tel: </b>" + Server.HtmlEncode(row["Telephone"].ToString()) + "<p/>";

    }


}

}

Would anyone like to suggest improvements?

Is my loop 'data tier' or 'presentation tier', should the loop be inside the class and I just return a formatted string instaed of a dataset?

Thanks for all the great advice

I also would use a Typed DataSet or create your own class that holds the properties so you are not dealing with strings like row["Address"] you would say object.Address and get compile time checking.

DataSets have a lot of built in functionality that is nice but also caries with it a lot of overhead that might not be needed in something simple. Creating a simple class with properties and passing that out of your data access layer is probably the simplest way to implement what you want.

Something like this on the DAL (Data Access Layer) side:

//Also pass in the blogID dont have the DAL get the value from the UI layer..
    //make the UI layer pass it in.
    public IList<Supplier> GetSuppliers(string connectionString, int blogID)
    {
        IList<Supplier> suppliers = new List<Supplier>();
        //wrap with the using statements
        using (SqlConnection conn = new SqlConnection(connectionString))
        {
            using (SqlCommand cmd = new SqlCommand("con_spSuppliersList", conn))
            {
                cmd.CommandType = CommandType.StoredProcedure;
                cmd.Parameters.AddWithValue("@blogid", blogID);
                SqlDataReader reader = cmd.ExecuteReader();
                while (reader.Read())
                {
                    suppliers.Add(new Supplier 
                    { 
                        Address = reader.GetString(0),
                        Telephone = reader.GetString(1)
                    });
                }
            }
        }

        return suppliers;
    }
}

public class Supplier
{
    //I would have Address an object....but you have as string
    //public Address Address { get; set; }
    public string Address { get; set; }
    public string Telephone { get; set; }
}

//Example if you went with Address class...
    public class Address
    {
        //Whatever you want in the address
        public string StreetName { get; set; }
        public string Country { get; set; }
        public string Region { get; set; }
        public string City { get; set; }

}

One thing you should get in the habit of doing is calling Dispose() on your SqlConnection. The best pattern to do this is to use the using statement, which will automatically dispose of it for you. It looks like this:

private DataSet FillDataSet(SqlCommand cmd, string tableName)
{
     using(SqlConnection conn = new SqlConnection(connectionString))
     {
         cmd.Connection = conn;
         SqlDataAdapter adapter = new SqlDataAdapter(cmd);

         DataSet ds = new DataSet();
         try
         {
             conn.Open();
             adapter.Fill(ds, tableName);
         }
         finally
         {
             conn.Close();
         }
         return ds;
    }
}

What you have in the foreach loop in Page_Load, is presentation logic (layout), and IMO this should not be in the code-behind of your page, but in the markup.

I'd suggest that instead of using a foreach loop to construct the HTML output, you should use a databound control (such as a asp:Repeater, DataList or GridView). Then you can bind the repeater to your dataset or datatable and have all the markup where it belongs (in the ASCX file). See this page for an example.

As a general note: you can find lots of tutorials on www.asp.net, e.g. about data access: http://www.asp.net/%28S%28pdfrohu0ajmwt445fanvj2r3%29%29/learn/data-access/