CODEWITHSUNDEEP
c#resharper
bflemi3
bflemi3

Reputation: 6790

Difference between IDictionary and IEnumerable<KeyValuePair>

Resharper is suggesting that I change IDictionary<string, string> in the following line of code:

private static void createCookie(HttpCookie cookie, IDictionary<string, string> values)

to IEnumerable<KeyValuePair<string, string>>.

I don't understand the advantage to using IEnumerable<KeyValuePair<string, string>> over IDictionary.

Upvotes: 4

Views: 4027

Answers (5)

Palec
Palec

Reputation: 13581

The best practice is to use the most general type that satisfies the requirements of the method on the input.

Static analysis tools like ReSharper or SonarQube give hints when they find a more general type that may be used while the code still compiles (roughly speaking).

They may over-generalize, though:

  1. You may need further members of the more specific type in a future iteration of the code. Do not consider just what the implementation currently requires -- think about its interface and how to make it evolvable. Changing the parameter type back to a more specific one is a breaking change -- it may be better to keep the more specific one from the start.
  2. You may already need the stronger semantics provided by the more specific type that cannot be expressed at the type system level. Or you may need them in a future iteration -- see the previous point.

The semantic difference mentioned in point 2 above may be illustrated by iterating IDictionary<TKey, TValue> versus IEnumerable<KeyValuePair<TKey, TValue>>. Both fit the bill syntactically, but IDictionary<TKey, TValue> promises that the keys do not repeat, which may be crucial to the correctness of the implementation.

This is a correct implementation of dictionary cloning when the parameter and return types change to IDictionary<TKey, TValue>, but it is broken as it is:

using System;
using System.Collections.Generic;

static IEnumerable<KeyValuePair<TKey, TValue>> Clone<TKey, TValue>(
    IEnumerable<KeyValuePair<TKey, TValue>> dictionary)
    where TKey : notnull
{
    ArgumentNullException.ThrowIfNull(dictionary);

    var clone = new Dictionary<TKey, TValue>();
    foreach (var (key, value) in dictionary)
    {
        clone.Add(key, value);
    }

    return clone;
}

The call to the dictionary's Add() throws for this input: System.Linq.Enumerable.Repeat(KeyValuePair.Create("duplicate key", "value"), 2).

Worse, if you used the indexer instead of the Add() method, it would not throw, but the "clone" would be a sequence of a different length. Not what the caller expects and how a Clone() method's interface should be designed.

Cloning a sequence is just very different from cloning a dictionary.

Upvotes: 0

termit
termit

Reputation: 200

Resharper suggests to use base class/interface for method argument whenever it's possible.

It's related to Dependency inversion principle of OOP: https://en.wikipedia.org/wiki/Dependency_inversion_principle.

Theoretically it's possible that there are some classes that implement IEnumerable<KeyValuePair<string, string>> but don't implement IDictionary<string, string>. And if you use IEnumerable<KeyValuePair<string, string>> your method will be more universal because it will be able to work with larger set of objects.

Upvotes: 1

Nick Bray
Nick Bray

Reputation: 1963

Resharper might wrong. You are not using any of IDictionary's specific members now, but you might eventually add them in the future. If this method should always work on dictionaries, then you might be better off defining that in your public interface. That way if you end up changing the implementation in the future to use dictionary specific methods you won't be causing any problems. Resharper only sees what you have written now, not what you expect to write in the future.

Upvotes: 0

Jan D&#246;rrenhaus
Jan D&#246;rrenhaus

Reputation: 6717

Resharper noticed that you are not doing anything dictionary-specific in your code, so it suggest to allow more generic objects to be accepted as well. All you are doing in your code, you can also do with an IEnumerable<KeyValuePair<string, string>>.

Upvotes: 9

Matthew Watson
Matthew Watson

Reputation: 109762

There isn't much advantage in this specific case.

However, in general, it's better to use the most general parameter type that you can, since it increases the number of ways that you can call the method.

So for your function, you could pass anything that implements IEnumerable<KeyValuePair<string, string>>, which includes IDictionary<string, string> plus potentially many other types too.

Resharper doesn't know whether or not it would actually be that useful, so it always warns you about it.

Upvotes: 2

Related Questions