FxCop backlog themes: Exceptions

Since I started monitoring traffic on this blog a little more closely about a week ago, I had the unexpected surprise that the posts on HTML encoding and server vs. client cultures were getting a lot more hits than I expected. I had been planning on starting a series of “how to” posts on those topics this weekend, but that was before David Kean from the FxCop team was kind enough to direct a bunch of folks my way with a post about my recent FxCop posts. Since it would seem that I’ve now got quite a few new subscribers who are interested in FxCop, I figured I might as well continue on that theme for a while…



Jumping the gun


I’ve got exception stuff on the brain this morning, so that’s what I’m going to write about. However, if you have decided to tackle an FxCop backlog, exceptions are probably not the first major theme you should cover. Then again, even assuming that my recent posts have inspired you to immediately start a backlog winnowing project, you should still be at least a month away from covering any major theme. I’m going to try to write about the much easier disposition and finalization topic within the next couple of weeks. Heck, it might even happen tomorrow morning if I don’t get coerced into some sort of housework. In the meantime, just keep in mind that the sequence in which I end up covering the topics here isn’t necessarily the sequence in which you’ll want to cover them in your backlog process.



Starting from scratch


Unless your team already has very specific guidelines in place around the generation and handling of exceptions (along with training and code reviews to reinforce those guidelines), it’s pretty safe to assume that there is a very wide variety of exception treatment in your code base. In addition, the developers on your team likely have similarly disparate understandings of when and how exceptions ought to be generated and handled. Instead of jumping right into covering the existing FxCop rules around exceptions, it would probably be a much better idea to start with some training around the basics of exceptions. Chances are excellent that individual developers will be carrying quite a bit of baggage based on their experiences with exception treatment in other languages or platforms, and you will most likely need to devote quite a bit of time to dispelling inappropriate preconceptions.


Here are a few resources that you may wish to consult during your exception training:



If you’re concerned about globalization and localization issues, you might also want to have a look at my post on client vs. server cultures. The disparity between application user and administrator languages can present challenges when emitting localized exception messages for any application, and it’s something you may wish to consider as you develop your internal exception generation guidelines, even if you don’t happen to be writing web applications.


The built-in rules


As part of the exceptions theme, you’ll want to start activating the FxCop rules that touch on exception generation and handling. The following table lists the rules available in FxCop 1.35 and Visual Studio 2005 static analysis:


Category Rule
Design DoNotCatchGeneralExceptionTypes
ExceptionsShouldBePublic1
Security CatchNonClsCompliantExceptionsInGeneralHandlers1
WrapVulnerableFinallyClausesInOuterTry
Usage DoNotRaiseExceptionsInFilterBlocks
DoNotRaiseReservedExceptionTypes
InstantiateArgumentExceptionsCorrectly
RethrowToPreserveStackDetails


I suppose that it could be argued that Usage.DoNotIgnoreMethodResults might also be worth including while activating exception rules. If your team is resistant to the idea of throwing exceptions instead of using failure return codes, then a bit of time cleaning up violations of this rule might be just the thing to convince them. Otherwise, it’s probably safe to activate this rule whenever it otherwise seems reasonable during your backlog process. If you’re following the guidelines, you’ll probably want to fix most violations of this rule by modifying the invoked method to throw exceptions rather than return a status code, which is a breaking change for public methods, and scheduling breaking changes can be a wee bit tricky.



Custom rules


If you’ve read the design guidelines concerning exceptions, it should be pretty obvious that the above eight rules don’t exactly cover all the potential problems for which you might want to screen. Here are a few custom rules for exceptions that we’re either already using at FinRad or will be introducing once we get a little further along in the backlog cleanup:


  • Do not throw System.NotImplementedException
    Pretty obvious, although you’ll occasionally need to create temporary exclusions, particularly for when someone checks in a class skeleton to be fleshed out by somebody else. Also, as a bit of side note, when you introduce this guideline, you might want to stress that the exception signifies that the code is not implemented yet, not that it’s not implemented here. For “not implemented here”, you should be using System.NotSupportedException instead.
  • Methods that throw System.NotSupportedException should not include other instructions
    Simple as it might seem, this rule is a bit tricky to author since the VB compiler has the nasty habit of adding extra instructions (e.g.: return statements) to methods. Also, one needs to keep in mind that a custom message might be provided for the exception, and that message might be retrieved via a method call.
  • Throw System.ComponentModel.InvalidEnumArgumentException for an invalid enum argument value
    Not really relevant to the rule itself, but it’s really annoying that this particular exception lives in the System.ComponentModel namespace rather than System.
  • An argument exception should be thrown directly from the method whose argument is being validated
    I had hoped that the parameter name verification on Usage.InstantiateArgumentExceptionsCorrectly would cover this issue but, after looking at the rule logic via Reflector, I’ve decided that a custom rule is going to be need for this one. I’m planning on activating it at the same time as Design.ValidateArgumentsOfPublicMethods (which will definitely be happening before we move to Orcas <gdr>).
  • Use recommended exception logging methods
    In order to make logged exceptions more noticeable, we decided to create a new helper method that is capable of writing exception details to both a central repository and whatever target location was previously used. The decentralized logging problem was the consequence of an application architecture that you may not share, so this might not be particularly relevant for your team. However, if you do have similarly decentralized logging, you may want to consider a similar approach. If you do decide to add a special exception logging method, then you’ll probably also want to add a custom rule to verify that it’s being used.
  • Review use of System.Diagnostics.Debug methods
    Some folks have a tendency to use Debug.Assert or Debug.Fail where they really ought to be throwing exceptions.
  • Review catch without throw
    Last but definitely not least, this is our “do not eat exceptions” rule. Unfortunately, fixing an undesirable violation of this rule can be very time-consuming, but discovering places where an application is hiding exceptions has tremendous impact on reliability, and the short-term cost is well worth the long-term benefit.

Personally, I’d love to see at least the more generally useful rules from the above list make it into the rule set shipped by Microsoft. I seem to remember seeing something from the code analysis team that promised some new rules around exceptions for Orcas, but I can’t find it anymore, and I haven’t seen any new exception rules either. It’s a pity, because I was really hoping to get rid of my custom rules, particularly the less than ideal verification for other instructions in methods that throw System.NotSupportedException…






1

2 thoughts on “FxCop backlog themes: Exceptions”

  1. One of the exception rules that has always puzzled me is CA2208 in regards to property getters and setters and exceptions. When testing for a null or empty string in a property setter, I throw ArgumentNullException(“Test”, “…”) where Test is the name of the property, but FxCop wants me to change this to ArgumentNullException(“value”). ArgumentNullException isn’t my first choice, but I can’t find a more appropriate exception to use in this case.

  2. “value” is actually the name of the argument in the setter method (take a look in Reflector to see for yourself), and not all languages support the property syntax, so it does make more sense to use the argument name rather than the property name in the ArgumentException.  When callers have to resort to using “set_YourProperty(value)”, an argument exception for “YourProperty” isn’t necessarily going to make a whole lot of sense.  Conversely, for those who can use the “YourProperty = value” syntax, the name “value” has implicit meaning and isn’t bound to create any problems in understanding the meaning of the exception message.

    As for what to throw for empty strings, the current guideline from Microsoft seems to be to throw a vanilla ArgumentException (see blogs.msdn.com/…/faq-what-exception-should-i-throw-instead-of-the-reserved-exceptions-found-by-donotraisereservedexceptiontypes.aspx).  However, a lot of folks (including me) at FinRad aren’t always very comfortable with that, so we’re using an ArgumentEmptyException that inherits directly from ArgumentException and is meant to be thrown for both string and collection arguments that are inappropriately empty.

Leave a Reply

Your email address will not be published. Required fields are marked *


*

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>