0

I am currently writing an application that isn't huge but will be used through-out my company of 200+ people and external customers about projects we are working on (not software projects), but there are a fair amount of business rules, along with the implied ones that go with creating, verifying, and maintaining user accounts.

Semi-often I find myself writing code like this

If (A)
{
   foo();
}
else If (B)
{
   foo2();
}
else
{
   throw new exception("Should not happen because XYZ");
}

or

foo()
{
   //do stuff    
   if (D == F)
   {
       throw new exception("Method should not be called if D==F, you forgot how to use this method");
   }    
   //do other stuff
}

Is this code-smell because it implies that my code might break?

Is this good a programming practice for debugging when things WILL go wrong?

Is this being paranoid?

gnat
  • 21,442
  • 29
  • 112
  • 288
TruthOf42
  • 752
  • 1
  • 7
  • 15
  • In your second example you should check for this precondition _before_ doing some stuff. Also, if `D` and `F` are not passed by the caller but instead relying on global state you should make this more explicit by letting the caller pass them (or their dependencies) as an argument. – valenterry May 21 '14 at 21:41

2 Answers2

1

It all depends on how important not clobbering things when someone makes a mistake, or when SNAFU happens, is. I'm going to assume that's important.

For example, you give an example in foo:

throw new exception("Method should not be called if D==F, you forgot how to use this method");

Assuming that foo doesn't set D and F, and the programmer does, and of course that foo isn't supposed to handle the situation where D and F are equal, it's probably the right choice to throw an error. Programmers are idiots, especially the one who uses your code next.

As for checking that something was successful, that's not a bad idea if you are able to identify possible points of failure. For example, assuming a HTTP grabber that returns a (integer flag, data object) tuple for whatever reason:

(flag, page_data) = http_fetch(bleh);
if (flag not in ACCEPTABLE_FLAGS) {
    throw new exception("Today is not a good day.");
}
file_thing.write(page_data.bytes);

Here, the error checking will mean that nobody knocks down your door because the internet went down and as a result your code clobbered the databases with random junk.

If it's your own work, e.g., that you wrote http_fetch above - do the statements anyway. Unless you're in a really terrible, terrible work environment, people should appreciate that you are just recognising that sometimes the worst case scenario happens.

On the other hand, it's probably excessive to do the following:

mid = (low + high)/2;
assert((low <= mid) && (mid <= high));

In short, if you can identify a point of failure, cover it. Even if it's your own code, sometimes things go wrong and the assumptions you made when writing that code - such as that the caller would actually read the docs - are incorrect.

ChemicalRascal
  • 111
  • 2
0

If you are handling the case where you don't expect something to ever happen, then this is one perfect reason to use runtime exceptions. Also, from http://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html (if you are using Java, that is), runtime exceptions should be used when the client cannot do anything to work around the issue (as probably is the case you described). If the case you've shown occurs, it represents a code bug, which is when runtime exceptions should be thrown.

However, you should probably only do this in few unique or special cases, not in every single method in the system--that would be being paranoid and not trusting your code.

jordan
  • 501
  • 2
  • 6