Skip to content

Optional with throws

Petr Arsentev edited this page Oct 21, 2023 · 5 revisions

Let's take this code.

    @GetMapping("/{id}")
    public ResponseEntity<Interview> getById(@Valid @PathVariable int id) {
        return new ResponseEntity<>(
                interviewService
                        .findById(id)
                        .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND)),
                HttpStatus.OK
        );
    }

I sure you have the same code in your projects too. You need to find somethings in DB, but it doesn't exist any more.

The method findById returns Optional value. In this case, you may to manage it few ways:

  1. Check by isEmpty with if-else statements.
    @GetMapping("/{id}")
    public ResponseEntity<Interview> getById(@Valid @PathVariable int id) {
        var interview = interviewService.findById(id);
        if (interview.isEmpty()) {
            return ResponseEntity.notFound().build();
        }
        return ResponseEntity.ok(interview.get());
    }
  1. Use Optional.map in functional style.
    @GetMapping("/{id}")
    public ResponseEntity<Interview> getById(@Valid @PathVariable int id) {
        return interviewService.findById(id)
                .map(ResponseEntity::ok)
                .orElseGet(() -> ResponseEntity.notFound().build());
    }

In my option, the second one it is cleaner then the first one. But in case when you need to add extra code to handle exception or to do somethings with finded value the first snippet is preferable.

Let's consider situation when you just to log it.

  1. With if.
    @GetMapping("/{id}")
    public ResponseEntity<Interview> getById(@Valid @PathVariable int id) {
        var interview = interviewService.findById(id);
        if (interview.isEmpty()) {
            log.warn(String.format("Not found interview id=%s", id));
            return ResponseEntity.notFound().build();
        }
        log.debug(String.format("Interview %s", interview));
        return ResponseEntity.ok(interview.get());
    }
  1. With Optional.map
    @GetMapping("/{id}")
    public ResponseEntity<Interview> getById(@Valid @PathVariable int id) {
        return interviewService.findById(id)
                .map(interview -> {
                    log.debug(String.format("Interview %s", interview));
                    return ResponseEntity.ok(interview);
                })
                .orElseGet(() -> {
                    log.warn(String.format("Not found interview id=%s", id));
                    return ResponseEntity.notFound().build();
                });
    }

In this case you increase the nesting, so you decrease the readability.

I would like to point anti-patterns.

  1. Don't use ternary statements.
    @PutMapping("/")
    public ResponseEntity<Interview> update(@Valid @RequestBody Interview interview) {
        return new ResponseEntity<Interview>(interview,
                interviewService.update(interview) ? HttpStatus.OK : HttpStatus.NO_CONTENT);
    }
    //good
    @PutMapping("/")
    public ResponseEntity<Interview> update(@Valid @RequestBody Interview interview) {
        return interviewService.update(interview)
                .map(ResponseEntity::ok)
                .orElseGet(ResponseEntity.notFound()::build);
    }
  1. Don't wrap Optional.
    //wrong
    @PostMapping("/")
    public ResponseEntity<Interview> save(@Valid @RequestBody Interview interview) throws SQLException {
        return new ResponseEntity<>(
                interviewService
                        .save(interview)
                        .orElseThrow(() -> new SQLException("An error occurred while saving data")),
                HttpStatus.CREATED
        );
    }
    //good
    @PostMapping("/")
    public ResponseEntity<Interview> save(@Valid @RequestBody Interview interview) throws SQLException {
        return interviewService.save(interview)
                .map(ResponseEntity::ok)
                .orElseThrow(() -> new SQLException("An error occurred while saving data"))
    }

Clone this wiki locally