5
\$\begingroup\$

I'm writing a form that contains 2 ListViews with a set of 4 buttons. 2 move all the items from one to the other in each direction, the other move only selected items in each direction.

Nothing fancy or complicated.

However I'm feeling like I could write the MoveItem method in a more elegant way and was wondering if any would mind taking a look and advising me.

private void MoveItem(ListView source, ListView dest, bool all)
{
    if (all)
    {            
        foreach (ListViewItem item in source.Items)
        {
            source.Items.Remove(item);
            dest.Items.Add(item);
        }
    }
    else
    {
        foreach (ListViewItem item in source.SelectedItems)
        {
            source.Items.Remove(item);
            dest.Items.Add(item);
        }
    }
}

Any suggestions would be thankfully recived

\$\endgroup\$
5
\$\begingroup\$

Before thinking about how to write this in a more elegant way, you should first ask yourself: is this code actually correct? The “problem” with that code is that you're iterating the collection you're modifying. Quite often (for example in the case of List<T>), that's not allowed and your foreach would throw an exception. Fortunately for you, the enumerator of ListViewItemCollection iterates over a snapshot of the collection of the items, so your code will actually work fine (although I didn't find this behavior documented anywhere).

Now, if you want to refactor code like this, just have a look at what's the same and what's different. You can notice that the only different thing in the two branches is the source of the foreach. So, just create a local variable for it, set it correctly and then have common code that iterates it:

private static void MoveItem(ListView source, ListView dest, bool all)
{
    var items = all ? source.Items : source.SelectedItems;

    foreach (ListViewItem item in items)
    {
        source.Items.Remove(item);
        dest.Items.Add(item);
    }
}
\$\endgroup\$
  • \$\begingroup\$ So would I be correct in thinking that with a normal list that doesn't create a snapshot the best solution would be to create my own copy of the items and then add/remove them? That refactoing was just what I was trying to do and failing as SelectedItems and Items are different types that don't inherit from a common base class but do implement IList \$\endgroup\$ – Hudsonw May 4 '12 at 12:20
  • \$\begingroup\$ Yeah, exactly. And the easiest way to create that snapshot is using ToArray(). \$\endgroup\$ – svick May 4 '12 at 12:21
  • \$\begingroup\$ Does this work? Modifying a list while looping over it don't cause an exception? \$\endgroup\$ – mmdemirbas May 4 '12 at 14:56
  • 1
    \$\begingroup\$ I would say that even though it does work and is safe, the code looks like it is modifying an existing collection and is thus better written with an explicit snapshot. \$\endgroup\$ – Bill Barry May 4 '12 at 22:06
  • 1
    \$\begingroup\$ How does this work? My code won't compile because it doesn't know what items should be. Type of conditional expression cannot be determined because there is no implicit conversion between 'ListView.ListViewItemCollection' and 'ListView.SelectedListViewItemCollection' .NET 4.0 \$\endgroup\$ – sab669 Jul 9 '15 at 21:14
1
\$\begingroup\$

Easier:

item.Remove <-- will remove item from the ListView it is currently assigned DestinationList.Items.Add(item);

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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