2
\$\begingroup\$

The following code is very slow. I was wondering if you can help me speed it up.

private void filter(List<CheckBox> boxList)
        {
            // refLanguageTabs is a TabControl the Tabcount is 9
            for (int i = 0; i < refLanguageTabs.TabCount; i++)
            {
                Control[] ctrls = refLanguageTabs.TabPages[i].Controls.Find(refLanguageTabs.TabPages[i].Name + "Grid", true);
                DataGridView dgv = ctrls[0] as DataGridView;

                // average row count is 3000 
                for (int j = 0; j < dgv.RowCount; j++)
                {
                    for (int k = 0; k < boxList.Count; k++)
                    {
                        if (dgv.Rows[j].Cells[1].Value.ToString() != boxList[k].Name.ToString())
                        {
                            dgv.Rows[j].Visible = false;
                        }
                    }
                }
                dgv.Refresh();
            }
        }
\$\endgroup\$
1
  • \$\begingroup\$ Only state the code purpose in the title, what does it do specifically? \$\endgroup\$
    – Caridorc
    Commented May 4, 2015 at 19:01

3 Answers 3

Reset to default
5
\$\begingroup\$

Some few thoughts without testing or anything else:

  • Cache the value of the cell in a local variable.
  • Replace for-loops if possible with foreach-loops.
  • The call of boxList[x].Name.ToString() is unnecessary, it is already a String.
  • break after setting the visibility to avoid unnecessary checks.
foreach(DataGridViewRow row in dgv.Rows)
{
    String cellValue = row.Cells[1].Value.ToString();
    foreach(CheckBox boxItem in boxList)
    {
        if(cellValue == boxItem.Name)
        {
            row.Visible = false;
            break;
        }
    }
}

On another sidenote, please use meaningful variable names, even in such stupid iterating loops.

\$\endgroup\$
8
  • \$\begingroup\$ Is there a speed difference between for and foreach? \$\endgroup\$
    – I am not smart
    Commented Dec 19, 2011 at 16:09
  • 1
    \$\begingroup\$ Yes, there can be. With for() you have to use indexed arrays, and when you use indexed arrays the compiler has to emit code to check that your indexes are within the bounds of your array all the time. With foreach() you do not use indexed arrays, so you save those checks. \$\endgroup\$
    – Mike Nakis
    Commented Dec 19, 2011 at 16:38
  • 1
    \$\begingroup\$ @PoiXen: The main reason is readability. \$\endgroup\$
    – Bobby
    Commented Dec 20, 2011 at 8:17
  • 2
    \$\begingroup\$ @MikeNakis - my understanding is that for() is faster than foreach() despite the index bound checking. If you perform a Google search "for vs foreach performance" you would find that a lot of people would disagree with your statement. \$\endgroup\$
    – Marko
    Commented Aug 27, 2013 at 6:02
  • 1
    \$\begingroup\$ @Marko I did the Google search that you suggested, and what I saw is that the answer to the question is still highly debatable, though most indications appear to suggest that for() will usually be somewhat faster, indeed. So, I stand corrected. But in any case, whatever performance difference there is, it will be negligible as far as the question at hand is concerned. The better readability of the foreach() loop is almost always worth the few extra clock cycles. \$\endgroup\$
    – Mike Nakis
    Commented Aug 27, 2013 at 9:32
1
\$\begingroup\$

My recommendation would be to first make one pass through your cells and populate a dictionary in which the key is the name and the value is the cell itself. Then make a single pass through the checkboxes, and for each checkbox use its name to lookup the cell in the dictionary and there you have it.

\$\endgroup\$
11
  • \$\begingroup\$ Whoever upvoted this, thank you. It is a pity how the best answer is some times overlooked! C-:= \$\endgroup\$
    – Mike Nakis
    Commented Dec 20, 2011 at 20:47
  • \$\begingroup\$ What name do you mean to use for the key? \$\endgroup\$
    – Bobby
    Commented Dec 22, 2011 at 22:15
  • \$\begingroup\$ Hmmmm, I meant the value of the cell. As in MyDictionary[row.Cells[1].Value] = row.Cells[1] \$\endgroup\$
    – Mike Nakis
    Commented Dec 22, 2011 at 22:23
  • \$\begingroup\$ And what if there are multiple cells with the same value? \$\endgroup\$
    – Bobby
    Commented Dec 22, 2011 at 22:25
  • \$\begingroup\$ I did not know that was a possibility, but I suppose I should have thought of it. So, in that case, you can reverse it, assuming that you do not have two checkboxes with the same name. (Which sounds like a reasonable assumption to make.) First enumerate the checkboxes and populate a dictionary mapping checkbox names to checkboxes. Then, enumerate your rows, and look up the corresponding checkbox using the value of the cell. This actually makes more sense, because I assume you have fewer checkboxes than rows, so the dictionary will take up less memory. \$\endgroup\$
    – Mike Nakis
    Commented Dec 22, 2011 at 22:29
1
\$\begingroup\$

If you can stand a little LINQ and Task Parallel library, this might give it a little boost:

    private void filter(IEnumerable<CheckBox> boxList)
    {
        // refLanguageTabs is a TabControl the Tabcount is 9
        for (var i = 0; i < refLanguageTabs.TabCount; i++)
        {
            var ctrls = refLanguageTabs.TabPages[i].Controls.Find(refLanguageTabs.TabPages[i].Name + "Grid", true);
            var dgv = ctrls.Any() ? ctrls[0] as DataGridView : null;

            if (dgv == null)
            {
                continue;
            }

            // average row count is 3000
            Parallel.For(0, dgv.RowCount, j =>
            {
                foreach (var t in boxList.Where(t => dgv.Rows[j].Cells[1].Value.ToString() != t.Name.ToString()))
                {
                    dgv.BeginInvoke(new MethodInvoker(() => { dgv.Rows[j].Visible = false; }));
                }
            });

            dgv.Refresh();
        }
    }
\$\endgroup\$

Your Answer

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

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