Iterating over KeyValuePair (as a key in dictionary) is not working

Question

Following is my function:-

public void deleteAllCarsWithType(Dictionary < KeyValuePair<string, string> , car > dictCar, string typeOfCar)
{
  var keyValuePairs = dictCar.Where(keyValuePair => keyValuePair.Key.Equals(typeOfCar));

  //var count = keyValuePairs.Count();
  //this also gives the size of keyValuePairs as 0.

  foreach (var keyValuePair in keyValuePairs)
  {
    dictCar.Remove(keyValuePair.Key);
  }
}

KeyValuePair<string, string> which is the key of the dictionary, has carType as key and carModel as the model of the car.

When I run the unit test of it

[TestMethod]
public void DeleteSameCarTypeTest()
{
  Dictionary<KeyValuePair<string, string>, car> Dictionary = new Dictionary<KeyValuePair<string, string>, car>();
  Dictionary.Add(new KeyValuePair<string, string>("x", "y"), new car());
  Dictionary.Add(new KeyValuePair<string, string>("x", "z"), new car());

  new Program().deleteAllCarsWithType(Dictionary, "x");
  Assert.AreEqual(0, Dictionary.Count);
}

It fails by saying Expected 0 and Actual 2.

When I debug it, I found out that keyValuePairs have the expected two values (which are with carType "x") but when I step into the foreach loop it's not going in and saying that count of keyValuePairs is 0. Then I tried to get the Count of the keyValuePairs which is also coming 0. I don't know what I am doing wrong. Please help me out.

@BradleyUffner I am iterating over keyValuePair not the actual dictionary. But first of all it's not going inside the loop. you can check running this program. — OldSchool, Sep 18 '17 at 13:33
This is a little confusing... You know when iterating over a dictionary (`dictCar.Where...`) you iterate through the `KeyValuePair, car>`... not through your keys of type `KeyValuePair`? — René Vogt, Sep 18 '17 at 13:37
@RenéVogt yes you are right. But as my test suggests, it should have two values. — OldSchool, Sep 18 '17 at 13:39
So `keyValuePair.Key` is of type `KeyValuePair`, so I guess it will never `Equal` a `typeOfCar`... btw: there will always be only **one** key that is equal. — René Vogt, Sep 18 '17 at 13:39
I'm pretty sure it's a bad idea to have a `KeyValuePair` as a key for a dictionary. I don't think that does what you expect, as the `Equals` (and `GetHashCode`) method of `KeyValuePair` will not work as required for a dictionary-key. — René Vogt, Sep 18 '17 at 13:42
@RenéVogt `Equals` will work as expected I think. If you use the same key say [{"x","y"}] to insert another value it will throw `ArgumentException`. I think this kind of proves that it's working as expected. — OldSchool, Sep 18 '17 at 13:45
Tested that, you're right: key and value are compared by `Equals`. Still feels a little strange to use that type for a dictionary key (but in the end it's almost the same as a tuple). — René Vogt, Sep 18 '17 at 13:53
I don't believe it will work for non-compile time known keys, as the compiler will optimise strings known at compile time to be identical to have the same reference. However run time strings are not optimised this way, so will have different references and will not be equal @RenéVogt — Yair Halberstadt, Sep 18 '17 at 13:54
@YairHalberstadt IMHO then you are indirectly saying that we should not be using `string` as keys in a dictionary? — OldSchool, Sep 18 '17 at 13:56
I'm not certain. However if you want to be sure, it would be best to inherit from KeyValuePair and override the equals method. — Yair Halberstadt, Sep 18 '17 at 13:59
@YakRangi. No. You can use strings since string.equals does not check for reference equality. However valuetype.equals might. I dont know. — Yair Halberstadt, Sep 18 '17 at 14:00
according to https://stackoverflow.com/a/1009456/7607408 it should work as it calls equals on the fields. However it may be slow as it uses reflection. — Yair Halberstadt, Sep 18 '17 at 14:03
@YairHalberstadt even if I subclass it and overrides the equals method it will not be fast because again it will call the equals of string which is basically comparing on value type. I think we should leave it as it is or it will have any other downside? — OldSchool, Sep 18 '17 at 14:09
It will be faster, as it wont use reflection. However premature optimisation is the root of all evil, so it may be sensible to leave it for now, and speed it up later if you need it. — Yair Halberstadt, Sep 18 '17 at 14:11

score 3 · Accepted Answer · answered Sep 18 '17 at 13:39

3

keyValuePair.Key is a KeyValuePair. You need:

var keyValuePairs = dictCar.Where(keyValuePair => keyValuePair.Key.Key.Equals(typeOfCar));

In fact a few more changes need to be made as others have pointed out:

private static void deleteAllCarsWithType(Dictionary<KeyValuePair<string, string>, car> dictCar, string typeOfCar)
{
    var keyValuePairs = dictCar.Where(keyValuePair => keyValuePair.Key.Key.Equals(typeOfCar)).ToArray();

    foreach (var keyValuePair in keyValuePairs)
    {
        dictCar.Remove(keyValuePair.Key);
    }               
}

answered Sep 18 '17 at 13:39

Yair Halberstadt

5,733
28
60

2

As Juan pointed out it is, or you will remove an item during iteration, which will cause an exception to be thrown in foreach. – Yair Halberstadt Sep 18 '17 at 13:51
so `keyValuePairs` is actually a reference to the actual `key` of the dictionary and hence removed from the dictionary through the loop. Am I right? – OldSchool Sep 18 '17 at 13:59
linq uses lazy evaluation, meaning if you change the original collection, you change the collection returned from linq. To cache the collection (hold it fixed) you need to use ToArray or ToList. – Yair Halberstadt Sep 18 '17 at 14:05

score 2 · Answer 2 · answered Sep 18 '17 at 13:39

You are looking into the values as opposed to the keys.

Change your code to this:

private static void deleteAllCarsWithType(Dictionary<KeyValuePair<string, string>, car> dictCar, string typeOfCar)
{
    var keyValuePairs = dictCar.Keys.Where(keyValuePair => keyValuePair.Key.Equals(typeOfCar)).ToArray();

    foreach (var keyValuePair in keyValuePairs)
    {
        dictCar.Remove(keyValuePair);
    }               
}

Notice I am also converting the query to an array. Otherwise the loop will fail because you modify the collection.

Iterating over KeyValuePair (as a key in dictionary) is not working

2 Answers2