To Save a loop or to go with 2 methods - Convention vs. Performance

Question

So everyone who cares about best practices in OOP and keeping code clean and in OOP knows that methods shouldn't be doing more than one thing. Methods are discrete units that do one thing and get out.

But here's a situation though where you could save some processing and improve performance if you were to combine 2 methods which are really doing 2 things into one and reuse the existing for loop that you already have in the first method:

private void RemoveDiscontinuedItems()
{
    for(int s = 0; s < itemList.Count; s++)
    {
        if(!itemList[s].ItemIsOnSite)
        {
            RemoveItem(itemList[s].Id); // remove it from the DB
            itemList.RemoveAt(s); // remove it from the collection

            s--;
        }              
    }
}

private void RemovePriceChangedItems()
{
    for (int s = 0; s < itemList.Count; s++)
    {
        if(!PricingOptionIsValid(itemList[s]))
        {
            RemoveItem(itemList[s].Id); // remove it from the DB
            itemList.RemoveAt(s); // remove it from the collection

            s--;
        }
    }
}

These are called at page load. One removes items that are discontinued. The other removes items that have some pricing options that have changed and removes them from the same list.

Now if we were to stick with best practices, one could say that these are 2 completely independent purposes, thus we should not combine the logic in both these methods. That would then make the method be doing 2 things and I'd also have to come up with some f'd up name like RemoveDiscontinuedAndPriceChangeItems() or a generic name that doesn't tell me jack sh** like RemoveInvalidItemsFromList():

private void RemoveDiscontinuedItems()
{
    for(int s = 0; s < itemsList.Count; s++)
    {
        if((!itemList[s].ItemIsOnSite))
        {
            RemoveItem(orderItemsSavedList[s].Id);  // remove it from the DB
            itemList.RemoveAt(s); // remove it from the collection

            s--;
        }
        else if(!PricingOptionIsValid(itemList[s]))
        {
            RemoveItem(itemList[s].Id); // remove it from the DB
            itemList.RemoveAt(s); // remove it from the collection

            s--;
        }
    }

however thinking about the performance side, calling 2 methods that are looping through the same list to remove some items, would be more costly in cycles.

So, anyone against combining or are you for combining in this situation? Would like to hear some opinions out there.

Answer 1

Why not refactor so that each method performs a single action, rather then doing the loop. Then in the body of the loop call each method as needed.

Update Here is a quick example based on your methods. Obviously the Loop method would be something different, in you application, however, I didn't have any other context for what you were doing. Also, I changed your for loop to a foreach.

private void Loop()
{ 
    foreach (Item i in itemList)
    {
         if(!item.ItemIsOnSite)
         {
            RemoveDiscontinuedItems(i)
         }
         if(!item.PricingOptionIsValid)
         {
            RemovePriceChangedItems(i)
         }

    }
}

private void RemoveDiscontinuedItems(itemType item)
{

    RemoveItem(item.Id); // remove it from the DB
    item.Remove; // remove it from the collection              

}

private void RemovePriceChangedItems(itemType item)
{
    RemoveItem(item.Id); // remove it from the DB
    item.Remove; // remove it from the collection

}

Answer 2

I think you're factoring is in the wrong place.

If you wrote it like this:

public void ConditionallyRemoveItems(Func<Item,bool> predicate)
{
    for (int s=0; s < itemsList.Count; s++) {
        if (predicate(itemList[s])) {
            RemoveItem(orderItemsSavedList[s].Id);
            itemList.RemoveAt(s);
            s--;
        }
    }
 }

 // ...
 ConditionallyRemoveItems(i => !i.ItemIsOnSize || !PricingOptionIsValid(i));

I also don't really like your style of messing with the loop variable - I prefer this style:

List<Item> itemsToRemove = new List<Items>();
foreach (Item i in itemList) {
    if (predicate(i)) {
        itemsToRemove.Add(i);
    }
}
foreach (Item i in itemsToRemove)
    itemList.Remove(i);

and if you don't like the performance of that, you can always do this:

List<Item> itemsToKeep = new List<Items>();
foreach (Item i in itemList) {
    if (!predicate(i)) {
        itemsToKeep.Add(i);
    }
 }
 itemList = itemsToKeep;