Argthtjtr

Is this a good way of handling exceptions?

No, I think this is pretty bad practice.  Throwing an exception vs. returning a value is a fundamental change in the API, changing the method's signature, and making the method behave quite differently from an interface perspective.

In general, when we design classes and their APIs, we should consider that

1. there may be multiple instances of the class with different configurations floating around in the same program at the same time, and,




2. due to dependency injection and any number of other programming practices, one consuming client may create the objects and hand them to another other use them — so often we have a separation between object creators and object users.

Consider now what the method caller has to do to make use of an instance s/he's been handed, e.g. for calling the calculating method: the caller would have to both check for area being zero as well as catch exceptions — ouch!  Testing considerations go not just to the class itself, but to callers' error handling as well...

We should always be making things as easy as possible for the consuming client; this boolean configuration in the constructor that changes the API of an instance method is the opposite of making the consuming client programmer (maybe you or your colleague) fall into the pit of success.

To offer both APIs, you're much better and more normal to either provide two different classes — one that always throws on error and one that always returns 0 on error, or, provide two different methods with a single class.  This way the consuming client can easily know exactly how to check for and handle errors.

Using two different classes or two different methods, you can use the IDEs find method users and refactor features, etc.. much more easily since the two use cases are no longer conflated.  Code reading, writing, maintenance, reviews, and testing is simpler as well.

On another note, I personally feel that we should not take boolean configuration parameters, where the actual callers all simply pass a constant.  Such configuration parameterization conflates two separate use cases for no real benefit.

Take a look at your code base and see if a variable (or non-constant expression) is ever used for the boolean configuration parameter in the constructor!  I doubt it.

And further consideration is to ask why computing the area can fail.  Best might be to throw in the constructor, if the calculation cannot be made.  However, if you don't know whether the calculation can be made until the object is further initialized, then perhaps consider using different classes to differentiate those states (not ready to compute area vs. ready to compute area).

I read that your failure situation is oriented toward remoting, so may not apply; just some food for thought.

Shouldn't it be the caller's responsibility to determine how to continue?

Yes, I agree.  It seems premature for the callee to decide that area of 0 is the right answer under error conditions (especially since 0 is a valid area so no way to tell the difference between error and actual 0, though may not apply to your app).

They have worked on critical applications dealing with aviation where the system could not go down. As a result ...

That is an interesting introduction, which gives me the impression the motivation behind this design is to avoid throwing exceptions in some contexts "because the system could go down" then. But if the system "can go down because of an exception", this is a clear indication that

• 
  exceptions are not handled properly , at least nor rigidly.

So if the program which uses the AreaCalculator is buggy, you colleague prefers not to have the program "crash early", but to return some wrong value (hoping no one notices it, or hoping noone does something important with it). That is actually masking an error, and in my experience it will sooner or later lead to follow-up bugs for which it becomes difficult to find the root cause.

IMHO writing a program that does not crash under any circumstances but shows wrong data or calculation results is usually in no way better than letting the program crash. The only right approach is to give the caller a chance to notice the error, deal with it, let him decide if the user has to be informed about the wrong behaviour, and if it is safe to continue any processing, or if it is safer to stop the program completely. Thus, I would recommend one of the following:

• make it hard to overlook the fact a function can throw an exception. Documentation and coding standards are your friend here, and regular code reviews should support the correct usage of components and proper exception handling.




• train the team to expect and deal with exceptions when they use "black box" components, and have the global behaviour of the program in mind.



• 
  

  if for some reasons you think you cannot get the calling code (or the devs who write it) to use exception handling properly, then, as a last resort, you could design an API with explicit error output variables and no exceptions at all, like

  
  
  

  CalculateArea(int x, int y, out ErrorCode err)
  
  

  
  
  

  so it gets really hard for the caller to to overlook the function could fail. But this is IMHO extremely ugly in C#; it is an old defensive programming technique from C where there are no exceptions, and it should normally not be necessary to work that nowadays.

  
  

Sometimes throwing an exception is not the best method. Not least due to stack unwinding, but sometimes because catching an exception is problematic, particularly along language or interface seams.

The best way to handle this is to return an enriched data-type. This data type has enough state to describe all of the happy paths, and all of the unhappy paths. The point is, if you interact with this function (member/global/otherwise) you will be forced to handle the outcome.

That being said this enriched data-type should not force action. Imagine in your area example something like var area_calc = new AreaCalculator(); var volume = area_calc.CalculateArea(x, y) * z;. Seems useful volume should contain the area multiplied by depth - that could be a cube, cylinder, etc...

But what if the area_calc service was down? Then area_calc .CalculateArea(x, y) returned a rich datatype containing an error. Is it legal to multiply that by z? Its a good question. You could force users to handle the checking immediately. This does however break up the logic with error handling.

var area_calc = new AreaCalculator();

var area_result = area_calc.CalculateArea(x, y);

if (area_result.bad())

{

    //handle unhappy path

}

var volume = area_result.value() * z;

vs

var area_calc = new AreaCalculator();

var volume = area_calc.CalculateArea(x, y) * z;

if (volume.bad())

{

    //handle unhappy path

}

The essentially logic is spread over two lines and divided by error handling in the first case, while the second case has all the relevant logic on one line followed by error handling.

In that second case volume is a rich data type. Its not just a number. This makes storage larger, and volume will still need to be investigated for an error condition. Additionally volume might feed other calculations before the user chooses to handle the error, allowing it to manifest in several disparate locations. This might be good, or bad depending on the specifics of the situation.

Alternately volume could be just a plain data type - just a number, but then what happens to the error condition? It could be that the value implicitly converts if it is in a happy condition. Should it be in an unhappy condition it might return a default/error value (for area 0 or -1 might seem reasonable). Alternately it could throw an exception on this side of the interface/language boundary.

... foo() {

   var area_calc = new AreaCalculator();

   return area_calc.CalculateArea(x, y) * z;

}

var volume = foo();

if (volume <= 0)

{

    //handle error

}

vs.

... foo() {

   var area_calc = new AreaCalculator();

   return area_calc.CalculateArea(x, y) * z;

}



try { var volume = foo(); }

catch(...)

{

    //handle error

}

By passing out a bad, or possibly bad value, it places a lot of onus on the user to validate the data. This is a source of bugs, because as far as the compiler is concerned the return value is a legitimate integer. If something was not checked you'll discover it when things go wrong. The second case mixes the best of both worlds by allowing exceptions to handle unhappy paths, while happy paths follow normal processing. Unfortunately it does force the user to handle exceptions wisely, which is hard.

Just to be clear an Unhappy path is a case unknown to business logic (the domain of exception), failing to validate is a happy path because you know how to handle that by the business rules (the domain of rules).

The ultimate solution would be one which allows all scenarios (within reason).

• The user should be able to query for a bad condition, and handle it immediately


• The user should be able to operate on the enriched type as if the happy path had been followed and propagate the error details.


• The user should be able to extract the happy path value through casting (implicit/explicit as is reasonable), generating an exception for unhappy paths.


• The user should be able to extract the happy path value, or use a default (supplied or not)

Something like:

Rich::value_type value_or_default(Rich&, Rich::value_type default_value = ...);

bool bad(Rich&);

...unhappy path report... bad_state(Rich&);

Rich& assert_not_bad(Rich&);

class Rich

{

public:

   typedef ... value_type;



   operator value_type() { assert_not_bad(*this); return ...value...; }

   operator X(...) { if (bad(*this)) return ...propagate badness to new value...; /*operate and generate new value*/; }

}



//check

if (bad(x))

{

    var report = bad_state(x);

    //handle error

}



//rethrow

assert_not_bad(x);

var result = (assert_not_bad(x) + 23) / 45;



//propogate

var y = x * 23;



//implicit throw

Rich::value_type val = x;

var val = ((Rich::value_type)x) + 34;

var val2 = static_cast<Rich::value_type>(x) % 3;



//default value

var defaulted = value_or_default(x);

var defaulted_to = value_or_default(x, 55);

Writing unit tests becomes slightly more complex since you have to test for the exception flag each time.

Any function with n parameters is going to be harder to test than one with n-1 parameters. Extend that out to the absurd and the argument becomes that functions shouldn't have parameters at all because that makes them harder to test.

While it's a great idea to write code that's easy to test, it's a terrible idea to put test simplicity above writing code useful to the people who have to call it. If the example in the question has a switch that determines whether or not an exception is thrown, it's possible that the number callers wanting that behavior merited adding it to the function. Where the line between complex and too complex lies is a judgment call; anyone trying to tell you there's a bright line that applies in all situations should be eyed with suspicion.

Also, if something goes wrong, wouldn't you want to know right away?

That depends on your definition of wrong. The example in the question defines wrong as "given a dimension of less than zero and shouldThrowExceptions is true." Being given a dimension if less than zero isn't wrong when shouldThrowExceptions is false because the switch induces different behavior. That is, quite simply, not an exceptional situation.

The real problem here is that the switch was poorly named because it isn't descriptive of what it makes the function do. Had it been given a better name like treatInvalidDimensionsAsZero, would you have asked this question?

Shouldn't it be the caller's responsibility to determine how to continue?

The caller does determine how to continue. In this case, it does so ahead of time by setting or clearing shouldThrowExceptions and the function behaves according to its state.

The example is a pathologically-simple one because it does a single calculation and returns. If you make it slightly more complex, such as calculating the sum of the square roots of a list of numbers, throwing exceptions can give callers problems they can't resolve. If I pass in a list of [5, 6, -1, 8, 12] and the function throws an exception over the -1, I have no way to tell the function to keep going because it will have already aborted and thrown away the sum. If the list is an enormous data set, generating a copy without any negative numbers before calling the function might be impractical, so I'm forced to say in advance how invalid numbers should be treated, either in the form of a "just ignore them" switch or maybe provide a lambda that gets called to make that decision.

His logic/reasoning is that our program needs to do 1 thing, show data to user. Any other exception that doesn't stop us from doing so should be ignored. I agree they shouldn't be ignored, but should bubble up and be handled by the appropriate person, and not have to deal with flags for that.

Again, there is no one-size-fits-all solution. In the example, the function was, presumably, written to a spec that says how to deal with negative dimensions. The last thing you want to be doing is lowering the signal-to-noise ratio of your logs by filling them with messages that say "normally, an exception would be thrown here, but the caller said not to bother."

And as one of those much-older programmers, I would ask that you kindly depart from my lawn. ;-)

Methods either handle exceptions or they don't, there are no need for flags in languages like C#.

public int Method1()

{

  ...code



 return 0;

}

If something goes bad in ...code then that exception will need to be handled by the caller. If no one handles the error, the program will terminate.

public int Method1()

{

try {  

...code

}

catch {}

 ...Handle error 

}

return 0;

}

In this case, if something bad happens in ...code, Method1 is handling the problem and the program should proceed.

Where you handle exceptions is up to you. Certainly you can ignore them by catching and doing nothing. But, I would make sure you are only ignoring certain specific types of exceptions that you can expect to occur. Ignoring (exception ex) is dangerous because some exceptions you do not want to ignore like system exceptions regarding out of memory and such.

To be honest, I'm not even sure that your colleague's notion of using this sort of approach in the aviation industry is a strong example. I certainly wouldn't want to be in a plane with this in the codebase:

public class AltitudeCalculator

{

    AltitudeCalculator(bool shouldThrowExceptions) { ... }

    CalculateAltitude(int x, int y)

    {

        if(x < 0 || y < 0)

        {

            if(shouldThrowExceptions) 

                throwException;

            else

                return 0;

        }

    }

}

Safety critical and 'normal' code can lead to very different ideas of what 'good practice' looks like. There's a lot of overlap - some stuff is risky and should be avoided in both - but there are still significant differences. If you add a requirement to be guaranteed to be responsive these deviations get quite substantial.

These often relate to things you'd expect:

• 
  

  For git, the wrong answer could be very bad relative to: taking-to-long/aborting/hanging or even crashing (which are effectively non-issues relative to, say, altering checked-in code accidentally).

  
  
  

  However:
  For an instrument panel having a g-force calculation stalling and preventing an air-speed calculation being made maybe unacceptable.

  
  

Some are less obvious:

• If you have tested a lot, first order results (like right answers) are not as big a worry relatively speaking. You know your testing will have covered this. However if there was hidden state or control flow, you don't know this won't be the cause of something a lot more subtle. This is hard to rule out with testing.




• Being demonstrably safe is relatively important. Not many customers will sit down the reason about whether the source they are buying is safe or not. If you are in the aviation market on the other-hand...

How does this apply to your example:

I don't know. There are a number of thought processes that might have lead rules like "No-one throw in production code" being adopted in safety critical code that would be pretty silly in more usual situations.

Some are good (tight performance bounds were needed) some are bad (we don't handle exceptions properly so best not risk it). Most of the time even knowing why they did it won't really answer the question. For example if it is to do with the ease of auditing the code more that actually making it better, is it good practice?
You really can't tell. They are different animals, and need treating differently.

All of that said, it looks a tad suspect to me BUT:

Safety critical software and software design decisions probably shouldn't be made by strangers on software-engineering stackexchange. There may well be a good reason to do this even if it's part of a bad system. Don't read to much into any of this other than as "food for thought".

Your colleague demonstrates a fundamental misunderstanding of exceptions. An exception isn't something that crashes the program, it simply says that in the context it is thrown in something out of the ordinary happened and it doesn't know how to proceed.
The CalculateArea method is "surprised" if any of the sides are negative and there are multiple ways to handle it:

1. 
   Guess - return X*Y or abs(X*Y). This means that the situation is not unexpected, you've already planned for it. It can be a valid solution, but it can be a source of problems if the guess is not obvious.


2. 
   Return error code - return -1 or some other error code. This is perfectly valid in languages that don't have error handling out of the box, but in C# this has been superseded exactly by exceptions. There are many ways in which exceptions are better, discussed below.


3. 
   Throw exception - the caller decides how to handle the unexpected case. He knows he can't get an area and he can decide what to do about it - he can assume it's 0, he can do some fallback computations, he can give up and throw a different exception/not catch the exception, moving the problem upwards in the call chain. The trick here is that someone with enough context makes the decision on what to do if the area can't be computed.


4. 
   Param for forgiveness - this is the situation you're in. The boolean parameter decides whether the function will report the exception to the caller or swallow it and return a guess/error code. However the one setting the boolean parameter is the caller, so it would be just as simple for the caller to catch the exception and fallback to 0 himself.

The only valid reason i can see for the 4th option is if the error handling is not trivial and repeated throughout the code base. However, even then, the right solution imho is to have an additional method CalculateAreaSafely that wraps the CalculateArea method adding the desired fallbacks.

The thing to remember is that if the method itself knows how to handle failure, it should do so. If it does not, it should report the failure up the chain and the caller can then decide what to do, keeping in mind that the caller knows more of the context and can make better decisions. This goes all the way to the top, so at some point your avionics system knowing the altitude computation failed is the one that decides whether to show it to the pilot as a flashing red light near the "reset avionics" button or to pretend that everything is ok and the altitude is 0 or whatever the last computed altitude was. By attempting to solve it in the lowest, most context-unaware method the system itself has a huge chance of not doing the right thing.