What is the downside to Conditional Attributes as opposed to #if/#endif?

Question

My code base has a lot of #if DEBUG/#endif statements that mostly have assertion type logic that I'm not brave enough to run in production.

[Conditional("DEBUG")]
public void CheckFontSizeMath()
{
  //This check must not block SellSnakeOil() in production, even if it fails.
  if(perferredSize+increment!=increment+preferredSize)
    throw new WeAreInAnUnexpectedParallelUniverseException();
}

Will I regret changing all these over to the new way of doing things?

UPDATE: I'm looking for the difference between characteristics of two similar but different syntax styles for doing Assertions. I understand there is a world of other ways to do demonstrate applications work, and I do those, too. I'm not ready to give up Assertions altogether.

Also I updated the method name for a realistic debug-release-only scenario.

Answer 1

There's no problem doing this. The more current approach is to use Code Contracts.

Answer 2

Write tests! Then you don't have to be afraid that your code is doing something that you don't expect when you start making these changes.

Once you get your code under test you can refactor to your heart's content.

Code without tests, even written yesterday, is legacy code... Get a copy of Working Effectively with Legacy Code, it's an amazing book for managing legacy code; 32 reviews with 5 stars.

Answer 3

Other options that you have is to either use Code Contracts as suggested by Hans or to use the assertion methods in the System.Diagnostics.Debug namespace, e.g.

Debug.Assert(1 + 1 == 2, "Math went wrong!");

This check is automatically removed when building the Release version.

Answer 4

The downsides:

1. It's not as robust as hard checks and exceptions that function independently of build configuration. It's a good idea to take into account the fact that most code should handle errors in release mode.

2. Behaviour has the potential to differ between configurations. If you're careless, you may end up putting some non debug logic in a conditional method or adding side effects so that your production code does not function correctly (basically the same thing as accidentally calling methods inside ASSERT() macros in C++; side effects suck!) The biggest thing here is exceptions. Conditional methods do not allow you to return values for whatever reason so that eliminates one potential error. However, they do allow you to throw exceptions which can dramatically change the path taken on an error condition occurring. This makes things harder to understand and maintain.

3. At the call site, it is not obvious that conditional methods are being called. You just see "DoSomething();". I prefer to name my conditional methods via a convention where I know it's a conditional; e.g. DEBUG_SanityCheckBufferContents().

4. If you don't #if out the method body, your debug code will still exist and it can be inspected/invoked via reflection amongst other things. The method's IL is still emitted, the call sites are not.

5. It makes unit testing very difficult, as behaviour differs between configurations. This is along the same lines as point 2, but I added it as a separate point because it's really sucky from a testing perspective. Most developers will run tests in debug or release prior to checking in, but not both. Our CI runs both sets of tests, so it's galling to check something in after passing all of the tests only to find you've broken the debug configuration but the tests passed because you ran the release build (or vice versa).

In short my rules of thumb are:

• Prefer hard checks & exceptions with unit tests. Use conditional methods where this is not possible (e.g. performance critical checks)
• Name conditional methods clearly
• Pragma #if out the code in the method body
• Be very wary of altering program control flow in conditional methods (I personally prefer Debug.Assert instead of an exception when using conditional methods, but I've seen loads of other programmers use exceptions, so this is probably up for debate)