HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Moving List View items from one list to another

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
viewoneitemsmovinganotherlistfrom

Problem

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

Solution

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), 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);
    }
}

Code Snippets

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);
    }
}

Context

StackExchange Code Review Q#11469, answer score: 5

Revisions (0)

No revisions yet.