javaexception

Best way to write a custom Exception


I'm looking for a better way to write my custom Exception:

public class NoSuchTaskException extends BaseImpactException {
    private final String taskId;
    private final String taskType;
    private final Long userId;
    private final DocumentType documentType;
    private final Long documentId;

    public NoSuchTaskException(String taskId, String taskType, Long userId,
        DocumentType documentType, Long documentId) {
        this(taskType, userId, documentType, documentId, taskId, null);
    }

    public NoSuchTaskException(String taskType, Long userId, DocumentType documentType,
        Long documentId) {
        this(taskType, userId, documentType, documentId, null, null);
    }

    public NoSuchTaskException(String taskType, Long userId, DocumentType documentType,
                               Long documentId, String taskId, Throwable throwable) {
        super("exception.task.noSuchTask", throwable);
        this.taskId = taskId;
        this.taskType = taskType;
        this.userId = userId;
        this.documentType = documentType;
        this.documentId = documentId;
    }
// ...
}

which I then use in various places:

I think from the usage examples, it is clear why I think this might not be the best implementation/usage. My idea was to improve this by utilizing a builder pattern:

public class NoSuchTaskException extends BaseImpactException {
    private final String taskId;
    private final String taskType;
    private final Long userId;
    private final DocumentType documentType;
    private final Long documentId;

    private NoSuchTaskException(Builder builder) {
        super("exception.task.noSuchTask", builder.throwable);
        this.taskId = builder.taskId;
        this.taskType = builder.taskType;
        this.userId = builder.userId;
        this.documentType = builder.documentType;
        this.documentId = builder.documentId;
    }

    public static Builder builder(String taskId) {
        return new Builder(taskId);
    }
    
    @RequiredArgsConstructor
    public static class Builder {
        private String taskId;
        private String taskType;
        private Long userId;
        private DocumentType documentType;
        private Long documentId;
        private Throwable throwable;

        public Builder taskId(String taskId) {
            this.taskId = taskId;
            return this;
        }

        public Builder taskType(String taskType) {
            this.taskType = taskType;
            return this;
        }

        public Builder userId(Long userId) {
            this.userId = userId;
            return this;
        }

        public Builder documentType(DocumentType documentType) {
            this.documentType = documentType;
            return this;
        }

        public Builder documentId(Long documentId) {
            this.documentId = documentId;
            return this;
        }

        public Builder throwable(Throwable throwable) {
            this.throwable = throwable;
            return this;
        }

        public NoSuchTaskException build() {
            return new NoSuchTaskException(this);
        }
    }
}

which I can then use as:

throw NoSuchTaskException.builder("task123")
    .documentType(DocumentType.DOCUMENT_B)
    .documentId(500L)
    .build();

instead of using it like this:

throw new NoSuchTaskException("task123", null, null, DocumentType.DOCUMENT_B, 500L);

Is this an acceptable approach? Should I use the builder pattern in my Exception, or is it an overkill (or even a code smell)?


Solution

  • You are already using lombok, so... just use lombok's builder then. Muuch simpler:

    import lombok.Builder;
    import lombok.NonNull;
    import lombok.Value;
    
    @Value
    public class NoSuchTaskException extends BaseImpactException {
        String taskId;
        String taskType;
        Long userId;
        DocumentType documentType;
        Long documentId;
    
        @Builder
        private NoSuchTaskException(@NonNull String taskId, String taskType, Long userId, DocumentType documentType, Long documentId, Throwable cause) {
            super("exception.task.noSuchTask", cause);
            this.taskId = builder.taskId;
            this.taskType = builder.taskType;
            this.userId = builder.userId;
            this.documentType = builder.documentType;
            this.documentId = builder.documentId;
        }
    }
    

    One small 'trick' being used here is that Builder has been put on the constructor; you can't put it on the type itself because you're extending something and lombok cannot divine that you'd want e.g. Throwable cause but not String message from your exception supertypes.

    NB: @Value makes all fields private and final (and even makes your class final), and, of course, gives you getters for all of it and sensible toString, equals, and hashCode implementations.