2
\$\begingroup\$

I have a situation when there could be and could not be values in database, which is why i return optional, so the response object that goes back to the UI could be different depending on the existing or not values.

Examples :

 new UserLogsResponse(auditEntity.get().getTimestamp(), latestRetrievedDocumentDateAndTime.get(),emailFromAuditMessage, tokeDateAndTimeStamp)

As you can see it where it says object.get() its me getting value from the optional.

I would like to avoid numerous if blocks saying if value is Present then add object to the response constructor so currently I am not checking the optional with is present and passing values to the constructor of a response object and inside the constructor i do certain checks and based on checks i either return null or do some operations with not null value and its fine it does the job, however code does not looks good since it highlights get() in optional objects when i do not proceed with checks before passing it to my response object.

@Getter
@Setter
@JsonInclude(JsonInclude.Include.NON_NULL)
public class UserLogsResponse {

    private final String latestSentEmailStamp;
    private final String latestDocumentRetrievalStamp;
    private final String privateEmail;
    private final String loginTimeStamp;

    public UserLogsResponse(LocalDateTime latestSentEmailStamp, LocalDateTime latestDocumentRetrievalStamp, String privateEmail, LocalDateTime loginTimeStamp) {
        this.latestSentEmailStamp = latestSentEmailStamp == null ? null : LocalDateFormatter.ddMMyyAtHHmm(latestSentEmailStamp);
        this.latestDocumentRetrievalStamp = latestDocumentRetrievalStamp == null ? null : LocalDateFormatter.ddMMyyAtHHmm(latestDocumentRetrievalStamp);
        this.privateEmail = privateEmail == null ? null : new Email(privateEmail).hideSomeEmailCharacters();
        this.loginTimeStamp = loginTimeStamp == null ? null : LocalDateFormatter.ddMMyyAtHHmm(loginTimeStamp);
    }
}


  public UserLogsResponse getUserLogs(HttpServletRequest request) {
    UserEntity userEntity = currentUser.get();
    Optional<LocalDateTime> tokeDateAndTimeStamp = tokenService.getPreviousLoginStamp(request,userEntity, LocalDateTime.now());
    Optional<LocalDateTime> latestRetrievedDocumentDateAndTime = auditRepository.getLatestRetrievalDocumentDateAndTime(userEntity.getId());
    Optional<AuditEntity> auditEntity = auditRepository.getLatestSendEmailDateAndTime(userEntity.getId());
    String emailFromAuditMessage = new AuditMessageExtractor(auditEntity.get().getMessage()).extractEmail();
    return tokeDateAndTimeStamp.map(localDateTime -> new UserLogsResponse(auditEntity.get().getTimestamp(), latestRetrievedDocumentDateAndTime.get(), emailFromAuditMessage, localDateTime))
            .orElseGet(() -> new UserLogsResponse(auditEntity.get().getTimestamp(), latestRetrievedDocumentDateAndTime.get(),emailFromAuditMessage, null));
}

Would appreciate any suggestions, as I said everything works and its good but if i was another developer and saw optional objects without ifPresent check it would be confusing.

\$\endgroup\$
0
\$\begingroup\$

Ah sorry guys i found a solution:

Will simply do the following and null checks will be handled in constructor

 LocalDateTime tokeDateAndTimeStamp = tokenService.getPreviousLoginStamp(request,userEntity, LocalDateTime.now()).orElseGet(null);
    LocalDateTime latestRetrievedDocumentDateAndTime = auditRepository.getLatestRetrievalDocumentDateAndTime(userEntity.getId()).orElseGet(null);
    AuditEntity auditEntity = auditRepository.getLatestSendEmailDateAndTime(userEntity.getId()).orElseGet(null);
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

While this appears to make the code simpler, it does have the effect of coupling the UserLogsResponse class to both the token service and the audit repository, which makes it harder to test without mocking or stubbing. Also, do you really want the @Setter annotation? Should those items be read-only after the constructor has run?

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Yes regarding the setter you are right, does not need to be there. \$\endgroup\$ – Artjom Sep 8 at 11:14

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.