Error Handling Bad Practices

This article is a follow-up on my previous 3-step-error-handling article.

In this article I want to show some bad practices in error handling and maybe some quick-fixes, if you stumble
across them in your codebase.

Disclaimer, this blog-post is only a personal opinion and you will need to adjust the recommendations to your specific use-case.

1. Swallowing exceptions

I regularly stumble across code-bases where exceptions are silently ignored. Maybe the developer was in a hurry or wanted to handle the error cases later on? Anyway, this is a bad practice because if the error occurs:

• the user will not get any feedback and might think that the operation was successful.
• there is no stacktrace or error-message to investigate.
• sometimes there is not even a proper log-entry, making it almost impossible to detect the error.

Bad Example:

  /**
   * Reads some data from another service or repo and returns it.
   * The repo might throw an exception, but we don't care.
   */
  public Mono<String> getHello() {
    try {
      return helloRepository.getHello()
          .map("Hello %s"::formatted);
    } catch (Exception e) {
      // do nothing
      return Mono.just("Hello World");
    }
  }

In this example we might wonder why some users complain about not getting the correct hello-message, but we don't see any errors, we might think that the users are just confused.

Option 1 - Keep the try-catch and handle the error

Handle the error where it occurs, e.g. already in the Repository if possible.
If you need to handle it here, at least log the error and return a proper error-message.

Minimal refactoring

• log the error (optionally also the stacktrace)
• return a proper error-message

  public Mono<String> getHello() {
    try {
      return helloRepository.getHello()
          .map("Hello %s"::formatted);
    } catch (Exception e) {
      log.error("Error reading hello data from repository - {}", e.getMessage(), e);
      return Mono.error(new RuntimeException("Internal error reading hello data %s".formatted(e.getMessage()), e));
    }
  }

NOTE:

This might break upstream code since you are now returning an error instead of some default value.

Option 2 - Proper reactive stream refactoring

Better would be to handle the error inside the reactive stream api

  public Mono<String> getHelloHandling() {
    return helloRepository.getHello()
      .map("Hello %s"::formatted)
      // wrap the exception in a RuntimeException with a meaningful message
      .onErrorMap(e -> new RuntimeException("Internal error reading hello data from HelloRepository` %s".
          formatted(e.getMessage()), e));
  }

1b. Swallowing exceptions - with a reactive stream

The pattern from example one also appears in reactive streams:

  public Mono<String> getHello() {
    return helloRepository.getHello()
        .map("Hello %s"::formatted)
        // do nothing on error, just return a default value
        .onErrorReturn("Hello world");
  }

That looks very nice and clean, right? But we will not be able to detect that the error is thrown in the repository!
If there is a default-value, at least an error-log should be written.

Mitigation

As in the previous example we are wrapping the exception in another exception, this time in a custom exception that even makes it easier to detect the specific place where that exception is thrown

  public Mono<String> getHello2() {
    return helloRepository.getHello()
        .map("Hello %s"::formatted)
        .onErrorMap(
            e -> new CustomException("Error reading hello-data from repository - %s".formatted(e.getMessage()), e));
  }

2. Repeatedly logging the stacktrace for the same exception

The very opposite of silently dropping exceptions is logging the same exception multiple times. This is a bad practice because it confuses the logs with hundreds or thousands of lines of stacktraces without providing any additional meaning.

In my worst example I found the same stacktrace five times in the logs with no meaningful message at all.

Bad Example:

The controller:

@RestController
@AllArgsConstructor
@Slf4j
public class HelloController {

  private final HelloService helloService;

  @GetMapping("/hello")
  public Mono<ResponseEntity<String>> hello() {
    return helloService.getHello()
        .map(ResponseEntity::ok)
        .defaultIfEmpty(ResponseEntity.notFound().build())
        .onErrorResume(e -> {
          log.error("Error:", e);
          return Mono.error(e);
        });
  }
}

And in the Service:

@Service
@AllArgsConstructor
@Slf4j
public class HelloService {

  private final HelloRepository helloRepository;

  /**
   * Reads some data from another service or repo and returns it.
   */
  public Mono<String> getHello() {
    return helloRepository.getHello()
        .map("Hello %s"::formatted)
        .onErrorResume(e -> {
          log.error("Error:", e);
          return Mono.error(e);
        });
  }
}

... and probably in some more places...

Mitigation

This is explained in my previous article 3-step-error-handling, therefore I will not show the code here again, but rather a recommendation:

• Have a global error-handler that logs the error and returns a proper error-message to the user.
• In the code, avoid the stacktrace but
  • wrap the exception in a custom exception or runtime exception with a meaningful message
  • log the error-message (not the whole stacktrace)
  • log the stacktrace only in the global error-handler

3. Catch generic exceptions

Catching generic exceptions like Exception or Throwable can lead to unintended behaviour and make debugging quite difficult. It's better to catch specific exceptions.

Bad Example

  public Mono<String> getHello() {
    try {
      return helloRepository.getHello();
    } catch (Exception e) {
      log.error("Error while fetching hello data", e);
      return Mono.empty();
    }
  }

Mitigation

Catch specific exceptions to handle different error scenarios appropriately.

  public Mono<String> getHello() {
    try {
      return helloRepository.getHello();
    } catch (SQLException e) {
      return Mono.error(new HelloDataException("Database error while getting hello-data - %s".formatted(e.getMessage()), e));
    } catch (IOException e) {
      // maybe perform a retry?
      return Mono.error(new HelloDataException("IO error while getting hello-data - %s".formatted(e.getMessage()), e));
    }
  }

and the equivalent using the reactive stream api

  public Mono<String> getHello() {
    return helloRepository.getHello()
        .onErrorMap(SQLException.class, e -> new HelloDataException("Database error while getting hello-data - %s".formatted(e.getMessage()), e))
        .onErrorMap(IOException.class, e -> new HelloDataException("IO error while getting hello-data - %s".formatted(e.getMessage()), e));
  }

4. (Over-)using checked exceptions

In my opinion, checked exceptions where a mistake in Java, they are not very useful and often lead to bad practices and cluttered code.

Bad Example

public void someMethod() throws IOException, SQLException {
  // some code that might throw an exception
}

With checked exceptions you have to deal with them in the calling code, which makes it harder to use other patterns, e.g. functional programming or reactive streams or in spring the global error-handler.

Exception:
Checked exceptions are useful e.g. in library code, where you want to force the user to handle the exception.

Mitigation

Use unchecked exceptions for scenarios where the caller cannot reasonably be expected to recover.

public void someMethod() {
  try {
    // some code that might throw an exception
  } catch (IOException | SQLException e) {
    throw new RuntimeException("An error occurred - %s".formatted(e.getMessage()), e);
  }
}

Using Exceptions for Control Flow

Using exceptions for control flow makes the code hard to understand and can lead to performance issues.

Bad Example

try {
  int value = Integer.parseInt("abc");
} catch (NumberFormatException e) {
  // handle the case where the string is not a number
}

Mitigation

Use a regular flow control mechanism like an if-statement.

String value = "abc";
if (value.matches("\\d+")) {
  int number = Integer.parseInt(value);
} else {
  // handle the case where the string is not a number
}

Conclusion

In this article I showed some bad practices in error handling and how to mitigate them.
I hope you found this article helpful and maybe you can use some of the recommendations in your codebase and in your next refactoring.

The above is the detailed content of Error Handling Bad Practices. For more information, please follow other related articles on the PHP Chinese website!