Today I was working in getting some information out of custom attributes which decorated enumeration values. It turns out that those values are nothing more than fields (Nay!! go on reading). And since getting information out of an attribute uses reflection techniques and we all have been told millions of times since it was invented that "reflection is slow" and being a relatively high-frequency used feature, I jumped onto adding caching to it.
I do not know what I was thinking of, but storing the attribute itself in a static dictionary which key was going to be the field represented by the enumeration value, sounded like a good idea to me*.
I do not remember the source (maybe CLR Via C#) but I had heard that using Type or a MemberInfo derivative it is a bit heavy when it comes to be using it as a key, and there are already a bunch of light-weighted structs for that purpose: the runtimeXhandle family (RuntimeMethodHandle, RuntimeTypeHandle@ and RuntimeFieldHandle).
I already had some "dictionaries" to store information keyed by those handles (only for types and methods) but I thought that implementing another one for fields would be trivial.
And then, suddenly, 4 more sins were added to my personal pool of sins committed when programming:
- do not rethink twice if what I was doing was the simplest thing that I could come with
- do more than one small thing at a time
- thinking that something is so trivial that does not have to be unit tested
- over-confidently checking-in untested code
Of the four, the more deadly one is the last one (closely followed by #3) but, in my discharge, I have to admit that the outcome is such a weird one that my unawareness was justified (at least I like to think so). Besides there are other guys with some heavy sins on their baggage too: the ones designing the framework and the ones not properly documenting its behavior.
I implemented the trivial dictionary class with this signature in his inner repository: IDictionary<RuntimeFieldHandle, T> and the wrapping class is in charge to invoke the FieldHandleProperty on the FieldInfo instance which value wants to be retrieved or stored.
As it was a very small change, I did not feel compelled to unit test it yet and I did some other changes that relied in the feature that I had just added. I quickly check-in so that my fellow could have the code and went myself to manually test it.
It blew up big time. Otherwise I would not be writing this post, right?
From what I learnt, some reflections were made:
- If it is a FieldInfo, it is a FieldInfo and all methods and properties should be available for invoking
- If, for whatever reason, it is decided that, instead of another type that "hides" the operations not supported, they reuse an existing type, all the operations that cannot be performed must be clearly documented. You get no mention of the matter in MSDN
- .Net framework designers, developers, testers and technical writer are a bunch of very talented fellows. But they are people and they make mistakes like you me, but a broader collective of users get exposed to their mistakes
- for myself, I made several sins. I expect I never made those mistakes again (naaaah, they are good fun to listen, aren't they?)
* it turns out that the simples solution is having some sort of static dictionary keyed by the value of the enumeration itself (blush)