Register a SA Forums Account here!
JOINING THE SA FORUMS WILL REMOVE THIS BIG AD, THE ANNOYING UNDERLINED ADS, AND STUPID INTERSTITIAL ADS!!!

You can: log in, read the tech support FAQ, or request your lost password. This dumb message (and those ads) will appear on every screen until you register! Get rid of this crap by registering your own SA Forums Account and joining roughly 150,000 Goons, for the one-time price of $9.95! We charge money because it costs us money per month for bills, and since we don't believe in showing ads to our users, we try to make the money back through forum registrations.
 
  • Post
  • Reply
EkardNT
Mar 31, 2011
One issue I see is that even when one predicate approves an item, all other predicates are still run on that item. This is also why you need the HashSet, because if all n predicates approve an item it will be added n times. I suggest reversing the order of your two foreach loops so that the totalItems list is only examined once, and breaking out of the inner predicate loop once a match is found.

code:
public IEnumerable<Item> GetItems()
{
	List<Item> totalItems = GlobalAccessor.GetTotalItems(); // 2500 thingamabobs
	List<Item> selectedItems = new List<Item>();	
	
	foreach(Item item in totalItems)
	{
		foreach(Predicate<Item> pred in itemSpecifiers)
		{
			if(pred(item))
			{
				selectedItems.Add(item);
				break;
			}
		}
	}
	
	return selectedItems;
}
Here's the same thing using Linq.

code:
public IEnumerable<Item> GetItems()
{
	var totalItems = GlobalAccessor.GetTotalItems();
	return totalItems.Where(item => itemSpecifiers.Any(pred => pred(item)));
}
It's still O(totalItems.Count * itemSpecifiers.Count), which will occur when none of the predicates accept any item, but at least now you spend as little time per item as possible instead of the maximum time every time.

Adbot
ADBOT LOVES YOU

  • 1
  • 2
  • 3
  • 4
  • 5
  • Post
  • Reply