Is this a redundant NullPointerException catch?

Viewed 76

Below Spring REST code returns List for a given ticketId.

Could a NullPointerException be thrown in this code ?

NullPointerException explicitly caught in TicketController:

    catch (NullPointerException nullPointerException) {
        throw new ResponseStatusException(
            HttpStatus.BAD_REQUEST, nullPointerException.getMessage(), nullPointerException);
    }

The thinking may have been when checking for null on the ticket id:

        if (ticketId == null) {
            throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "ticket id cannot be null");
        }

the expectation is that a NullPointerException would be thrown but instead a ResponseStatusException is thrown ?

If the variable ticketId is a path parameter it can never be null as hit the base url / without a ticketId I receive:

There was an unexpected error (type=Method Not Allowed, status=405).

Entire source:

@RestController
    public class TicketController {

        private final TicketServiceImpl ticketServiceImpl;

        public TicketController(TicketServiceImpl ticketServiceImpl) {
            this.ticketServiceImpl = ticketServiceImpl;
        }

        @GetMapping(path = "/{ticketId}")
        public ResponseEntity<List<TicketResponse>> getTicketsById(
            @PathVariable("ticketId") final Long ticketId) {
            try {
                final List<TicketResponse> ticketsById = ticketServiceImpl.getAll(ticketId);
                return new ResponseEntity<>(ticketsById, HttpStatus.OK);
            }
            catch (NullPointerException nullPointerException) {
                throw new ResponseStatusException(
                    HttpStatus.BAD_REQUEST, nullPointerException.getMessage(), nullPointerException);
            }
            catch (TicketNotFoundException ticketNotFoundException) {
                throw new ResponseStatusException(
                    HttpStatus.NOT_FOUND, "Ticket id not found",
                    ticketNotFoundException);
            }

        }

    }

@Service
public class TicketServiceImpl implements TicketService {

    private final TicketRepository ticketRepository;

    public TicketServiceImpl(TicketRepository ticketRepository) {

        this.ticketRepository = ticketRepository;

    }

    @Override
    public List<TicketResponse> getAll(Long ticketId) {

        final List<TicketResponse> ticketResponselist = ticketRepository.findData(ticketId);

            if (ticketId == null) {
                throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "ticket id cannot be null");
            }
            else if (ticketResponselist.size() == 0) {
                throw new TicketNotFoundException("ticket not found");
            }
            else {
                return ticketResponselist;
            }

    }
}

@Repository
public interface TicketRepository {

    public List<TicketResponse> findData(Long ticketId);

}
2 Answers

The if (ticketId == null) check should happen before the ticketRepository.findData(ticketId); is called.

Otherwise, the validation doesn't make sense.

Also, as a side note, catching NullPointerException is a bad practice. The reason is that having a null pointer exception thrown is mostly a sign of a coding smell. The code should be null-safe, by either using e.g. Optional or having a proper validation at the method level. In this case, it would be at the route level (which is the external input). From that point onward, if the validation is set, you're dealing with non nullable id.

This is also somehow related to returning null from a method, which is also a bad practice, since it requires a check in every method which then uses the returned value. This would pollute the code, will introduce a new level of abstraction, and will generally lead to nasty bugs.

NullPointerException check is not required in this case.

Side note : Its better to use the super type when passing as a method parameter until and unless you are pretty sure that no exception will be thrown instead of yours.

Related