Move list items from one listbox to another

Question

In my C# application, I have two ListBox controls. One ListBox, named lstCategory, is populated with items retrieved from a database. The other ListBox is named lstSelCategory.

I want to move the selected items from lstCategory into lstSelCategory, and then sort the items in lstSelCategory. How might I go about doing this efficiently? Any help would be appreciated.

protected void BtnCopyNext_Click(object sender, EventArgs e)
 {

     try
     {
         MakeDecision(lstCategory,lstSelCategory);
     }
     catch (Exception ex)
     {

     }


 }
 private void MakeDecision(ListBox Source, ListBox Target)
 {
     int[] selectedIndices;
     try
     {
         selectedIndices = Source.GetSelectedIndices();
         if (!(selectedIndices.Length == 0))
         {
             Copy(Source, Target);
         }

     }
     catch (Exception ex)
     {
         throw ex;
     }
 }
 private void Copy(ListBox Source, ListBox Target)
 {
     int[] selectedIndices;
     ListItemCollection licCollection;
     ListBox objTarget;
     try
     {
         selectedIndices = Source.GetSelectedIndices();
         licCollection = new ListItemCollection();
         objTarget = new ListBox();
         if (Target != null && Target.Items.Count > 0)
         {
             foreach (ListItem item in Target.Items)
             {
                 objTarget.Items.Add(item);
             }
             Target.Items.Clear();
         }
         int selectedIndexLength = selectedIndices.Length;
         for (int intCount = 0; intCount < selectedIndexLength; intCount++)
         {
             licCollection.Add(Source.Items[selectedIndices[intCount]]);
         }
         int collectionCount = licCollection.Count;
         for (int intCount = 0; intCount < collectionCount; intCount++)
         {
             Source.Items.Remove(licCollection[intCount]);
             if (!objTarget.Items.Contains(licCollection[intCount]))
                 objTarget.Items.Add(licCollection[intCount]);
         }
         Target.DataSource = ConvertToArrayList(objTarget);
         Target.DataBind();
     }
     catch (Exception ex)
     {
         throw ex;
     }
     finally
     {
         licCollection = null;
         objTarget = null;
     }
 }
 private ArrayList ConvertToArrayList(ListBox Source)
 {
     ArrayList arrayList;
     try
     {
         arrayList = new ArrayList();
         foreach (ListItem item in Source.Items)
             arrayList.Add(item.Text);
         arrayList.Sort();
     }
     catch (Exception ex)
     {
         throw ex;
     }
     return arrayList;
 }

Answer 1

why not just?

var r = (from p in lstCategory.SelectedItems.Cast<string>()
    where p.Length > 0 
    orderby p
    select p);

lstSelcategory.Items.AddRange(r.ToArray());

Answer 2

It looks like there's a lot of unnecessary work going on here.

Is there any particular reason you're converting the ListBox's items to an ArrayList? Its items already implement IEnumerable, and should work just fine as the DataSource.

Also, why databind? You're already moving items from one listbox to another through the Add method. Unless there's something I don't see here, the databinding operation is expensive and completely unnecessary.

UPDATE You asked for pasted code, so I'm going to paste some of your code and discuss it:

private void Copy(ListBox Source, ListBox Target) {
     int[] selectedIndices;
     ListItemCollection licCollection;
     ListBox objTarget;
     try
     {
         selectedIndices = Source.GetSelectedIndices();
         licCollection = new ListItemCollection();
         objTarget = new ListBox();
         if (Target != null && Target.Items.Count > 0)
         {
             foreach (ListItem item in Target.Items)
             {
                 objTarget.Items.Add(item);
             }
             Target.Items.Clear();
         }
         int selectedIndexLength = selectedIndices.Length;
         for (int intCount = 0; intCount < selectedIndexLength; intCount++)
         {
             licCollection.Add(Source.Items[selectedIndices[intCount]]);
         }
         int collectionCount = licCollection.Count;
         for (int intCount = 0; intCount < collectionCount; intCount++)
         {
             Source.Items.Remove(licCollection[intCount]);
             if (!objTarget.Items.Contains(licCollection[intCount]))
                 objTarget.Items.Add(licCollection[intCount]);
         }

Up to this point, you've done pretty much everything you need to do. The items have already been added to the ListBox controls. When the page is rendered, the Framework will iterate over the ListItem collection and render appropriate HTML for the items. If ViewState is enabled for the control, the items will have ViewState cached for them.

But then, you move on to do this:

         Target.DataSource = ConvertToArrayList(objTarget);
         Target.DataBind();

     }
     catch (Exception ex)
     {
         throw ex;
     }
     finally
     {
         licCollection = null;
         objTarget = null;
     }
 }

Now, my understanding of data binding tells me that this series of statements completely overwrites anything that you've stored in the ListItemCollection. So all that work you did in the steps above is obliterated.

Further, you make a call to a function, ConvertToArrayList:

 private ArrayList ConvertToArrayList(ListBox Source) {
     ArrayList arrayList;
     try     {
         arrayList = new ArrayList();
         foreach (ListItem item in Source.Items)
             arrayList.Add(item.Text);
         arrayList.Sort();
     }
     catch (Exception ex)
     {
         throw ex;
     }
     return arrayList;
 }

Now, this is all good and well, but the problem is this: you can just set the ListItemCollection to be your data source. Or, as we've shown, you can avoid data binding altogether, since you're already building the lists yourself, and just drop this function call.

It looks like you need to make a decision about whether you want to build the list yourself, or data bind. If you're going to data bind, clean up how you do it. If you're not going to data bind, get rid of any code that isn't absolutely necessary to build the list and manage it.

ALSO:

• You're catching an exception (of type Exception, no less) and not doing anything with it but rethrowing it. If you don't intend to do anything with that exception, remove the Catch clause. If you DO plan to rethrow the exception, throw it like this to preserve the stack trace:

  try { // Blahdy blahdy } catch (Exception e) { throw; // Do not pass e; this preserves the stack trace. }

• Manually setting objects to null in the Finally clause is a hold-over from older languages that didn't have automatic garbage collection. I'd eliminate that code as it just clutters up the body of the function and makes it harder to read.

• Given the two points above, you look like you could remove the entire Try...Catch...Finally, and just let exceptions bubble up the food chain.