2
\$\begingroup\$

I am using the below syntax to color code sales persons data. The issue that I have is that the datagridview returns roughly 12,000 rows so the coloring syntax takes some extensive time to execute.

Is there a way to optimize this code?

private void datagridview1_CellFormatting(object sender, DataGridViewCellFormattingEventArgs e)
{
    foreach (DataGridViewRow row in datagridview1.Rows)
    {
        if (row.Cells["Jake"].Value == System.DBNull.Value && row.Cells["Jones"].Value == System.DBNull.Value && row.Cells["Lati"].Value == System.DBNull.Value && row.Cells["Venitia"].Value == System.DBNull.Value)
        {
            row.DefaultCellStyle.BackColor = Color.Red;
        }
    }            
}
private void PopulateIT()
{
    datagridview1.Visible = true;
    lblNoData.Visible = false;

    string connString = @"Connection String";
    string query = @"SELECT * from tempdata";
    SqlConnection conn = new SqlConnection(connString);
    SqlCommand cmd = new SqlCommand(query, conn);
    conn.Open();
    SqlDataAdapter da = new SqlDataAdapter(cmd);
    da.Fill(formattedDT);
    conn.Close();
    da.Dispose();
    datagridview1.DataSource = formattedDT;
    datagridview1.AutoResizeColumns();
}
\$\endgroup\$
10
  • 1
    \$\begingroup\$ We need to see more of the code. What is filling up the DataGridView datagridview1 in the first place? \$\endgroup\$
    – Snowbody
    Dec 12, 2017 at 16:09
  • \$\begingroup\$ @Snowbody - see edit. This is how I am populating the datagridview \$\endgroup\$ Dec 12, 2017 at 16:12
  • 1
    \$\begingroup\$ I am surprised you don't need to define formattedDT \$\endgroup\$
    – paparazzo
    Dec 12, 2017 at 16:51
  • 1
    \$\begingroup\$ Cool but you should post code that will stand on its own \$\endgroup\$
    – paparazzo
    Dec 12, 2017 at 16:54
  • 2
    \$\begingroup\$ Are you sure as that is not what the documentation states? msdn.microsoft.com/en-us/library/… The CellFormatting event occurs every time each cell is painted, so you should avoid lengthy processing when handling this event. \$\endgroup\$
    – paparazzo
    Dec 12, 2017 at 17:00

2 Answers 2

5
\$\begingroup\$

SqlConnection, SqlCommand and SqlDataAdapter are implementing the IDisposable interface. Enclosing the usage of these objects inside a using statement ensures that the objects will always be proper disposed because the using statement just equals a try...finally where the objects would be disposed in the finally part.

The main problem regarding your performance issue is, that by calling the AutoResizeColumns() method the event will be triggered for each row and each column.
This results for each row and each column that you iterate over every row in the DataGridView.

So if we assume you have 12,000 rows with at least 5 columns .... you can do the math yourself.

The DataGridViewCellFormattingEventArgs has a property RowIndex which allows to access a single row so you should use this.

Summing this up in some code could look like so

private bool shouldFormattingBeDone = false;
private void PopulateIT()
{
    datagridview1.Visible = true;
    lblNoData.Visible = false;

    string connString = @"Connection String";
    string query = @"SELECT * from tempdata";

    using (SqlConnection conn = new SqlConnection(connString))
    using (SqlCommand cmd = new SqlCommand(query, conn))
    using (SqlDataAdapter da = new SqlDataAdapter(cmd))
    {
        conn.Open();
        da.Fill(formattedDT);
    }
    datagridview1.DataSource = formattedDT;

    shouldFormattingBeDone = false;
    datagridview1.AutoResizeColumns();
    shouldFormattingBeDone = true;
}


private void datagridview1_CellFormatting(object sender, DataGridViewCellFormattingEventArgs e)
{
    if (shouldFormattingBeDone == false) { return; }

    DataGridViewRow row = datagridview1.Rows[e.RowIndex];

    if (row.Cells["Jake"].Value == System.DBNull.Value && row.Cells["Jones"].Value == System.DBNull.Value && row.Cells["Lati"].Value == System.DBNull.Value && row.Cells["Venitia"].Value == System.DBNull.Value)
    {
        row.DefaultCellStyle.BackColor = Color.Red;
    }

}

This could be beautified by having an extension method which takes a DataGridViewCell as a parameter like so

public static class DataGridViewExtension
{
    public static bool IsDbNull(this DataGridViewCell cell)
    {
        return cell.Value == DBNull.Value;
    }
}  

resulting in the if condition looking like so

if (row.Cells["Jake"].IsDbNull() && row.Cells["Jones"].IsDbNull() && row.Cells["Lati"].IsDbNull() && row.Cells["Venitia"].IsDbNull())  

PS: I can't remember if the connection needs to be open when the constructor of the SqlDataAdapter is called.

\$\endgroup\$
4
  • \$\begingroup\$ I LOVE the extension class you created DataGridViewExtension - makes the code much more readable and cleaner! This cut the execution down to < 3 minutes, whereas before I was hitting 15 minutes and the execution was still not complete! \$\endgroup\$ Dec 12, 2017 at 18:31
  • \$\begingroup\$ In a comment OP claimed datagridview1_CellFormatting is only called once. \$\endgroup\$
    – paparazzo
    Dec 13, 2017 at 14:36
  • \$\begingroup\$ @Paparazzi which just isn't true \$\endgroup\$
    – Heslacher
    Dec 13, 2017 at 14:41
  • \$\begingroup\$ Agree but that is what the OP claimed. Good answer. \$\endgroup\$
    – paparazzo
    Dec 13, 2017 at 16:26
2
\$\begingroup\$

I don't like that repeated code pattern in the if...DbNull &&... . I think the field names should be stored in an array/list and then you could do something LINQy like

if (fieldNamesList.All(fieldName=>row.Cells[fieldName].IsDbNull()))

That way it would be easier to add and remove field names without having to make error prone copy and paste, also the line is shorter.

Also I hope that in your actual code, instead of this stripped-down version, the connection string is read from a config store instead of being hardcoded.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.