0

This is my class:

public class DbHelper

{

public static DataTable Select(string query, object[,] parameters)
{
    DataTable dt = new DataTable();
    SqlConnection cn = new SqlConnection(ConfigurationManager.ConnectionStrings["DBConnection"].ToString());
    SqlCommand com = new SqlCommand();
    com.Connection = cn;
    com.CommandText = query;
    for (int i = 0; i < parameters.Length/2; i++)
        com.Parameters.AddWithValue(parameters[i, 0].ToString(), parameters[i, 1]);
    try
    {
        cn.Open();
        dt.Load(com.ExecuteReader());
    }
    catch
    {
    }
    finally
    {
        com.Dispose();
        cn.Close();
        cn.Dispose();
    }
    return dt;
}

}

I am using this class for select from data base, Like this:

    public static string NewsListTitle(int count, int categoryId, string domainName)
{
    StringBuilder cnt = new StringBuilder();
    DataTable dt = DbHelper.Select("SELECT top " + count + " ..... ");
    foreach (DataRow i in dt.Rows)
    {
        cnt.Append("<li><a target=\"_blank\" href=\"http://" + domainName + "/news/" + i["NewsID"] + "\"\">" + i["Title"] + "</a></li>");
    }
    return cnt.ToString();
}

the DataTable type is a powerful way to store data in memory. i thing this is best way to select from data base, but my problem is DateTable memory usage! is there any wrong here or is there any better way?

Thank you

hossein
  • 99
  • 3
  • 11
  • You should really enable code analysis and then listen to the warnings it puts out. `using` blocks would make your code more readable and probably even solve your problem. And don't swallow exceptions please. – nvoigt Aug 24 '14 at 07:45
  • Based on the simplicity of what you're doing with the data, you might consider a more lightweight approach of just hydrating an IEnumerable of a custom object that holds your data. DataTable is a very versatile object, but it will come with the memory expense since it has many features. This expense would be justified if you used these features, but it doesn't appear this is the case. – Daniel Graham Aug 24 '14 at 07:48
  • A part from the consuption of memory I will be more concerned with the double loop present in your code. First you create a DataReader to loop over the resultset returned by your query and fill a DataTable then you loop over the DataRows to create the HTML required by your code and just discard the expensive LOCAL DataTable. Why don't you change your code to use directly the DataReader to create the HTML? (In ANY case you should avoid this kind of generic methods. Use a small ORM like Dapper) – Steve Aug 24 '14 at 08:05

2 Answers2

0

the DataTable type is a powerful way to store data in memory.

if with powerfull you mean memory bloated and slow but very flexible - which in 90% of the cases is absolutely not needed.... then yes.

i thing this is best way to select from data base

You are totally right. If you write a report generator where you do not know the structure of the data while programming. In any other case (which is like 95% of the cases) that is a nice "go, work for the competition" reason to get you fired right there.

but my problem is DateTable memory usage!

Yes. Because your termendous powerfull generic mechanism is known to be a memory and performance hog. How good is it now?

Use an ORM to get an enum of objects (that is real objects, not a generic stat store) out of the SQL statement. Anything goes - heck, ever heard of Entity Framework?

Then never materialize the data in memory.

On top learn the absolute basic of programming with data: ask for as little data as possible. For example, if you think you ahve a problem now - the browser will really love a list of 100 million entries. Best to use paging, cut down memory consumption, by never asking for more than maybe a hundred or two hundred rows.

And please learn the basics of working with daa in 2014 - datasets where a bad architecture when MS started pushing them out many years ago and not a lot has changed. The world has moved forward.

TomTom
  • 61,059
  • 10
  • 88
  • 148
0

As I have said in my comment above. I will be more concerned with the double loop implicit in your code. An elegant solution to both problems is the IEnumerable approach using a DataReader

In you DBHelper class add this

public static IEnumerable<IDataReader> Reader(string query, object[,] parameters)
{
    using(SqlConnection cn = new SqlConnection(.....))
    using(SqlCommand com = new SqlCommand(query, cn))
    {
        for (int i = 0; i < parameters.Length/2; i++)
            com.Parameters.AddWithValue(parameters[i, 0].ToString(), parameters[i, 1]);

        cn.Open();
        using(SqlDataReader r = com.ExecuteReader())
        {
            while(r.Read())
                yield return r;
        }
    }
}

Now you could create your output without creating a DataTable and looping only one time over the resultset of your query.

public static string NewsListTitle(int count, int categoryId, string domainName)
{
    StringBuilder cnt = new StringBuilder();
    foreach(IDataRecord rc in DbHelper.Reader("......", ....);
    {
         cnt.Append(@"<li><a target=\"_blank\" href=\"http://" + domainName + "/news/" +
               rc.GetInt32(rc.GetOrdinal("NewsID")).ToString() + "\"\">" + 
               rc.GetString(rc.GetOrdinal("Title")) + "</a></li>");
    }
    return cnt.ToString();
}
Steve
  • 213,761
  • 22
  • 232
  • 286