Is it good practice to throw custom exception from catch statement?

Question

I have command line tool which runs tests. There is test runner class, which does some preparation before test execution and then runs the test and makes a report. Is it OK if my classes catch an exception and throw new custom exception to the upper layer, and then the upper layer will throw it to the upper layer too (until the View class, which will display/log the exception)?

class Program
{
    static void Main(string[] args)
    {
        testRunner = new TestRunner();

        try
        {
            testRunner.RunTest();
            testRunner.GetReport();
        }
        catch (TestRunnerException ex)
        {
            Print(ex);  // prints nicely two levels of exceptions for user
            Log(ex);    // writes to file all levels of exceptions
        }
    }
}

class TestRunner
{
    void RunTest()
    {
        // run test here
    }

    TestReport GetTestReport()
    {
        try
        {
            return testReporter.GenerateReport();
        }
        catch (Exception ex)
        {
           throw new TestRunnerException("Failed to generate report.", ex);
        }
    }
}


class TestReporter
{
    TestReport GenerateReport()
    {
        try
        {
            // create report here
        }
        catch (Exception ex)
        {
            throw new ReportException($"Test '{testName}' missing data.", ex)
        }
    }
}

This is too broad. Sometimes custom makes sense, sometimes it does not. Sometimes preserving the call stack and inner exceptions makes more sense - just re-throw. It will depend on the circumstance. There is not really a right and wrong here. — J..., Sep 04 '18 at 12:14
Unless your application specific exception returning some extra data than `Exception` class why would you need one? — Rahul, Sep 04 '18 at 12:14
I suggest changing `catch (Exception ex)` into *specific* `catch (SomeKnownCaseException ex) {throw new ReportException($"Test '{testName}' missing data.", ex);}` — Dmitry Bychenko, Sep 04 '18 at 12:15
I don't see a good use-case for nested exceptions here. I don't see any data added in your `TestRunnerException` that will make it worthwhile. You can simply remove the `try...catch` in the `TestRunner` and handle the original exceptions in the main class. — Zohar Peled, Sep 04 '18 at 12:16
Don't reinvent the wheel by creating your own test framework. Use nunit or xunit or even ms-test. — Neil, Sep 04 '18 at 12:18
See MSDN: `Do create and throw custom exceptions if you have an error condition that can be programmatically handled in a different way than any other existing exceptions. Otherwise, throw one of the existing exceptions.` (https://learn.microsoft.com/en-us/previous-versions/dotnet/netframework-4.0/ms229021%28v%3dvs.100%29) — Stefan, Sep 04 '18 at 12:20
Exceptions bubble up the stack. That design allows you to throw the exception as soon as possible (that is, in the place where your program actually encounter an exceptional situation that can't (or shouldn't) be resolved by it's normal code flow, and can't (or shouldn't) be ignored by the calling method (or the one calling that method and so on) - and catch it as late as possible (or at least, only when there's actually something you can do with it like write to a log or display a message to the user. — Zohar Peled, Sep 04 '18 at 12:20
@Stefan: the first rule was: _"Consider throwing existing exceptions residing in the System namespaces instead of creating custom exception types."_ — Tim Schmelter, Sep 04 '18 at 12:25
@J...: I would argue the closure: reasons not to do this are documented and commonly accepted; therefore not open for opinion. — Stefan, Sep 04 '18 at 12:26
@Stefan A point that is open for debate, which is exactly why this should remain closed. — J..., Sep 04 '18 at 12:30
@ZoharPeled,@Rahul. I do add specific data in most of my custom exceptions. So, my conclusion is - get rid of custom exceptions that does not have additional data, the other have the right to life. — Elephant, Sep 04 '18 at 12:54
That is a good rule of thumb, but as all cases, it depends... I find that most of the time, creating and throwing a custom exception is not needed. — Zohar Peled, Sep 04 '18 at 13:38

Dmitry Bychenko · Answer 1 · 2018-09-04T13:02:04.043

It's not throwing custom exception from catch but catching all exceptions that is a bad practice; imagine:

  TestReport GetTestReport() {
    // throws NullReferenceException (yes, GenerateReport() has a bug)
    return testReporter.GenerateReport(); 
  }
  catch (Exception ex) {
    // Bad Practice: here we hide hideous NullReferenceException, 
    // but throw innocent TestRunnerException
    throw new TestRunnerException("Failed to generate report.", ex);
  }

  ...

  try { 
    GetTestReport();
  }
  catch (TestRunnerException ex) {
    // Nothing serious: Tests report fails, let's do nothing
    // Under the hood: the routine went very wrong  - 
    // NullReferenceException - in GenerateReport(); 
    ;
  }

I suggest using specific exception(s) in the catch:

  TestReport GetTestReport() {
    // throws NullReferenceException 
    return testReporter.GenerateReport(); 
  }
  catch (NotAuthorizedException ex) {
    // We are not allowed to run tests only, nothing serious
    throw new TestRunnerException("Failed to generate report. Not authorized", ex);
  }

  ...

  try { 
    GetTestReport();
  }
  catch (TestRunnerException ex) {
    // Nothing serious: Tests report fails, let's do nothing
    ;
  }

I am not going to hide the initial exception. I return it with the custom one and log all levels of exceptions. — Elephant, Sep 04 '18 at 13:17
@Elephant: I see; however when we catch exceptions we check but the *top one* - `catch (TestRunnerException ex) {...}` and so the *initial* one `NullReferenceException` has been efficiently *hidden*. — Dmitry Bychenko, Sep 04 '18 at 13:22

Is it good practice to throw custom exception from catch statement?

1 Answers1