how to use Optional isPresent() properly

by

I have this method:

public String updateShift(ShiftsDTO shiftsDTO){

    Optional<Shifts> optionalPreUpdateShift = shiftsRepository.findById(shiftsDTO.getId());

    if (optionalPreUpdateShift.isPresent()) {
        if (!shiftsRepository.existsByShiftStartAndShiftEnd(shiftsDTO.getShiftStart(), shiftsDTO.getShiftEnd())) {
            Shifts preUpdateShift = optionalPreUpdateShift.get();
            shiftsMapper.mapShiftFromDto(shiftsDTO,preUpdateShift);
            shiftsRepository.save(preUpdateShift);
            return " Shift updated successfully";
        } else {
            throw new ShiftAlreadyExistsException("An existing shift already covers this period");
        }
    } else {
        throw  new ShiftNotFoundException("This shift doesn't exist");
    }
}

This method is for updating shifts in a shop. I didn’t use isPresent() before and now tried to implement it but the code looks incredibly dirty and unreadable. What are the correct ways to use Optional in such cases?

  • 1

    shiftsRepository.findById(shiftsDTO.getId()).orElseThrow(() -> new ShiftNotFoundException("This shift doesn't exist")); then you can remove the outer if else block

    – 




  • I tend to agree with the OP in general though, Optional makes code look worse. If you simply document a null result, it should be enough. The extra code doesn’t add enough value over simply requiring a human to read the docs. (I’m ignoring the idea that Shifts plural should perhaps be its own container and never null. Optional doesn’t fix problems with the domain model.)

    – 

One way to keep yourself from indenting like this: Change your first line

Optional<Shifts> optionalPreUpdateShift = ShiftsRepository.findById(shiftsDTO.getId());

to instead unwrap the optional right away:

Shifts shifts = ShiftsRepository.findById(shiftsDTO.getId())
                .orElseThrow(()-> new ShiftNotFoundException("This shift doesn't exist");

I would extract the inner logic of your if...else statement into its own private method. Then you can do something like this, which has the added bonus of improved readability as well.

  public String updateShift(ShiftsDTO shiftsDTO)
  {
    return shiftsRepository.findById(shiftsDTO.getId()).map(preUpdateShift -> doUpdate(preUpdateShift, shiftsDTO))
        .orElseThrow(() -> new ShiftNotFoundException("This shift doesn't exist"));
  }

  private String doUpdate(Shifts preUpdateShift, ShiftsDTO shiftsDTO)
  {
    if (!shiftsRepository.existsByShiftStartAndShiftEnd(shiftsDTO.getShiftStart(), shiftsDTO.getShiftEnd()))
    {
      shiftsMapper.mapShiftFromDto(shiftsDTO, preUpdateShift);
      shiftsRepository.save(preUpdateShift);
      return " Shift updated successfully";
    }
    else
    {
      throw new ShiftAlreadyExistsException("An existing shift already covers this period");
    }
  }

Leave a Comment