Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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();
            }
        }
share|improve this question
add comment (requires an account with 50 reputation)

3 Answers

up vote 3 down vote accepted

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.

share|improve this answer
Is there a speed difference between for and foreach? – PoiXen Dec 19 '11 at 16:09
1  
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. – Mike Nakis Dec 19 '11 at 16:38
So I gather its like using iterators in c++ . . . . – PoiXen Dec 20 '11 at 8:09
1  
@PoiXen: The main reason is readability. – Bobby Dec 20 '11 at 8:17
Caching the value of the cell in a local variable and using break are indeed optimizations which could produce a perceptible difference. But I still bet that the optimization that I proposed (just 2 passes instead of N*M passes) should provide the greatest performance improvement. Did you try it, @PoiXen? – Mike Nakis Dec 22 '11 at 21:46
show 1 more commentadd comment (requires an account with 50 reputation)
up vote 1 down vote

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.

share|improve this answer
Whoever upvoted this, thank you. It is a pity how the best answer is some times overlooked! C-:= – Mike Nakis Dec 20 '11 at 20:47
What name do you mean to use for the key? – Bobby Dec 22 '11 at 22:15
Hmmmm, I meant the value of the cell. As in MyDictionary[row.Cells[1].Value] = row.Cells[1] – Mike Nakis Dec 22 '11 at 22:23
And what if there are multiple cells with the same value? – Bobby Dec 22 '11 at 22:25
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. – Mike Nakis Dec 22 '11 at 22:29
show 6 more commentsadd comment (requires an account with 50 reputation)
up vote 1 down vote

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();
        }
    }
share|improve this answer
add comment (requires an account with 50 reputation)

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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