Panagiotis Kanavos' Weblog

Totally wrong way to filter a combo box

From the same legacy code as this post comes this code that loads a combo box with ListItems and then removes unwanted items in a way that is guaranteed to cause an exception when the indexer reaches the middle of the list, which now has only half the initial items:

ddlItems.Items.Clear();

// Re-load so we can filter the required
UIHelper.FillListBoxFromSharePointList(ddlItems, Microsoft.SharePoint.SPContext.Current.Web, "Items");

#region Remove all items from list that are not allowed for selection by the current user
int listCount = ddlItems.Items.Count;

for (int i = 0; i < listCount; i++)
{
    try
    {
        ListItem item = ddlItems.Items[ i];

        string roleId = item.Value;
		
        if (roleId.Length > 0)
        {
            roleId = string.Concat(";", roleId, ";");

	   if (!allowedRolesStringList.Contains(roleId))
            {
                ddlItems.Items.Remove(item);
                i = i - 1;  // Skip back 1 index - since we deleted one.
            }
        }
    }
    catch { ; }
}

#endregion

Obviously the coder noticed the problem and instead of fixing it, he covered it under the catch{;} carpet. The immediate problem could be fixed of course just by reversing the direction of the iteration

ddlItems.Items.Clear();

// Re-load so we can filter the required
UIHelper.FillListBoxFromSharePointList(ddlItems, Microsoft.SharePoint.SPContext.Current.Web, "Items");

#region Remove all items from list that are not allowed for selection by the current user
int listCount = ddlItems.Items.Count;

for (int i = listCount-1; i >= 0; i--)
{
        ListItem item = ddlItems.Items;
        string roleId = item.Value;
		
        if (roleId.Length > 0)
        {
           roleId = string.Concat(";", roleId, ";");
           if (!allowedRolesStringList.Contains(roleId))
            {
                ddlItems.Items.Remove(item);
            }

        }
}

#endregion

Of course the real solution would be to filter the collection of list items before loading them in the combobox. Or, better yet, bind them to the list of desired items instead of adding the list items one by one. Unfortunately, the code that loads the ListItems and adds them to the combo is lost and recoverable only through Reflector ....

P.S. Notice the #region blocks inside the code. You know your methods are too big when you have to break them apart using #region . It is infinetely better just to break the regions out into their own methods instead of creating this (un)maintenability nightmare.