6

It is my first time here and I am struggling to solve this issue. I have this piece of code:

try
{
    progressBar1.Maximum = lista.Items.Count;
    lista.BeginUpdate();

    for (int i = 0; lista.Items.Count > i; i++)

    //for (int i = lista.Items.Count - 1; -1 < i; i--)
    {
        if (lista.Items[i].SubItems[1].Text.ToLower().Contains(Text) == false)
        {                        
            lista.Items[i].Remove();                        
        }

        progressBar1.Value = progressBar1.Value + 1;
    }

    lista.EndUpdate();

    progressBar1.Value = 0;
}
catch (Exception errore)
{
    txt_info.Text = "" + errore.Message;
    progressBar1.Value = 0;
}

The method lista.items[i].remove is extremely slow. lista is a ListView and I am working on a log file bigger than 50,000 lines. Is there anyway to speed up the process?

4
  • 3
    Curious...does lista.Items.RemoveAt(i) differ in speed? Perhaps (counter-intuitively) the class has to go back and resolve the index itself. May 31, 2013 at 15:56
  • 4
    You shouldn't change the size of a data structure (remove items from it) within a For loop. Try re-writing the loop without having the remove in it. For example flag the indexes that need to be removed in the for loop and then remove them outside of it. May 31, 2013 at 16:13
  • 2
    @RezaShirazian One clearly can't do that with foreach... I believe it reasonably ok to use for to remove items (eventually some piece of code have to do it anyway) - also obviously incorrectly written (like in sample above due to skipping items next to one that just removed) is bad idea. May 31, 2013 at 16:20
  • DonBoitnott is right. RemoveAt(i) is the answer - verified. And yes, the logic in the loop doesn't make sense.
    – Yogee
    May 31, 2013 at 16:40

6 Answers 6

3

I would take a different approach and use LINQ, something like this :

lista.Items = lista.Items.Where(x=>x.SubItems[1].Text.ToLower.Contains(Text)).AsParallel().ToList();

Basically, rebuilding the list once rather than trying to remove individual items over and over again.

5
  • 1
    This is a good and compact implementation, although the AsParallel strikes me as overkill. May 31, 2013 at 16:23
  • The ToLower and Contains are both CPU costing operations, if the list is large it will benefit from the Parallel approach
    – David C
    May 31, 2013 at 16:23
  • 2
    ListView.Items is a read-only property, so this doesn't work. May 31, 2013 at 16:28
  • 1
    You should replace the .ToLower().Contains(Text) with .IndexOf(Text, StringComparison.CurrentCultureIgnoreCase) != -1, which will avoid creating a new string for comparison. May 31, 2013 at 16:29
  • 1
    @MichaelLiu wow, good call, i didn't even check if it had a setter.
    – David C
    May 31, 2013 at 16:36
2
ListViewItem[] allElements = new ListViewItem[listView1.Items.Count];
listView1.Items.CopyTo(allElements, 0);
List < ListViewItem > list = allElements.ToList();
list.RemoveAll(item => item.SubItems[1].Text.ToLower().Contains(TextToFind) == false);
listView1.BeginUpdate();
listView1.Clear();
listView1.Items.AddRange(list.ToArray());
listView1.EndUpdate();

First Rule is never update list in for loop. Your logic will only run till half of the list. I guess that's not what you want.
I've seen that manipulating listview.items is very slow even after using BeginUpdate and EndUpdate. Key is to do the manipulation outside (in list or so) and then re populate the list with AddRange (which is much faster than Add).

7
  • i have tried the code above but gives me back an empty list. Do i miss something ? In your comment you said that you are using foreach loop but i don't see it in the code that you left. Jun 3, 2013 at 10:39
  • @user2441083: My older comment was using foreach and then I was removing elements from list. But then I used RemoveAll which didn't require foreach. Btw, when does the list becomes empty? After RemoveAll? if so, you need to check if the condition written in "RemoveAll" might be doing so.
    – Yogee
    Jun 3, 2013 at 12:14
  • @user2441083: I wrote the same logic in my application and it is working fine and fast enough.. takes around 2450 milliseconds for removing 10% of list times for 50,000 entries of two dimensional string as LIstViewItems in listView1.Items. Are you sure you want to check for item.SubItems[1] and not item.SubItems[0]?
    – Yogee
    Jun 3, 2013 at 12:20
  • I did a bit of debug and the problem is actually after the lista.clear(). It seems that the AddRange is not filling the list back. Jun 3, 2013 at 13:13
  • 1
    done !!! It works perfectly, i have just readded the column lista.Columns.Add("Description"); before use the AddRange. Thank you so much for your kind support !!! Jun 3, 2013 at 14:07
2

The simplest option would be to use the list's own RemoveAll method.

list.RemoveAll(x => !x.SubItems[1].Text.ToLower().Contains(Text))

P.S.

You might want to look for speed gains in the actual comparison. Using String.Compare is much faster if your requirement fits it. If you want to check for a sub-string, I would suggest using ToUpperInvariant for invariance related matters - it's designed to be faster.

4
  • 1
    ListViewItemCollection doesn't have a RemoveAll method, so this doesn't work. May 31, 2013 at 16:43
  • @Yogee I was suggesting that filtering is several times faster for direct comparison - not a replacement for Contains. If he wants to use Contains, then ToUpperInvariant is faster.
    – Asti
    Jun 3, 2013 at 4:05
  • @Yogee SO can also be a place for general advice. Not just "give me teh codes". Nonetheless, I've edited it to weed out ambiguity.
    – Asti
    Jun 3, 2013 at 4:07
  • Thanks Asti for editing the answer. I will remove my obsolete comment. Btw, the code that I posted in this thread is reasonably faster. I am not sure if it will make any difference in performance as I see my list.RemoveAll is fast for 50Krecords. Bottleneck in this case is directly manipulating (like edits/deletes) the listview items. I noticed that if you clear and add elements (especially using AddRange), it is much faster.
    – Yogee
    Jun 3, 2013 at 7:13
0

You could stick it in a background worker and have it do this on it's own. Therefore your users could still use the program while this process is occuring.

0

If you remove an item in for loop, you should set the counter to 1 less so you won't miss one. Because you remove [i], [old i+1] becomes the [new i] (Items.Count decreases 1 also) and you will miss checking the [new i].

e.g: ListView.Items.Remove[i]; i--;

0

Lists are slow to access dynamically and much better suited to iteration. I would suggest manipulating the data outside of the listview (maybe by copying the rows you want to display into a temporary list one at a time as you iterate across the source collection) and then assigning to the listview at the end. This can be done on a different thread (to improve UI performance) and doesn't require expensive lookups.

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.