28
try { 
  if (isFileDownloaded)
   // do stuff
  else
   throw new CustomException()
} 
catch (Exception e)
{
  // something went wrong to save the error to log
}
finally
{
  //release resources
}

My question is would the catch catches the ApplicationException thrown in the try block? is it in poor coding style?

Should it be written in another way?

fatihyildizhan
  • 8,614
  • 7
  • 64
  • 88
Quincy
  • 1,923
  • 5
  • 27
  • 37

4 Answers4

37

The catch will catch your exception (and any other that occurs). That being said, I try to avoid writing code like this when possible.

Personally, I see little reason to ever have exception handling (catch) for an exception thrown in the same scope. If you can handle your error in your method - put the exception handling (ie: logging) directly in the try block as well.

Using a catch is more useful, IMO, for catching exceptions thrown by methods within your try block. This would be more useful, for example, if your // do stuff section happened to call a method that raised an exception.

Also, I recommend not catching every exception (Exception e), but rather the specific types of exceptions you can handle correctly. The one exception to this would be if you're rethrowing the exception within your catch - ie: using it for logging purposes but still letting it bubble up the call stack.

Reed Copsey
  • 554,122
  • 78
  • 1,158
  • 1,373
  • 3
    Anything you would do in the catch you can do in the else that raises the exception. You should only raise exceptions in exceptional circumstances. As an aside you should not get used to calling exception objects `e`. This will clash with the `EventArgs` parameter in event handlers. – Philip Smith Jul 20 '10 at 19:56
  • @Philip: "Anything you would do in the catch you can do in the else that raises the exception." - That was my point :) As for your other arguments, I agree to a point, but I personally have no problem with naming exception objects e (though I usually use a more meaningful name), since they're rarely used with EventArgs directly. – Reed Copsey Jul 20 '10 at 19:58
  • 2
    Let's say I have a method that reads some XML files with instructions. It could throw an exception due to disk I/O or XML parsing, but both are expected to be rare. It could also find that the contents of the XML are invalid in terms of their semantics, which is also rare. Given that we already have a catch block that logs the failure already, how is it bad to throw an exception when we get a semantic violation? – Steven Sudit Jul 20 '10 at 20:02
  • @Steven: I agree that there are cases to do this - however, both of your ideas fall into my last sentence (the "The one exception to this..." - I'd potentially catch those to log them, but rethrow them. – Reed Copsey Jul 20 '10 at 20:07
  • @Steven: I would NOT throw an exception that's only going to get swallowed in this case - I'd rather handle it explicitly. – Reed Copsey Jul 20 '10 at 20:08
  • @Reed: In the case I'm thinking of, it logs without rethrowing. Either it returns a `false` or `null`, or it just returns. There's no benefit to letting the exception propagate any further. – Steven Sudit Jul 20 '10 at 20:14
  • @Reed: I understand *that* you would. I don't understand *why*. – Steven Sudit Jul 20 '10 at 20:14
  • @Steven: There are exceptions to everything, of course, so I won't argue that there may be good reasons for what you're saying. ;) I, personally, feel that any method that needs to throw an exception AND swallow the same exception is probably too long, and should be refactored out (in which case, the exception throwing is probably going to be in a different scope)... – Reed Copsey Jul 20 '10 at 20:21
  • @Reed: There's a joke in there somewhere about exceptions regarding exceptions, but I'm more interested in learning from this disagreement than mining it for laughs. Having said that, I'm not sure how to proceed. I suppose I could try to talk about the benefits of having a single point for handling failures or the idea of converting exceptions that occur in the scope of a method into returns, but that may well be missing your point entirely. – Steven Sudit Jul 20 '10 at 20:32
  • @Steven: (Yeah - downvote doesn't matter anyways - but I was curious because I had 2, though now just 1) I agree that it's tricky - I guess, given your XML parsing exception, I'd probably treat that VERY differently than I'd treat, for example, a missing file entirely, or something that would throw some other exception. My statement about not trapping general purpose exceptions without throwing was more about "catch (Exception e)" [ie: catching EVERY exception]. In your case, you're throwing some specific, known exception type, so swallowing it may be appropriate. In my exp., though, ... – Reed Copsey Jul 20 '10 at 20:40
  • ...This ends up with lots of different exception handling options, and just putting the logging method call in place and returning null or false or whatever has always seemed more maintainable - at least in my code. :) I definitely see how DRY comes into play, though, so I'm not saying I'm right here. – Reed Copsey Jul 20 '10 at 20:41
  • @ReadCopsey - To prevent Exceptions from reaching the UI the final place to catch them would be in event handlers from UI controls, So I disagree that EventArgs and Exception catching would rarely meet. – Philip Smith Jul 20 '10 at 21:00
  • @Philip: I'm not saying they never meet - but there are enough cases where they don't that I wouldn't, personally, treat it as a rule. The compiler handles those cases well enough for me. – Reed Copsey Jul 20 '10 at 21:02
  • @Reed: Thanks for trying to explain. – Steven Sudit Jul 20 '10 at 21:17
  • Sorry for the necromancy here, and I'm probably being naïve, but it seems like a use case for this would be validating data passed to a function. Say some data will result in nonsense output. You can cout or log an error, but the code will blunder ahead with the bad output. If you throw an exception, the code halts and you know exactly where you passed invalid data. – Robert M. Aug 19 '23 at 22:19
13

Yes, it will catch ApplicationException as it derives from Exception.

Handling the base exception should be fine in most cases unless you need to log or do something with a different type of exception...

try {
    if (isFileDownloaded)
       doSomeThings();
    else
       throw new ApplicationException("Something expectedly unexpected happened.");
}
catch(ApplicationException e)
{
   // log application exception here...
}
catch(Exception e)
{
   // log all other exceptions here...
}
finally
{
   // release resources...
}
Alex
  • 34,899
  • 5
  • 77
  • 90
2

Also, FYI, ApplicationException has been deprecated since .NET 2.0 as an exception to derive from. It was never intended as an exception to throw on its own, so you probably shouldn't use it at all.

John Saunders
  • 160,644
  • 26
  • 247
  • 397
  • FxCop enforces these rules. You cannot throw a base exception or derive from `ApplicationException` or `SystemException`. These are the base exceptions along with `Exception` itself – Philip Smith Jul 20 '10 at 19:53
1

Yes the catch would catch your ApplicationException and yes is is poor coding style. As a good general rule only catch specific exception and those that you are going to do something with, like fixing up application state.

btlog
  • 4,760
  • 2
  • 29
  • 38