Prefer Dictionary<K, V>.TryAddValue(key) over guarded Add(key)

[terrajobst]

!Dictionary<K, V>.ContainsKey(key) followed by Dictionary<K, V>.Add(key) can be combined into a single TryAdd call.

Category: Performance

[jeffhandley]

Estimates:

• Analyzer: Medium
• Fixer: Medium

[carlossanlop]

Suggested severity: Info

Cases to cover:

using System;
using System.Collections.Generic;

namespace MyNamespace
{
    public static class MyClass
    {
        static void Main()
        {
            Dictionary<int, int> d = new();

            // A) Check the negative returned value
            // Before
            if (!d.ContainsKey(5))
            {
                d.Add(5, 6);
            }

            // After
            d.TryAdd(5, 6);


            // B) Check the negative returned value and preserve existing logic inside the if body
            // Before
            if (!d.ContainsKey(5))
            {
                d.Add(5, 6);
                Console.WriteLine($"Value: {d[5]}");
            }

            // After
            if (d.TryAdd(5, 6))
            {
                Console.WriteLine($"Value: {d[5]}");
            }

            // C) Check the positive returned value and preserve existing logic in the else bodyif there's one
            // Before
            if (d.ContainsKey(5))
            {
                Console.WriteLine($"Value existed: {d[5]}");
            }
            else
            {
                d.Add(5, 6);
                Console.WriteLine($"Value added: {d[5]}");
            }

            // After
            if (!d.TryAdd(5, 6))
            {
                Console.WriteLine($"Value existed: {d[5]}");
            }
            else
            {
                Console.WriteLine($"Value added: {d[5]}");
            }
        }
    }
}

[GrabYourPitchforks]

I'd suggest enabling this analyzer only for scenarios where the associated value is: (a) a literal; or (b) already fully hydrated before the call to ContainsKey. Otherwise you risk a perf regression in scenarios like the following.

if (!dict.ContainsKey(key))
{
    dict[key] = ComputeExpensiveValue(); // expensive computation guarded within ContainsKey check
}

In theory you could take a factory argument to perform the expensive computation, but now we'd be making callers understand lambdas and closures when all they wanted to do was a simple dictionary access, and I don't think that's a good trade-off for the majority of our developers.

[terrajobst]

Video

• Looks good
• Please consider the comment here.

[CollinAlpert]

I'd like to do this one. @carlossanlop

[carlossanlop]

@buyaa-n @CollinAlpert has a PR been merged fixing this? I see there are 3 related issues and more than one related PR but I'm unsure which issues got fixed.

[buyaa-n]

Yes this one is fixed

[buyaa-n]

Oops, mistaken with #33798, it is not done, reopening