Question: I have a Spring JobRunner Component that has a `run()` method which throws my custom exception:

```
public Response run(final Request request, final String id) {
    try {
        execution = jobLauncher.run(job, parameters);
    } catch (JobExecutionAlreadyRunningException e) {
        throw new MyCustomException("already running ", e);
    } catch (JobRestartException e) {         
        throw new MyCustomException("Restart Exception", e);
    } 
    return generateResponse(request, id, execution);
}

```

In my service class, I call this `run()` method inside `process()`

```
  protected Response process(final Request request, final String id) {
      //get entity
      //save
      //bla bla
    Response response;
    try {
        response = jobRunner.run(request, id);
        updateStatus(entity, response.getStatus(), "");
    } catch (MyCustomException ex) {
        updateStatus(entity, FAILED);
        throw new MyCustomException(ex.getMessage(), ex);
    }
}

```

You will notice I am catching `MyCustomException` and rethrowing again. I have to catch because I need to update the status accordingly here in the service class as it has a repository dependency to update status in db for tracking purpose.

I am also rethrowing because attached to this custom exception is a `ControllerAdvice` for api requests so it responds accordingly.

What I would like to know, is this a good design or a bad practice? What could I change if anything?

Answer: 
This can be a good practice, bad practice, or unnecessary, depending on the case. When you catch and throw a new exception with:

```
throw new MyCustomException(ex.getMessage(), ex);

```

you get a new exception with *a new stack trace* that points to this line of code, and you maintain the original exception with its stack trace as the "cause".

* This is a good practice if the original exception was produced in an asynchronous context. This is a common problem with reactive programming: when an exception is created by some asynchronous operation, its stack trace has no reference to the calling code. Creating a new exception at the point of capture adds the context needed to find the operation that resulted in the exception.
* This is a bad practice if the original exception includes *privileged information* that should remain secret. In this case the new exception should be created without a cause, and the original exception should be logged. The code should look like:

```
    log.error(ex);
    throw new MyCustomException("whoopsie");

```

* In other cases, creating a new exception instance is unnecessary. Just re-throw the original exception.

```
    } catch (MyCustomException ex) {
        updateStatus(entity, FAILED);
        throw ex;
    }

```
This is a very good question actually.

I see that you are separating concerns by layers. You should do the same with your exceptions.

And you should also be aware of the type of exceptions you are throwing (checked vs. unchecked).

**JobCustomException**

I'd advise you to create a Checked Exception for your Jobs. Here you would encapsulate all the details related to a job error. And by using a checked exception, you are achieving two things:

1. You are warning the calling class that an exception may be thrown and can be handled.
2. You are signalling the developer that this may be a recoverable error.

From the [Java Docs](https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html):

> 
> If a client can reasonably be expected to recover from an exception,
> make it a checked exception. If a client cannot do anything to recover
> from the exception, make it an unchecked exception
> 
> 
> 

**ServiceCustomException**

Then, another exception can be created for your service layer. This kind of exception should extend from RuntimeException if you believe it may no longer be handled (except for the ControllerAdvice, of course).

It may also be useful to transform or introduce extra information.

Finally, while you may have a single generic Job exception, there may be many client classes (services) that may throw different Runtime-based exceptions to add further business detail about the business flow where the error occurred.

**Throwing, catching, and re-throwing**

You are doing it right, as long as you pay attention to the aforementioned bullets. This is the whole point of throwing exceptions.

Exceptions don't need to be terminal errors, although there is some kind of belief that RuntimeExceptions are the only accepted type of Exceptions.

Most well-designed object's APIs and frameworks, make use of proper Checked exceptions to communicate to the client classes what are the errors that may occur.