views:

1012

answers:

8

Surprisingly I was only able to find one previous question on SO about this subject, and I'd just like to get the community "Vote of Confidence" (or not!) on my approach.

The way I see it is thus:

  • use Debug.Assert to state things you EXPECT would be true. This would be used when we are in complete control over our environment, for example in a method to verify some pre and post-conditions.
  • use Exceptions when exceptional circumstances arise. Dealing with external resources, i.e. files, databases, networks etc is a no-brainer. But...

It gets a little murky in the following scenario. Please note that this is a CONTRIVED EXAMPLE for illustration only!

Say we have class MyClass, which has a public property MyMode and a method GetSomeValueForCurrentMode(). Consider MyClass as one intended to be shipped (release built) in a library for use by other developers.

We expect MyMode to be updated by external users of this class. Now, GetSomeValueForCurrentMode() has the following logic:

switch(MyMode)
{
case Mode.ModeA:
return val1;
case Mode.ModeB:
return val2;
default:
//Uh-uh this should never happen

}

What I'm getting at here is that the user of MyClass has left it in an invalid state. So what should we do?

In the default, should we Debug.Assert or throw new InvalidOperationException (or other) ?

There is one mantra that says we should not trust users of our classes. If we choose Debug.Assert and built MyClass as a release build (thereby removing the Debug Asserts) the user of the class wouldn't get helpful information that they had left it in an invalid state. But it's sort of contrary to the other mantra which says only throw exceptions when things completely out of your control happen.

I find I go round in circles with this - one of those programming debates that don't seem to have a definitive 'correct' answer. So let's put it to the vote!

Edit: I noticed this response in a related SO question (http://stackoverflow.com/questions/117171/design-by-contract-tests-by-assert-or-by-exception):

"The rule of thumb is that you should use assertions when you are trying to catch your own errors, and exceptions when trying to catch other people's errors. In other words, you should use exceptions to check the preconditions for the public API functions, and whenever you get any data that are external to your system. You should use asserts for the functions or data that are internal to your system. "

To me, this makes sense, and can be coupled with the 'Assert then throw' technique outlined below.

Thoughts welcome!

A: 

It depends on language, is assert if you syntax sugar then you should use it. however in Java asserts need to be turn on for this to work, so exception is better. However its always better to have specific exception, so here it should be IllegalStateException.

01
A: 

In .Net, Debug class methods are not included in your program when you do make a release build, so you will have to throw an exception if this code makes it to production.

Gerrie Schenck
+2  A: 

My use of asserts follows ideas from design by contract. Basically, you assert that incoming parameters, globals variables and other state information is valid as you enter your function, and assert the return values and state is also valid as you leave. If you get a failed assert at the start, it is a bug in the calling code, if you get it at the end, the bug is in this code. These are pre-conditions and post conditions.

An assert in your switch statement is only really useful if you have checked your pre-conditions on entry. If you arrive at an unacceptable state in the middle of your function in this scenario, it is a failing in your function or a called function. Personally, I would stick with an assert here as it is not an exception. As you imply, exceptions relate to resources, not bugs. You can however create a custom assertion handler that exists in the release build and throws an exception on failure, to give your program an opportunity to return to a stable state.

Shane MacLaughlin
Stick with Asserts even if MyClass is packaged and released as a library for other developers?
Duncan
Yup. If MyClass is in an unexpected state, it is either down to an internal bug or mis-use (not meeting pre-conditions). I wouldn't throw exceptions for either of these. For the former, I would return a bad parameter error. For the latter I include asserts in a release diagnostics mode.
Shane MacLaughlin
+5  A: 

First, MyClass being valid should be, of course, expressed by MyClass's invariant.

Second, you say "We expect MyMode to be updated by external users of this class" - of course the setter of this mode should have the typical design-by-contract form (as any public function):

  void Setter(mode m)
  {
    // INVARIANT ASSERT (1)
    // PRECONDITION ASSERTS (uses "m") (2)

    // BODY (3)

    // POSTCONDITION ASSERTS (if any) (4)
    // INVARIANT ASSERT (5)
  }

In (5) you would fail with a screaming assertion violation that the invariant doesn't hold. But in (2) you would fail, earlier, because the mode m passed is invalid. This would send a clear message to the user and thus solves your problem.

And please don't tell me that the mode field is public and users change it without any control whatsoever.

Edit: About assertions and release mode, see also:

Daniel Daranas
+2  A: 

Often both: Assert, then throw.

You assert because you want to notify developers of a mistaken assumption during development.

You throw because if this happens in a release build, you need to make sure the system doesn't continue processing while in a bad state.

The desired reliability characteristics of your system may affect your choice here, but 'assert then throw' is often a useful strategy, I think.

Brian
Thanks, that makes a lot of sense and is similar to my thinking. I'll leave it up to the SO community to decide if they agree!
Duncan
Brian, I Disagree. The assertion is there to help you test your code. If you slip past the assertion in production, it means you have an untested code path, and there is no guarantee that your exception handling further down the stack is tested either.
Carl Seleborg
True. Again the reliability characterics of the system come into play. If you're working on a web server you may have designed and reviewed the exception-handling of the system to death (to keep server uptime but allow for things to go wrong with one message/session), in which case you may have...
Brian
...the reasonable expectation that the exception handler is definitely well-tested (but many of the code paths through the meat of the product are not as well tested).
Brian
+3  A: 

I basically agree with the conclusion of your own question: if Alice's code detects a mistake Alice made, it's a case for Assert (and assertions should be left on in production code, unless performance dictates otherwise). If Alice's code detects a mistake in Eve's code, it's a case for Exceptions, assuming that Alice and Eve are on opposite sides of your bug-tracking software.

Now that's a general rule of thumb. Assertions, in a slightly modified form, can also be used as a "heads-up, developer!" mechanism (and then they should not be called "ASSERT" but "HEADS_UP" or something similar). What if your company develops a client/server product, and the server sends invalid data to the client? If you're a client programmer, you feel like treating it as external data (it is Eve's data that is kaputt) and you want to throw an exception. But a "softer" assertion, which makes Visual Studio's debugger halt right there, can be a very good way to detect those problems really early and report it to the server team. In a real installation, it could very well be Mallory tempering with the data between Eve and Alice, but most of the time it's really a bug by one of your colleagues, and you want to see it when it happens - that's why I call them "heads-up" assertions: they don't replace exceptions, but they give you a warning and a chance to inspect the problem.

Carl Seleborg
+4  A: 

I agree with most people here and follow Design-by-Contract. You should try and differentiate very clearly between requirements in deployed code (Contracts) and figuring out expected state during design (Debugging Assertions).

You should ALWAYS throw contract assertions as exceptions (as they should always be exceptional). There are mechanisms built in to most frameworks for catching debug assertions. But at runtime you should always throw an exception.

I use a custom library to help with this (in C#/VB.NET). I recently put up it up on Codeplex (http://www.contractdriven.com/) if you're interested in how this works in practice.

A side benefit of this is that as you start using DbC more regularly, you seldom need to use debugging assertions as there are already explicit guarantees written in to your code, so it's actually difficult to get in to an invalid state.

So the question in your original post... "What I'm getting at here is that the user of MyClass has left it in an invalid state. So what should we do?"...should never arise.

You may never need to debug anything again! ;-)

ChrisV
Chris, throwing a "contract broken" exception (which can only indicate a bug) is not "fail fast", it's "fail medium fast": lots of stack unwinding has to happen first. Aborting right there and then when you detect the breach of contract would be the real "fail fast".
Carl Seleborg
Y. Sorry Everyone. I Removed reference to "Fail Fast". If you want to FailFast you can use Environment.FailFast() in C# rather than throw an exception. But just be aware your finally blocks won't run.
ChrisV
A: 

I would say: use an exception if the bug is in someone else's code, or in code in a different subsystem (whether you wrote it or not). Also use an exception if there is an error in data that came from an external source such as a file.

Sometimes, you don't know whether the data came from an external source or not. If security is important and there's any possibility that you're dealing with external data that has not been verified correct, use an exception. If security is not important then I would use performance as a tiebreaker: might this code be executed in a tight loop? If so consider using an assert, as you'll get better performance in a Release build. Otherwise, use an exception.

Qwertie