Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefer Dictionary<K, V>.TryGetValue() over guarded indexer access #33798

Closed
Tracked by #57797
terrajobst opened this issue Mar 19, 2020 · 5 comments · Fixed by dotnet/roslyn-analyzers#4851
Closed
Tracked by #57797
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@terrajobst
Copy link
Member

Dictionary<K, V>.ContainsKey(key) followed by Dictionary<K, V>.this[key] can be combined into a single TryGetValue call.

Category: Performance

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 19, 2020
@jeffhandley jeffhandley added this to the Future milestone Mar 20, 2020
@jeffhandley jeffhandley added the code-fixer Marks an issue that suggests a Roslyn code fixer label Mar 21, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Medium
  • Fixer: Medium

@carlossanlop
Copy link
Member

carlossanlop commented Jan 8, 2021

Suggested severity: info

// Before
        public int TryGet(Dictionary<string, int> data, string key)
        {​​
            if (data.ContainsKey(key))
            {
                return data[key];
            }
            return 0;
        }​​
// After
        public int TryGet(Dictionary<string, int> data, string key)
        {​​
            if (data.TryGetValue(key, out int value))
            {
                return value;
            }
            return 0;
        }​​

It may be possible to preserve unrelated code inside the if statement as long as key is used without modifying the dictionary:

// Before
        public int TryGet(Dictionary<string, int> data, string key)
        {​​
            if (data.ContainsKey(key))
            {
                Foo(data[key]); // Unrelated code
                return data[key];
            }
            return 0;
        }​​
// After
        public int TryGet(Dictionary<string, int> data, string key)
        {​​
            if (data.TryGetValue(key, out int value))
            {
                Foo(value); // Unrelated code
                return value;
            }
            return 0;
        }​​

Avoid triggering if the data is modified before returning:

        public int DontTrigger(Dictionary<string, int> data, string key)
        {​​
            if (data.ContainsKey(key))
            {
                data[key] = DoSoemthing(data[key]); // Unrelated code
                return data[key];
            }
            return 0;
        }​​

@carlossanlop carlossanlop added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 15, 2021
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 4, 2021
@terrajobst
Copy link
Member Author

terrajobst commented Feb 4, 2021

Video

@CollinAlpert
Copy link
Contributor

CollinAlpert commented Feb 7, 2021

I'd like to do this one. @carlossanlop

@carlossanlop carlossanlop removed the help wanted [up-for-grabs] Good issue for external contributors label Feb 8, 2021
@carlossanlop
Copy link
Member

It's yours, @CollinAlpert . Thanks for offering your help!

@jeffhandley jeffhandley added this to the 7.0.0 milestone Aug 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
None yet
6 participants