Runtime: Proposal: Dictionary<TKey, TValue>.Remove(TKey, out TValue)

[Thaina]

API Proposal

```c#
public bool Remove(TKey key, out TValue value);

### Motivation

The following is a common way to pull an item out of a Dictionary:

```c#
Dictionary<TKey, TValue> data = ...;
...
if(data.TryGetValue(key, out value))
{
  data.Remove(key);
  // do something with value
}

This requires the dictionary to calculate the hash code twice when the key is in the dictionary.

Details

Functionality-wise it would do the same thing as the following method, but will be more efficient by having to generate the hash code only once:

c# public bool Remove(TKey key, out TValue value) { if(!TryGetValue(key, out value)) return false; Remove(key); return true; }

Original Proposal

Like Stack.Pop or Queue.Dequeue, I think ability to get and remove at the same time is common

So I think we should have T TakeAt(int index),T[] TakeRange(int index,int count) for List and T TakeValue(K key),bool Remove(K key,out V value) for Dictionary in addition to Remove

For performance wise it should implement on base class directly

[safern]

So as we understand from this proposed APIs is that

• T TakeAt(int index, int count) doesn't seem valuable as you could only do

Get(int index);
Remove(int index);

• T[] TakeRange(int index, int count) same here, you could only do it in two lines of code as:

List.GetRange(index,count);
List.RemoveRange(index,count);

• What we understand from T TakeValue(K key) is that it is just a getter which already exists in Dictionary, is this correct?

• TryRemove(K key, out T Value) sounds reasonable and a good API to have so could you do an API Proposal for that specific API. Here is an example of an API proposal dotnet/corefx#15691

As a related issue we would like to point this one out dotnet/runtime#14676

Thanks and let us know if you have any question @Thaina

[Thaina]

@safern First, reduce it from 2 lines tobe only one line has benefit on one line return. Such as Linq

I wanted this API when I was writing random unique select with Linq

```C#
var list = new List(arr);
var randomTake = Enumerable.Range(0,3).Select((n) => {
int i = random.Next(0,list.Count);
var item = list[i]; // Also cannot remove before get so I need to temp to return. Another line
list.RemoveAt(i);
return item;
});

instead I should
```C#
var list = new List(arr);
var randomTake = Enumerable.Range(0,3).Select((n) => list.TakeAt(random.Next(0,list.Count)));

Second. I use the word Take to make a difference from Get that it will Get and Remove at the same time

Dictionary.TakeValue(K key) I propose work like the list above. Internally it act like
C# var obj = dict[key]; dict.Remove(key);
In one line. Same reason as List above

Third. I request this feature for performance too. So we could check index and indexing then removing in the same function. Especially for Dictionary that it need to lookup for key before get and remove.

Also as I mention Stack.Pop or Queue.Dequeue. This feature should be standardized. So I don't need to make extension method and have a possibility to conflict with anyone

[Thaina]

ps. I myself don't like word Take but I just can't think of another vocab. Are there anyone have better wording for this functionality?

[jnm2]

ps. I myself don't like word Take but I just can't think of another vocab. Are there anyone have better wording for this functionality?

Dunno if it's better, but in my codebase I've stuck to calling this Pop because of the resemblance to the stack operation. Could just as easily be Pull though.

[ianhays]

@Thaina

is T List.TakeAt(int index,int count) supposed to be T List.TakeAt(int index)? Why does it have a count?

Did you only want T Dictionary.TakeValue(K key) in addition to bool Dictionary.TryRemove(K key,out V value) so you could have the return type be T instead of bool? To me it seems more potentially confusing than actually beneficial when you consider that we'd have 3 different ways to do the same operation.

I'm not certain that the List methods justify the API addition where get/remove are constant time operations and combining them doesn't save you real time.

[Thaina]

@ianhays

is T List.TakeAt(int index,int count) supposed to be T List.TakeAt(int index)? Why does it have a count?

That's right, fixed. Thank you

For Dictionary, I just think we should have compact keyword in one line. Make it 2 lines again defeat the purpose of making this API
Please consider that we can get value from dictionary by indexer and TryGetValue. And we just use each API in different place
Also Remove is not the same purpose as that TryRemove and TakeValue. So why you say it 3 way to do same operation ? We cannot get any value from Remove

For List, I have seen implementation of list and there are operation before and after both function that do the same. Even it not much it still add up
But anyway. The main reason is the same as dictionary. The compact one line API is useful

[Thaina]

On a second thought. I think we should just have bool Remove(K key,out V value) being overload method

Because Remove already return bool. Should not need Try in the name

[danmosemsft]

TryRemove on Dictionary makes sense to me for the same efficiency argument as TryAdd which we already approved.

It actually has 376 votes on User Voice: https://visualstudio.uservoice.com/forums/121579-visual-studio/suggestions/4035387-add-tryremove-tkey-key-out-tvalue-value-to-dicti

I could go either way on the name (Remove is already basically TryRemove) but if it matters ConcurrentDictionary has both TryRemove and TryAdd.

I suggest having one issue for the Dictionary proposal and another for Array, unless someone wants to write both of them up. I think the Dictionary one would likely not be too controversial.

[safern]

I also really like the TryRemove on Dictionary as you mentioned! We're just not convinced about the Array proposals. @Thaina could you please open anothe issue with the Array proposed APIs?

I can help you writing the API proposal for TryRemove so that we can have it reviewed and if so approved.

[danmosemsft]

@sepidehMS as you have a few cycles, do you want to write this up, since IMO it would be nice to squeeze this into 2.0?

[sepidehkh]

Proposal added on top

[safern]

Thanks @sepidehMS looks good.

[karelz]

Minor formatting modifications to the proposal. BTW: Nice job @sepidehMS! Thanks!

[karelz]

API review: Approved if we change the method name to Remove - it will be overload of existing bool Remove(TKey):
c# public bool Remove(TKey key, out TValue value);

[karelz]

I modified the top post and renamed TryRemove to Remove.

[ianhays]

Yay!

[safern]

Nice :)

[WinCPP]

@karelz May I grab this?

[karelz]

@WinCPP assigned to you ...

[WinCPP]

Thanks @karelz !!

[karelz]

We should thank you for contributions, not the other way around ;-)

[danmosemsft]

@WinCPP not sure whether you've done this before, but the impl of Dictionary is in the coreclr repo in Dictionary.cs. It is necessary to make a change there and prepare a PR, and also prepare a change in this corefx repo that does two things: (1) exposes the new member in src\System.Collections\ref\System.collections.cs and (2) adds tests for it in src\System.Collections\tests\Generic\Dictionary.
To run your tests against your implementation, look at this.
When you have the corefx PR up with passing tests, we can merge the coreclr PR. A day or two later it will flow to corefx and we can merge the corefx PR and we're done.

[WinCPP]

@danmosemsft thank you for the concise note. Yup, been there previously, with all the mess I could possibly do, for dotnet/corefx#16274. I thank @stephentoub and @karelz for patiently bailing me out every time I got into a messy situation. :-)

[WinCPP]

@danmosemsft @karelz I have open a PR #10203. Kindly review.

I am working on the CoreFx changes + tests. Will raise a PR for them tomorrow. Thanks.

[safern]

Thanks @WinCPP we will review your PR. Thanks for your contribution.

[karelz]

Thanks for pushing it through @WinCPP!