18

I find myself often looking up questions online, and many solutions include dictionaries. However, whenever I try to implement them, I get this horrible reek in my code. For example every time I want to use a value:

int x;
if (dict.TryGetValue("key", out x)) {
    DoSomethingWith(x);
}

That's 4 lines of code to essentially do the following: DoSomethingWith(dict["key"])

I've heard that using the out keyword is an anti pattern because it makes functions mutate their parameters.

Also, I find myself often needing a "reversed" dictionary, where I flip the keys and values.

Similarly, I often would like to iterate through the items in a dictionary and find myself converting keys or values to lists etc to do this better.

I feel like there's almost always a better, more elegant way of using dictionaries, But I'm at a loss.

  • 7
    Other ways may exist but I generally use a ContainsKey first before trying to get a value. – Robbie Dee Aug 27 at 15:53
  • 2
    Honestly, with a few exceptions, if you know what your dictionary keys are ahead of time, there probably is a better way. If you don't know, the dict keys shouldn't generally be hard-coded at all. Dictionaries are useful for working with semi-structured objects or data where the fields are at least mostly orthogonal to the application. IMO, the closer those fields get to being relevant business/domain concepts, the less helpful dictionaries become for working with those concepts. – svidgen Aug 27 at 17:08
  • 6
    @RobbieDee: you have to be careful when you do that though, as doing so creates a race condition. It's possible the key could be removed between calling ContainsKey and getting the value. – whatsisname Aug 27 at 18:24
  • 12
    @whatsisname: Under those conditions, a ConcurrentDictionary would be more suitable. Collections in the System.Collections.Generic namespace are not thread-safe. – Robert Harvey Aug 27 at 20:49
  • 3
    A good 50% of the time I see someone using a Dictionary, what they really wanted was a new class. For those 50% (only), it's a design smell. – BlueRaja - Danny Pflughoeft Aug 28 at 0:59
23

Dictionaries (C# or otherwise) are simply a container where you look up a value based on a key. In many languages it's more correctly identified as a Map with the most common implementation being a HashMap.

The problem to consider is what happens when a key does not exist. Some languages behave by returning null or nil or some other equivalent value. Silently defaulting to a value instead of informing you that a value does not exist.

For better or for worse, the C# library designers came up with an idiom to deal with the behavior. They reasoned that the default behavior for looking up a value that does not exist is to throw an exception. If you want to avoid exceptions, then you can use the Try variant. It's the same approach they use for parsing strings into integers or date/time objects. Essentially, the impact is like this:

T count = int.Parse("12T45"); // throws exception

if (int.TryParse("12T45", out count))
{
    // Does not throw exception
}

And that carried forward to the dictionary, whose indexer delegates to Get(index):

var myvalue = dict["12345"]; // throws exception
myvalue = dict.Get("12345"); // throws exception

if (dict.TryGet("12345", out myvalue))
{
    // Does not throw exception
}

This is simply the way the language is designed.


Should out variables be discouraged?

C# isn't the first language to have them, and they have their purpose in specific situations. If you are trying to build a highly concurrent system, then you cannot use out variables at the concurrency boundaries.

In many ways, if there is an idiom that is espoused by the language and core library providers, I try to adopt those idioms in my APIs. That makes the API feel more consistent and at home in that language. So a method written in Ruby isn't going to look like a method written in C#, C, or Python. They each have a preferred way of building code, and working with that helps the users of your API learn it more quickly.


Are Maps in General an Anti-pattern?

They have their purpose, but many times they may be the wrong solution for the purpose you have. Particularly if you have a bi-directional mapping you need. There are many containers and ways of organizing data. There are many approaches that you can use, and sometimes you need to think a bit before you pick that container.

If you have a very short list of bi-directional mapping values, then you might only need a list of tuples. Or a list of structs, where you can just as easily find the first match on either side of the mapping.

Think of the problem domain, and pick the most appropriate tool for the job. If there isn't one, then create it.

  • 4
    "then you might only need a list of tuples. Or a list of structs, where you can just as easily find the first match on either side of the mapping." -- If there are optimizations for small sets, they should be made in the library, not rubber-stamped into user code. – Blrfl Aug 27 at 20:50
  • If you choose a list of tuples or a dictionary, that is an implementation detail. It's a question of understanding your problem domain and using the right tool for the job. Obviously, a list exists and a dictionary exists. For the common case, dictionary is correct, but maybe for one or two applications you need to use the list. – Berin Loritsch Aug 27 at 21:18
  • 3
    It is, but if the behavior is still the same, that determination should be in the library. I've run across containers implementations in other languages that will switch algorithms if told a priori that the number of entries will be small. I agree with your answer, but once this has been figured out, it should be in a library, maybe as a SmallBidirectionalMap. – Blrfl Aug 27 at 22:52
  • 5
    "For better or for worse, the C# library designers came up with an idiom to deal with the behavior." – I think you hit the nail on the head: they "came up" with an idiom, instead of using an existing widely-used one, which is an optional return type. – Jörg W Mittag Aug 28 at 8:46
  • 3
    @JörgWMittag If you're talking about something like Option<T>, then I think that would make the method much harder to use in C# 2.0, because it didn't have pattern matching. – svick Aug 28 at 16:21
21

Some good answers here on the general principles of hashtables/dictionaries. But I thought I'd touch on your code example,

int x;
if (dict.TryGetValue("key", out x)) 
{
    DoSomethingWith(x);
}

As of C# 7 (which I think is around two years old), that can be simplified to:

if (dict.TryGetValue("key", out var x))
{
    DoSomethingWith(x);
}

And of course it could be reduced to one line:

if (dict.TryGetValue("key", out var x)) DoSomethingWith(x);

If you have a default value for when the key doesn't exist, it can become:

DoSomethingWith(dict.TryGetValue("key", out var x) ? x : defaultValue);

So you can achieve compact forms by using reasonably recent language additions.

  • 1
    Good call on the v7 syntax, nice little option to have to cut out the extra definition line while allowing the use of var +1 – BrianH Aug 28 at 0:07
  • 2
    Note also that if "key" is not present, x will be initialized to default(TValue) – Peter Duniho Aug 28 at 4:16
  • Might be nice as a generic extension, too, invoked like "key".DoSomethingWithThis() – Ross Presser Aug 28 at 20:11
  • I greatly prefer designs that use getOrElse("key", defaultValue) Null object is still my favorite pattern. Work that way and you don't care if TryGetValue returns true or false. – candied_orange Aug 29 at 6:01
  • @candied_orange, then add public static TValue GetOrElse<TKey, TValue>(this IDictionary<TKey, TValue> dict, TKey key, TValue defaultValue) => dict.TryGetValue(key, out var x) ? x : defaultValue); to a static class in your project and you've got that :) – David Arno Aug 29 at 15:26
12

This is neither a code smell nor an anti-pattern, as using TryGet-style functions with an out parameter is idiomatic C#. However, there are 3 options provided in C# to work with a Dictionary, so should you be sure you are using the correct one for your situation. I think I know where the rumor of there being a problem using an out parameter comes from, so I'll handle that at the end.

What features to use when working with a C# Dictionary:

  1. If you are sure the key will be in the Dictionary, use the Item[TKey] property
  2. If the key should generally be in the Dictionary, but it is bad/rare/problematic that it is not there, you should use Try...Catch so that an error will be raised and then you can try to handle the error gracefully
  3. If you are not sure the key will be in the Dictionary, use TryGet with the out parameter

To justify this, one need only refer to the documentation for Dictionary TryGetValue, under "Remarks":

This method combines the functionality of the ContainsKey method and the Item[TKey] property.

...

Use the TryGetValue method if your code frequently attempts to access keys that are not in the dictionary. Using this method is more efficient than catching the KeyNotFoundException thrown by the Item[TKey] property.

This method approaches an O(1) operation.

The whole reason TryGetValue exists is to act as a more convenient way to use ContainsKey and Item[TKey], while avoiding having to search the dictionary twice - so pretending it doesn't exist and doing the two things it does manually is a rather awkward choice.

In practice, I rarely have ever used a raw Dictionary, due to this simple maxim: choose the most generic class/container that gives you the functionality you need. Dictionary was not designed to look up by value rather than by key (for example), so if that is something you want it may make more sense to use an alternate structure. I think I may have used Dictionary one time in the last year-long development project I did, simply because it was rarely the right tool for the job I was trying to do. Dictionary is certainly not the Swiss Army knife of the C# toolbox.

What's wrong with out parameters?

CA1021: Avoid out parameters

Although return values are commonplace and heavily used, the correct application of out and ref parameters requires intermediate design and coding skills. Library architects who design for a general audience should not expect users to master working with out or ref parameters.

I'm guessing that's where you heard that out parameters were something like an anti-pattern. As with all rules, you should read closer to understand the 'why', and in this case there is even an explicit mention of how the Try pattern does not violate the rule:

Methods that implement the Try pattern, such as System.Int32.TryParse, do not raise this violation.

10

There are at least two methods missing from C# dictionaries that in my opinion clean up code considerably in a lot of situations in other languages. The first is returning an Option, which lets you write code like the following in Scala:

dict.get("key").map(doSomethingWith)

The second is returning a user-specified default value if the key isn't found:

doSomethingWith(dict.getOrElse("key", "key not found"))

There is something to be said for using the idioms a language provides when appropriate, like the Try pattern, but that doesn't mean you have to only use what the language provides. We're programmers. It's okay to create new abstractions to make our specific situation easier to understand, especially if it eliminates a lot of repetition. If you frequently need something, like reverse lookups or iterating through values, make it happen. Create the interface you wish you had.

  • I like the 2nd option. Maybe I'll have to write an extension method or two :) – Adam B Aug 27 at 17:26
  • 2
    on the plus side c# extension methods means you can implement these yourself – jk. Aug 28 at 9:46
5

That's 4 lines of code to essentially do the following: DoSomethingWith(dict["key"])

I agree that this is inelegant. A mechanism that I like to use in this case, where the value is a struct type, is:

public static V? TryGetValue<K, V>(
      this Dictionary<K, V> dict, K key) where V : struct => 
  dict.TryGetValue(key, out V v)) ? new V?(v) : new V?();

Now we have a new version of TryGetValue that returns int?. We can then do a similar trick for extending T?:

public static void DoIt<T>(
      this T? item, Action<T> action) where T : struct
{
  if (item != null) action(item.GetValueOrDefault());
}

And now put it together:

dict.TryGetValue("key").DoIt(DoSomethingWith);

and we're down to a single, clear statement.

I've heard that using the out keyword is an anti pattern because it makes functions mutate their parameters.

I would be slightly less strongly worded, and say that avoiding mutations when possible is a good idea.

I find myself often needing a "reversed" dictionary, where I flip the keys and values.

Then implement or obtain a bidirectional dictionary. They are straightforward to write, or there are plenty of implementations available on the internet. There are a bunch of implementations here, for example:

https://stackoverflow.com/questions/268321/bidirectional-1-to-1-dictionary-in-c-sharp

Similarly, I often would like to iterate through the items in a dictionary and find myself converting keys or values to lists etc to do this better.

Sure, we all do.

I feel like there's almost always a better, more elegant way of using dictionaries, But I'm at a loss.

Ask yourself "suppose I had a class other than Dictionary that implemented the exact operation that I wish to do; what would that class look like?" Then, once you've answered that question, implement that class. You're a computer programmer. Solve your problems by writing computer programs!

  • Thanks. Do you think you could go into a bit more explanation of what the action method is actually doing? I’m new to actions. – Adam B Aug 29 at 4:07
  • 1
    @AdamB an action is an object that represents the ability to call a void method. As you see, on the caller side we pass the name of a void method whose parameter list matches the generic type arguments of the action. On the caller side the action is invoked like any other method. – Eric Lippert Aug 29 at 14:19
  • 1
    @AdamB: You can think of Action<T> as being very similar to interface IAction<T> { void Invoke(T t); }, but with very lenient rules about what "implements" that interface, and about how Invoke may be invoked. If you want to know more, learn about "delegates" in C#, and then learn about lambda expressions. – Eric Lippert Aug 29 at 20:20
  • Ok I think I got it. So the action let’s you put a method in as the parameter for DoIt(). And calls that method. – Adam B Aug 29 at 20:35
  • @AdamB: Exactly right. This is an example of "functional programming with higher-order functions". Most functions take data as arguments; higher-order functions take functions as arguments and then do stuff with those functions. As you learn more C# you'll see that LINQ is entirely implemented with higher-order functions. – Eric Lippert Aug 29 at 20:51
4

The TryGetValue() construct is only necessary if you don't know whether "key" is present as a key within the dictionary or not, otherwise DoSomethingWith(dict["key"]) is perfectly valid.

A "less dirty" approach might be to use ContainsKey() as a check instead.

  • 6
    "A "less dirty" approach might be to use ContainsKey() as a check instead." I disagree. As suboptimal as TryGetValue is, at least it makes it hard to forget to handle the empty case. Ideally, I wish this would just return an Optional. – Alexander Aug 27 at 17:37
  • 1
    There's a potential problem with the ContainsKey approach for multithreaded applications, which is time of check to time of use (TOCTOU) vulnerability. What if some other thread deletes the key between the calls to ContainsKey and GetValue? – David Hammen Aug 28 at 10:41
  • 2
    @JAD Optionals compose. You can have an Optional<Optional<T>> – Alexander Aug 28 at 13:49
  • 2
    @DavidHammen To be clear, you would need TryGetValue on ConcurrentDictionary. The regular dictionary wouldn't be synchronized – Alexander Aug 28 at 13:50
  • 2
    @JAD No, (Java's optional type blows, don't get me started). I'm talking more abstractly – Alexander Aug 28 at 13:50
2

Other answers contain great points, so I won't restate them here, but instead I'll focus on this part, which seems to be largely ignored so far:

Similarly, I often would like to iterate through the items in a dictionary and find myself converting keys or values to lists etc to do this better.

Actually, it's pretty easy to iterate over a dictionary, since it does implement IEnumerable:

var dict = new Dictionary<int, string>();

foreach ( var item in dict ) {
    Console.WriteLine("{0} => {1}", item.Key, item.Value);
}

If you prefer Linq, that too works out just fine:

dict.Select(x=>x.Value).WhateverElseYouWant();

All in all, I don't find Dictionaries to be an antipattern - they're just a specific tool with specific uses.

Also, for even more specifics, check out SortedDictionary (uses R-B trees for a more predictable performance) and SortedList (which is also a confusingly named dictionary that sacrifices insertion speed for lookup speed, but shines if you're staring out with a fixed, pre-sorted set). I've had a case where replacing a Dictionary with a SortedDictionary resulted in an order of magnitude faster execution (but it could happen the other way round too).

1

If you feel using a dictionary is awkward it may not be the right choice for your problem. Dictionaries are great but like one commenter noticed, often they are used as a shortcut for something that should have been a class. Or the dictionary itself may be right as a core storage method but there should be a wrapper class around it to provide the desired service methods.

A lot has been said about Dict[key] versus TryGet. What I use a lot is iterating over the dictionary using KeyValuePair. Apparently this is a less commonly known construct.

The main boon of a dictionary is that it is really fast compared to other collections as the number of items grows. If you have to check whether a key exists a lot you may want to ask yourself if your use of a dictionary is appropriate. As a client you should typically know what you put in there and thus what would be safe to query.

-5

IMHO form a purely theoretical point of view, TryGetValue and other ThisAndThat methods is a code smell by itself, it is a method/function that does more than one thing. If methods/functions were composable you would use one to get the value and one to decide what to replace the null with or to handle the exception. The API is flawed in 2019. But it is what it is.

The currently popular concept of Optional wrappers in every OO language is a more modern but just as flawed solution as it just moves the problem somewhere else.

There are much better design approaches

  1. Validate the map/dictionary has the keys that are required in another method/function that is a precondition and do it once, and throw an exception that is basically an illegal argument mandatory key is missing with the name of the key so everyone knows exactly how to fix the problem.

  2. Provide a DEFAULT map/dictionary that you copy the values from, then update that with whatever you are putting in there and you always are sure that the keys exist with a proper default and you can just let errors happen because they are indeed exceptional. Or use the DEFAULT map as an overlay map to fill in a sparse one with required keys with the proper default values.

The goal is to just avoid that TryGetValue mess, it is a code smell because it does more than one thing at time.

  • 1
    "does more than one thing" - It doesn't. It gets you the information associated with a particular key in the map, whether that information is "key not associated" or "this is the associated value". That the Try* pattern splits that information into two parts is unfortunate, but not critical, and saying that Optional and its ilk have the same problem is just completely wrong. – Sebastian Redl Aug 28 at 6:56
  • 4
    This completely misses the point that TryGetValue does a lookup once, whereas your proposed separation does the same lookup twice. – rwols Aug 28 at 6:59
  • I does not say anywhere that Optional did the same thing, doing two things instead of one, I said it just moves the problem somewhere else but thanks for the reading for comprehension is important lesson. The alternative solutions have none of these problems with violating very basic CS 101 tenets. Also my solution does no more than one look up in the implementations I use. – Jarrod Roberson Aug 29 at 19:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.