CODEWITHSUNDEEP
c#.netdictionarytrygetvalue
mafu
mafu

Reputation: 32650

How can I make this Dictionary TryGetValue code more readable?

I'd like to test if an id was not yet known or, if it is known, if the associated value has changed. I'm currently using code similar to this, but it is hard to understand for those not familiar with the pattern. Can you think of a way to make it more readable while keeping it short in LOC?

string id;
string actual;
string stored;

if (!someDictionary.TryGetValue (id, out stored) || stored != actual) {
    // id not known yet or associated value changed.
}

Upvotes: 6

Views: 5147

Answers (10)

supercat
supercat

Reputation: 81149

While I recognize that the "try" pattern is necessary, I dislike implementations which require an "out" parameter. It would seem much more useful have functions similar to TryGetValue:

  • TryGetDictValue(dictionary, key) returns null if key is not in dictionary
  • TryGetDictValue(dictionary, key, defaultValue) returns defaultValue if key is not in dictionary
  • TryGetDictValue(dictionary, key, valueReturningDelegate) invokes the supplied delegate if key is not in dictionary and returns its result

In every case, the return type of the result would be that of the dictionary's data.

It's too bad there's no way to sneak into a time machine and make such things be methods of Dictionary. On the other hand, one could implement them as static functions taking a dictionary as the first parameter.

Upvotes: 0

Sam Holder
Sam Holder

Reputation: 32936

I'd prefer a new method:

public bool ShouldSetValue(Dictionary someDictionary, object id,object actualValue)
{
    string stored;

    if (someDictionary.TryGetValue (id, out stored)) 
    {
        if (stored != actualValue)
            return true;
    }
    else
    {
        return true;
    }
}

then in the existing method I'd just:

if (ShouldSetValue(someDictionary,id,actual))
{
     someDictionary[id]=actual;
}

Upvotes: 1

Jake Kalstad
Jake Kalstad

Reputation: 2065

public T GetValue(int id, object actual)
{
  object stored;
 if (someDictionary.TryGetValue (id, out stored) || stored == actual) 
    return stored;
  return new object();
}

Upvotes: 0

Mark Brackett
Mark Brackett

Reputation: 85645

It looks fine to me...reads as easy as any other 2 condition if statement. About the only thing I'd possibly change is to flip the negations for an early exit:

if (someDictionary.TryGetValue(id, out stored) && stored == actual) {
    return;
}
// store new value

I don't see any confusion in it at all, have never thought of it as a particularly troublesome idiom, and humbly suggest that those C# devs confused by it get used to it. It's common, succint, and gives as many LOC to the problem as it deserves. Turning it into 10 lines of code makes it way too important.

If I used it often, an extension method named something like ContainsEqualValue would be appropriate - but I'd use the exact same code in the extension method as you have.

Upvotes: 1

Jay
Jay

Reputation: 57919

If you mean that you have to do this repeatedly, and it is long and ugly, abstract the logic to another class and use an extension method.

public static class DictionaryExtensions
{
    public static DictionaryChecker<TKey,TValue> contains<TKey,TValue>(this IDictionary<TKey,TValue> dictionary, TValue value)
    {
        return new DictionaryChecker<TKey,TValue>(value, dictionary);
    }
}

public class DictionaryChecker<TKey,TValue>
{
    TValue value;
    IDictionary<TKey,TValue> dictionary;

    internal DictionaryChecker(TValue value, IDictionary<TKey, TValue> dictionary)
    {
        this.value = value;
        this.dictionary = dictionary;
    }

    public bool For(TKey key)
    {
        TValue result;
        return dictionary.TryGetValue(key, out result) && result.Equals(value);
    }
}

Now replace your code with:

if(!someDictionary.contains(actual).For(id)){
    // id not known yet or associated value changed.
}

Upvotes: 0

Andrew Anderson
Andrew Anderson

Reputation: 3449

An extension method would be slick:

public static class DictionaryExtensions
{
    public static bool ShouldAddValue<TKey, TValue>(this Dictionary<TKey, TValue> someDictionary, TKey id, TValue actual)
    {
        TValue stored;
        return (!someDictionary.TryGetValue(id, out stored) || !stored.Equals(actual)); 
    }
}

Usage:

someDictionary.ShouldAddValue("foo", "bar")

Upvotes: 0

max
max

Reputation: 34407

You can write an extension method with a good name:

public static class Utility
{
    public static bool ValueChangedOrUnknown(this Dictionary<string, string> dictionary, string id, string actual)
    {
        string stored = null;
        return (!dictionary.TryGetValue(id, out actual) || stored != actual);
    }
}

so later you can use

string id;
string actual;

if (someDictionary.ValueChangedOrUnknown(id, actual) {
    // id not known yet or associated value changed.
}

Upvotes: 5

Stefan Steinegger
Stefan Steinegger

Reputation: 64628

So I would most probably break it up and give it meaningful names. This is more to read, but you don't need much to say in comments:

bool isKnown = someDictionary.TryGetValue (id, out stored);
// can only change when it is known
bool valueChanged = isKnown && stored != actual;

// quite self-explanatory, isn't it?
if (!isKnown || valueChanged) 
{

}

Upvotes: 4

epitka
epitka

Reputation: 17637

wrap each part of the || into its own method or property, than you can write it like this

if ( IdIsNew() || IdChanged())

Upvotes: 3

leppie
leppie

Reputation: 117220

Duality.

if (!(someDictionary.TryGetValue (id, out stored) && stored == actual)) ...

Not sure if it is more readable though... but it's good to know.

Upvotes: 2

Related Questions