保存循环或使用2种方法 - 约定与性能

Question

因此,每个关心OOP中的最佳实践并保持代码清洁的人都知道这些方法不应该只做一件事.方法是离散的单位,做一件事,然后离开.

但是,如果您要将两个实际上做两件事的方法结合起来并重用第一种方法中已有的现有for循环,那么可以节省一些处理并提高性能的情况如下:

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

这些在页面加载时调用.一个删除已停止的项目.另一个删除具有某些已更改的定价选项的项目,并将其从同一列表中删除.

现在,如果我们坚持使用最佳实践,可以说这些是完全独立的两个目的,因此我们不应该将这两种方法中的逻辑结合起来.然后,这将使该方法做两件事,我还必须提出一些新的名称,如RemoveDiscontinuedAndPriceChangeItems()或通用名称,不告诉我杰克sh**像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--;
        }
    }

但是考虑到性能方面,调用2个循环遍历同一列表的方法来删除一些项目,在循环中会更昂贵.

那么,任何反对组合的人还是你在这种情况下合并？想听听那里的一些意见.

Answer 1

为什么不重构,以便每个方法执行单个操作,而不是执行循环.然后在循环体中根据需要调用每个方法.

更新 此处是基于您的方法的快速示例.显然,在你的应用程序中,Loop方法会有所不同,但是,我没有任何其他上下文来处理你正在做的事情.另外,我将你的for循环改为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

我认为你的因素是在错误的地方.

如果你这样写:

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

我也不太喜欢你弄乱循环变量的风格 - 我更喜欢这种风格:

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

如果你不喜欢它的表现,你可以随时这样做:

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

Answer 3

一些东西:

• 我不明白为什么你在删除项目时向前循环.您应该向后循环,并在执行删除时避免混乱的索引操作.

• 根据属性删除一个项目与另一个项目的关系是不正确的.您应该将其视为过滤列表.为此,你应该有一个方法,它接受一个Predicate<T>然后然后返回一个IEnumerable<T>你可以枚举的方法(或者IList<T>,如果你想只改变列表,则相同).这样,您就可以对列表进行过滤和条件分离(这是更好的分离IMO).

• 如果您可以访问LINQ,那么就没有理由这样做.您应该在原始列表中使用where过滤器,如果需要,将过滤逻辑分解为单独的方法,这些方法将获取该项并查看是否应该返回该项.然后,您可以从中构造where子句(或Predicate<T>传递给Where扩展方法).