Friday, March 25, 2011

FxCop: Properties should not return arrays

Did you run FxCop and it reported a “Performance” issue regarding “Properties should not return arrays”?
Well, don’t be in a hurry to fix it just yet.
After an online meeting with Microsoft online tech team, I was surprised to hear them claim that this may pose a performance issue, since developers “must return a copy of the array”, which will cause a performance issue when accessing this property excessively (like via an indexer, in a loop).
They went on later to recommend changing this property into a method, to make it clear for the developer who is using it that he is better off calling the method once and storing it in a local member, rather then using the indexer on the property itself, Or change the property type into a collection. There is even an example at the end of the page of converting the array into a collection, which “resolves” this “performance” issue.
Also, from reading the rule page on MSDN, it seems this issue only refers to a possibly problem of breaking the read-only attribute of an array that is returned as a property, which would unintentionally allow modifications to it. Definately not what I was told in that meeting, and besides - when the array is not read only I don't see the problem here. Isn't it up to the architect to decide what to use, and if the array should or should not be read-only?
Let me give you a few points to think about before complying to this rule:
1. Properties do not serve the same purpose as methods. If you need to serialize this class, a property would be serialized while a method would not – resulting in loss of data upon serialization. This is extremely important when talking about web part properties. Even changing the property into a read only collection will not help, as XmlSerializer will throw an exception on that property.
2. An array does not serve the same functionality as a collection. Especially when reflecting it, or serializing it for use later (JSON serialize / SOAP serialize in web service for example).
3. If you have an existing web part that uses an array as property, DONT CHANGE IT! you cannot change it’s type, nor change it into a method! Doing so will break all your existing web parts in all pages and you will have to remove them and re-configure them, page by page. This goes to web parts, but also to any serialized content you have of existing code.
4. Speaking of performance issue, the author assumes that *if* you build the array on every get_Property access this will have negative performance. But – why would you do that? In most you just return a private array member that should not be read-only, which does not affect performance at all.
5. Even more, by implementing the resolution suggested and converting into a collection, performance will have a far worst negative implication! Working with arrays is with no doubt the fastest way of working with a collection in .NET! It is faster than any other collection type, including Generic collections, in every possible way.
Consider a simple “for” loop, using “items[i]” <—an array will beat any collection any time, any place!
So, by suggesting to fix a non-existent “performance” issue, you actually cause a much greater real “performance” issue on your application.
What I don’t understand is, this rule has been there since 2006 with many people posting comments on its validity, Why is it still out there?
The worst thing about it, with Microsoft SharePoint online hosting, they require your to fix all of these warnings on your code. How can this be a requirement if this is clearly a design consideration and not a clear-cut case of issue to be fixed?
I would suggest you take any tool (FxCop or any other automated tool) as suggestion only, and use your own discretion on when or if to apply these rules.

No comments: