Best Practice - Removing item from generic collection in C#

Question

I'm using C# in Visual Studio 2008 with .NET 3.5.

I have a generic dictionary that maps types of events to a generic list of subscribers. A subscriber can be subscribed to more than one event.

private static Dictionary<EventType, List<ISubscriber>> _subscriptions;

To remove a subscriber from the subscription list, I can use either of these two options.

Option 1:

ISubscriber subscriber;  // defined elsewhere
foreach (EventType event in _subscriptions.Keys) {
    if (_subscriptions[event].Contains(subscriber)) {
        _subscriptions[event].Remove(subscriber);
    }
}

Option 2:

ISubscriber subscriber;  // defined elsewhere
foreach (EventType event in _subscriptions.Keys) {
    _subscriptions[event].Remove(subscriber);
}

I have two questions.

First, notice that Option 1 checks for existence before removing the item, while Option 2 uses a brute force removal since Remove() does not throw an exception. Of these two, which is the preferred, "best-practice" way to do this?

Second, is there another, "cleaner," more elegant way to do this, perhaps with a lambda expression or using a LINQ extension? I'm still getting acclimated to these two features.

Thanks.

EDIT

Just to clarify, I realize that the choice between Options 1 and 2 is a choice of speed (Option 2) versus maintainability (Option 1). In this particular case, I'm not necessarily trying to optimize the code, although that is certainly a worthy consideration. What I'm trying to understand is if there is a generally well-established practice for doing this. If not, which option would you use in your own code?

Answer 1

Option 1 will be slower than Option 2. Lambda expressions and LINQ will be slower. I would use HashSet<> instead of List<>.

If you need confirmation about item removal, then Contains has to be used.

EDITED: Since there is a high probabilty of using your code inside lock statement, and best practice is to reduce time of execution inside lock, it may be useful to apply Option 2. It looks like there is no best practice to use or not-use Contains with Remove.

Answer 2

I'd opt for the second option. Contains() and Remove() are both O(n) methods, and there's no reason to call both since Remove doesn't throw. At least with method 2, you're only calling one expensive operation instead of two.

I don't know of a faster way to handle it.

Answer 3

The Remove() method 'approches O(1)' and is OK when a key does not exist.

But otherwise: when in doubt, measure. Getting some timings isn't that difficult...

Answer 4

If you wanted to use Linq to do this, I think this would work (not tested):

_subscriptions.Values.All(x => x.Remove(subscriber));

Might want to check the performance on that though.

Answer 5

Why enumerate the keys when all you're concerned with is the values?

foreach (List<ISubscriber> list in _subscriptions.Values)
{
    list.Remove(subscriber);
}

That said, the LINQ solution suggested by Eric P is certainly more concise. Performance might be an issue, though.

Answer 6

collection is a two type 1 non-generic 2 generic non generic 4 type: 1 array-list 2 heap 3 stack 4 queue