Handling Optional get method inside Services-CodePudding

I'm implementing an API but in my service layer I don't really know how to handle .get() of Optionals.

@Service
public class AttributedValueService {
    ...

    public AttributedValueDTO createAttributedValue(ActionDTO actionDTO) {
        Optional<RedeemableValue> redeemableValue = redeemableValueRepository.findRedeemableValueByProductId(actionDTO.getProductId());
        Optional<Value> value = valueRepository.findById(redeemableValue.get().getValue().getId());
        AttributedValue attributedValue = new AttributedValue(value.get(), actionDTO.getId(), actionDTO.getUserId());
        return new AttributedValueDTO(attributedValueRepository.save(attributedValue));
    }
}

My API receives a object and retrieves some other objects from the DB using info from that object and I want to make sure that a ObjectNotFound is returned when one of the .get() doesn't find a object.

Currently I'm thinking about using a orElseThrow on the find calls, something like that:

@Service
public class AttributedValueService {
    ...

    public AttributedValueDTO createAttributedValue(ActionDTO actionDTO) {
        RedeemableValue redeemableValue = redeemableValueRepository.findRedeemableValueByProductId(actionDTO.getProductId())
        .orElseThrow(() -> new ObjectNotFoundException("Id: " redeemableValueDTO.getRedeemableValueId(),"Referred Redeemable Value could not be found"));

        Value value = valueRepository.findById(redeemableValue.get().getValue().getId())
        .orElseThrow(() -> new ObjectNotFoundException("Id: " valueDTO.getValueId(),"Referred Value could not be found"));

        AttributedValue attributedValue = new AttributedValue(value.get(), actionDTO.getId(), actionDTO.getUserId());
        return new AttributedValueDTO(attributedValueRepository.save(attributedValue));
    }
}

Leaving exception handling for Spring, which returns a 404. But, is this the correct approach?

edit: Fixed the Optional return of the createAttributedValue method, thanks for the tips.

CodePudding user response：

First of all, as @Turing85 said, there is no silver bullet.

If you have a RedeemableValueService and ValueService you can let those classes handle the Optional instead of handling it at AttributedValueService

It would be something like:

@Service
public class AttributedValueService {
    ...

    public AttributedValueDTO createAttributedValue(ActionDTO actionDTO) {
        Value value = valueService.findByProductId(actionDTO.getProductId());
        AttributedValue attributedValue = new AttributedValue(value, actionDTO.getId(), actionDTO.getUserId());
        return new AttributedValueDTO(attributedValueRepository.save(attributedValue));
    }
}

@Service
public class ValueService {
    ...

    public Value findByProductId(IdType productId) {
        RedeemableValue redeemableValue = redeemableValueService.findByProductId(productId);
        return valueRepository.findById(redeemableValue.getValue().getId()).orElseThrow(YourException::new);
    }
}

@Service
public class RedeemableValueService {
    ...

    public RedeemableValue findByProductId(IdType productId) {
        return redeemableValueRepository.findRedeemableValueByProductId(productId).orElseThrow(YourException::new);
    }
}

CodePudding user response：

If i get you right, then you only have the use cases:

either you find nothing, then an exception is thrown
or you find something, then you return the object

Thus there is no need for an Optional because it will never be absent ^^ So why add this complexity?

CodePudding user response：

As far as I understand your question, your requirement is to check whether the object you are fetching from database is present or not if not then you want to throw some exception. Here in Optional class we have one method isPresent() it will return true if Optional has some value or it will return false if not.

You can do like below:

@Service
public class AttributedValueService {
    ...

    public Optional<AttributedValueDTO> createAttributedValue(ActionDTO actionDTO) {
        Optional<RedeemableValue> redeemableValue = redeemableValueRepository.findRedeemableValueByProductId(actionDTO.getProductId());
if(redeemableValue.isPresent()){
        Optional<Value> value = valueRepository.findById(redeemableValue.get().getValue().getId());
if(value.isPresent()){
        AttributedValue attributedValue = new AttributedValue(value.get(), actionDTO.getId(), actionDTO.getUserId());
        return Optional.of(new AttributedValueDTO(attributedValueRepository.save(attributedValue)));}
else{
throw ObjectNotFoundException() //  custom exception
}
}
else{
throw ObjectNotFoundException() //  custom exception
}
    }
}

I don't understand your logic behind returning the Optional, because at each step we are checking for optional value then its not required.