How can I make this Dictionary TryGetValue code more readable?

Question

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.
}

Answer 1

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) 
{

}

Answer 2

Duality.

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

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

Answer 3

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

if ( IdIsNew() || IdChanged())

Answer 4

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;
}

Answer 5

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.
}

Answer 6

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")

Answer 7

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.
}

Answer 8

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.

Answer 9

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

Answer 10

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.