Is My DataTable Snippet Written Correctly?

   public static DataTable GetDataTable(SqlCommand sqlCmd)
    {
        DataTable tblMyTable = new DataTable();
        DataSet myDataSet = new DataSet();

        try
        {
            //1. Create connection
            mSqlConnection = new SqlConnection(mStrConnection);

            //2. Open connection
            mSqlConnection.Open();

            mSqlCommand = new SqlCommand();
            mSqlCommand = sqlCmd;

            //3. Assign Connection   
            mSqlCommand.Connection = mSqlConnection;开发者_如何学JAVA

            //4. Create/Set DataAdapter
            mSqlDataAdapter = new SqlDataAdapter();
            mSqlDataAdapter.SelectCommand = mSqlCommand;

            //5. Populate DataSet    
            mSqlDataAdapter.Fill(myDataSet, "DataSet");

            tblMyTable = myDataSet.Tables[0];
        }
        catch (Exception ex)
        {

        }
        finally
        {
            //6. Clear objects
            if ((mSqlDataAdapter != null))
            {
                mSqlDataAdapter.Dispose();
            }

            if ((mSqlCommand != null))
            {
                mSqlCommand.Dispose();
            }

            if ((mSqlConnection != null))
            {
                mSqlConnection.Close();
                mSqlConnection.Dispose();

            }
        }

        //7. Return DataSet
        return tblMyTable;
    }

• I use the above code to return records from database.

• The above snippet would run in web application which expected to have around 5000 visitors daily.

• The records returned reach 20,000 or over.

• The returned records are viewed (read-only) in paged GridView.

Would it be better to use DataReader instead of DataTable?

NOTE: two columns in the GridView are hyperlinked.

---

If you want to use a DataTable, then you should:

• always put your SqlConnection and SqlCommand objects into using(..) { ... } blocks to ensure proper disposal

• open your SqlConnection as late as possible and close it again right away

• and with the SqlDataAdapter, you don't even need to explicitly open/close your connection - the data adapter will do this for you

• don't use any unnecessary additional objects, like your DataSet - just fill the DataTable directly!

So your code should be:

public static DataTable GetDataTable(SqlCommand sqlCmd)
{
    DataTable tblMyTable = new DataTable();

    try
    {
        // Create connection
        using(SqlConnection mSqlConnection = new SqlConnection(mStrConnection))
        {
           // Assign Connection   
           sqlCmd.Connection = mSqlConnection;

           // Create/Set DataAdapter
          using(SqlDataAdapter mSqlDataAdapter = new SqlDataAdapter(sqlCmd))
          {
              mSqlDataAdapter.Fill(tblMyTable);
          }
        }
     }
    catch (Exception ex)
    {
       // handle exception
    }

    // Return DataTable
    return tblMyTable;
}

Like some of the other respondants have already mentioned:

• don't return 20'000 rows..... return just as many as your grid can show, anything more is a waste of time, space and processing power

• also: I don't particularly like the idea of passing in a SqlCommand - if I were to write this method, I'd pass in the SQL query as a string and create the SqlCommand only inside this method

A final recommendation: in this time and age, I would try to move away from the row/column based programming model - that's soooo 20th century.... you should definitely look into ORM - Object-Relational Mappers - that can turn your database rows/columns into programming-friendly objects and then use those to program against.

Check out NHibernate, Linq-to-SQL, Entity Framework v4 or others... having to still mess around with DataTable and DataRow is very low-level and not very productive!

---

Your snippet on the whole is not too bad, but a few notes.

1) You are ignoring any exceptions that occur, this may be just in your example here but if not that is very bad. You should log the errors.

2) You are using the horrible form of hungarian notation. You should get out of this habit for the benefit of other C# developers you may work with now or in the future.

3) Displaying 20K records to a user is madness. It will be slow to render and almost entirely useless as no human will stream through that many records. Provide a smarter UI, using search, filter or a combination of both.

As for the DataAdapter/DataReader question; it makes no difference really as a data adapter is using a data reader under the hood anyway. A better question might be "Should I use a DataSet or my own custom object to represent records?" and the answer would be that you should use a DataSet if the extra functionality it provides (such as maintaining the state of each record) is required. In your case where the records are just retrieved to display on a grid I would guess probably not.

---

Why you are returned 20,000 or over records from database. I think you should fetch records according to your GridView pagesize.

for example if your GridView page size is 10 then you can get only 10 records from db.

---

1-If your data is not user specific and some sort of static even if it is static for a few hours (which i think is the the case here), you can fetch the data and store in a cache and present this cached data to the user this way you can reduce the re-populating the data table for each user request.

2- If above point is not your case than read the data in chunks and use paging to reduce the the data returned because no user in going to see 20000+ rows in a go , use paging this will make your application respond faster.

Caching can be used in both the points above mentioned to increse through put of your application.