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

Payer

Answer 1

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.

Oralla

It isn't quite the same as any 2-condition statement insofar as it requires the pre-declaration of the out parameter. "…get used to it?" It isn't that devs are "confused", per se. Even a veteran programmer who reads left-to-right has to bounce back, forth, and up just to read this, and juggle several elements in the mind while so doing. This isn't a hard task, but it isn't natural, either. This is somewhat procedural code and introduces opportunities for repetition and error. Readability is a legitimate concern and TryGetValue with a bool result is semantically weak. – Stillage

@Jay: Why would you check the declaration of the out variable? It's obviously there (or the compiler would complain), and you shouldn't care what the value was prior (since it's an out param, the value is not used by the function). Is this really all about out instead of the conditional? About the only points I would concede on this statement is that 1.) side-effects are in the conditional, and 2.) the conditionals are order-dependent. bool Try*(out) is a well-established idiom in the BCL that I expect devs to grok, so I forgive the first. – Oralla

@Jay: Your comment is "spot-on". In my opinion, none of the answers address readability. That is why I constributed my suggested code. – Kynan

Bracket: "you're calling into the dictionary twice if the key exists". Good point. Didn't think about that because my goal was lean and clear. When I revisted the code to make the change back to TryGetValue, I realized I could not add to the discussion, so deleted my answer, and upvoted your answer because you removed the not-logic. – Kynan

Answer 2

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

Colb

+1. I hate "out" parameters, and I generally wrap any access to TryGetValue with an extension method (for instance a TryGetValueOrDefault to return the entry directly, or null or 0 if it can't find the entry). – Conferva

Answer 3

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

}

Hundredpercenter

It is initialized, the out keyword in combination with the operator precedence rules ensures that. – Payer

An out parameter must be set inside the function so TryGetValue initialize it. – Valentinavalentine

stored is always initialized after the call to TryGetValue. That's the rule of an out parameter. Obviously, it is not a valid value. – Anfractuous

Yes, but not by the stored value. – Hundredpercenter

The proposed code checks for equality always, so the performance is worse than in the original. Also, it takes many lines - I can't really agree here. – Payer

@Stefan Steinegger: I am not sure what you mean. – Anfractuous

Nice, this is the way code should be written. I immediately understand what is going on without having to understand the details. – Kynan

I remove the comment (about initialization of stored) anyway, because when TryGetValue returns false, it is inverted, so it doesn't compare the stored value... sorry, but it had been too complex to grab quickly. – Hundredpercenter

@Stefan: yes, but the equality check is omitted in that case, so it does not matter. The || operator returns if the left side succeeds. – Payer

@mafutrct: no, performance is not worse, when isKnown is false, the values are not compared, this is ensured by the && operator. – Hundredpercenter

@Stefan: Indeed, so there is probably only a single additional boolean test. I mostly take back my statement about the performance. – Payer

@mafutrct: there is no additional test here. # of compares are the same. – Anfractuous

Answer 4

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

if ( IdIsNew() || IdChanged())

Personify

I wouldn't create new methods unless they will eliminate any potential errors. I would create new methods if it increases readability and maintenance. – Kynan

Answer 5

Duality.

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

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

Anfractuous

-1 for not readable. I would have trouble understanding and fixing these statements. Especially, because stored is out then you use it in the same statement. You are forcing me to read each element in order to understand what is going on. – Kynan

@AMissico: If you don't read each element of a piece of code then how can you ever be sure that you understand what's going on? – Tammitammie

@LukeH: That is exactly why I hate it when people write tons of code that does very little. – Anfractuous

@LukeH: Do you read every word? No, we skim, getting a basic understanding. When that fails, we read. – Kynan

Answer 6

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

Blouin

AddValue is a horrible name for method that does not actually do/mutate anything. CanAddValue might be a better name. – Anfractuous

Nice idea. Maybe rather an extension method for Dictionary? And the naming is weird yet. – Payer

agreed naming was weird. changed it. And yes you could implement it as an extension method on dictionary, which would probably make it a bit nicer... – Blouin

Why not HasValue or KeyHasValue? – Kynan

@AMissico, because it not only checks if it has a value, but if it does have one, it compares that value to another one to see if the value is different. So it is a function to see if we should set the value, so maybe ShouldSetValue would be better.... – Blouin

Answer 7

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

Lac

Answer 8

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

Stillage

@Kynan "Too much code" is subjective. It depends on the domain and usage needs. This is just offered as one approach that favours readability in usage, and would only make sense if this were a check that had to be made in many places. Readability degrades as the number of parameters increases; this is a way to mitigate that degradation, if you see value in so doing. – Stillage

I am in complete agreement with you, which is why I didn't down vote. I favor "lean" so your approach just seems a little heavy because you added ten lines of code, did not make it more readable, and did not change the confusing statement. – Kynan

Answer 9

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

Ventura

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.

Dario

This could still be done as extension method, which is maybe what you mean with your last comment. – Bombproof