Dictionary Misuse

Martin Thwaites

Just found this little gem in a live application that has been wrote (for us) and maintained by an outsourcing partner we use... paraphrased to make it easier to read.

string keyToFind = "asdfgsd44rtgr";

Dictionary dctTemp = GetDictionary();

object theObject = dctTemp.ToList().FirstOrDefault(x => x.key == keyToFind).Value;

This is in a live piece of code, and is run on every application login. As far as I'm aware, the compiler would not be able to optimise this to use dctTemp[keyToFind] and therefore would not be using a HashCode

Henning Dieterichs

This is "wise foresight": In the next update, they can say that they increased their performance by about 20%. I've the feeling, some companies introduce bugs on purpose in their own application to boost the sale for their future updates.

If you find spelling- or grammer-mistakes, please let me know, so that I can correct them (at least for me) - english is not my first language...

PIEBALDconsult

The duece you say.

Harley L Pebley

Martin Thwaites wrote:

run on every application login

So, how often is this? Unless there are a lot of logins/second, don't sweat the small stuff. IMO, the use of ToList (which can simply be deleted) and FirstOrDefault is less of an issue than the fact that they're storing objects. That's a bigger WFT than the optimization from the dictionary specific methods. Don't optimize for speed until you have confirmation it's a hotspot. Optimize first for robustness and maintainability. And be aware, it can't be replaced it with:

object theObject = dctTemp[keyToFind];

For the same behavior, it'd have to be:

object theObject = dctTemp.ContainsKey(keyToFind) : dctTemp[keyToFind] : null;

Martin Thwaites

I wasn't saying it was a hotspot, just that it's poor use of code. It's actually nowhere near to being a hotspot (they have 1 particular click in the application that takes 15 minutes to load the page, and therefore only works when there is a decent internet connection and a high timeout set!). The object thing is FAR worse, however, in the context of how they've wrote the application it does make sense. That is actually one of the things I'm tackling at the moment. This is one of many issues I've found that they've put in (before I started reviewing the code), and I suspect that they will be doing less now. I also appreciate it can't be replaced by exactly what I've said based on the code sample, and in the application itself it would actually need to be different than you've suggested.

Brisingr Aerowing

Harley L. Pebley wrote:

object theObject = dctTemp.ContainsKey(keyToFind) : dctTemp[keyToFind] : null;

Shouldn't that be

object theObject = dctTemp.ContainsKey(keyToFind) ? dctTemp[keyToFind] : null;

? EDIT: And why was this downvoted?

Bill Gates is a very rich man today... and do you want to know why? The answer is one word: versions. Dave Barry Read more at [BrainyQuote](http://www.brainyquote.com/quotes/topics topic_technology.html#yAfSEbrfumitrteO.99)[^]

Harley L Pebley

Yep! No red underline to alert me to the typo. :-D

ely_bob

Martin Thwaites wrote:

string keyToFind = "asdfgsd44rtgr"; Dictionary<string, object> dctTemp = GetDictionary(); object theObject = dctTemp.ToList().FirstOrDefault(x => x.key == keyToFind).Value;

Should be:

object theObject = GetDictionary()[keytoFind];

will return null if it's not in there, and with a dictionary you can only add one unique Key per...See:

using System.Collections.Generic;

namespace ConsoleApplication2
{
class Program
{
static void Main(string[] args)
{
Class1.Sample();
}
static class Class1
{
private static Dictionary GetDictionary()
{
var retval = new Dictionary();
retval.Add("asdfgsd44rtgr", "1");

			//Throws Error
			//System.ArgumentException was unhandled
			//Message=An item with the same key has already been added.
			//Source=mscorlib
			//StackTrace: {ommitted}
			retval.Add("asdfgsd44rtgr", "throws error");
			return retval;
		}
		public static void Sample()
		{
			string keyToFind = "asdfgsd44rtgr";

			Dictionary dctTemp = GetDictionary();

			//object theObject = dctTemp.ToList().FirstOrDefault(x => x.Key == keyToFind).Value;
			object theObject = GetDictionary()\[keyToFind\];
		}


	}

}

}

or did I miss Something ?:confused:

I'd blame it on the Brain farts.. But let's be honest, it really is more like a Methane factory between my ears some days then it is anything else...
-----
"The conversations he was having with himself were becoming ominous."-.. On the radio...

James Lonero

Aren't we in a sad state when we trust our tools more than our minds? Don't feel bad, I am also guilty of such offenses. (I am using MS Word to type this to check may spelling and grammar errors as well as use the thesaurus.)

Florin Jurcovici

And the reliance on tools bites you as you comment on it ...