HiveBrain v1.2.0
Get Started
← Back to all entries
debugjavaMinor

Map error messages to conditions

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
maperrormessagesconditions

Problem

I have a method which checks some preconditions and if everything is fine returns a success response object, otherwise fail with an appropriate error message.

My problem is that I have a bunch of if-elses which are checking some specific condition and if it fails return a fail object with a message.

Here is an example code:

public Response sendFirstInquiryEmail(long itemId) {

        Item item = itemService.findById(itemId);

        if (item == null) {
            return Response.fail().withComment("Can't find item");
        }

        if (State.NEW != item.getState()) {
            return Response.fail().withComment("Can't send first inquiry to customer for items not in 'NEW' state");
        }

        if (!item.hasNumber()) {
            return Response.fail().withComment("Can't find ticket in OTRS");
        }

        if (!item.isActive()) {
            return Response.fail().withComment("Item is not active anymore");
        }

        if (!item.isReady()) {
            return Response.fail().withComment("Item is not finalized, first inquiry can be send only for processed items");
        }

        send(item);

        return Response.success();

}


And here is my response:

public class Response {

    private       String  comment;
    private final boolean successful;

    private Response(boolean successful) {
        this.successful = successful;
    }

    public static Response success() {
        return new Response(true);
    }

    public static Response fail() {
        return new Response(false);
    }

    Response withComment(String comment) {
        this.comment = comment;
        return this;
    }

    public String comment() {
        return comment;
    }

    public boolean isSuccessful() {
        return successful;
    }

}


Those conditions are getting more and more, and do not like these bunch of if. Is there a better way to achieve the same? I was thinking if it is possible somehow to map this conditio

Solution

I am going to critique your Response class. Firstly, I think that code should handle the common cases really well, and, hopefully, success happens often.

I would create a constant SUCCESS instance to indicate the success, and all other instances are fails. As a consequence, I would not have the withComment method. How about:

public final class Response {

    public static final Response SUCCESS = new Response();

    private final String  comment;

    private Response() {
        this.comment = null;
    }

    public Response (String comment) {
        this.comment = Objects.requireNonNull(comment, "Fail responses require a non-null comment");
    }

    public String comment() {
        return comment;
    }

    public boolean isSuccessful() {
        return this == SUCCESS;
    }

}


With the above class, any constructed Response is automatically a fail... only the static SUCCESS instance is a success.

Your use code then becomes:

public Response sendFirstInquiryEmail(long itemId) {

    Item item = itemService.findById(itemId);

    if (item == null) {
        return new Response("Can't find item");
    }

    if (State.NEW != item.getState()) {
        return new Response("Can't send first inquiry to customer for items not in 'NEW' state");
    }

    if (!item.hasNumber()) {
        return new Response("Can't find ticket in OTRS");
    }

    if (!item.isActive()) {
        return new Response("Item is not active anymore");
    }

    if (!item.isReady()) {
        return new Response("Item is not finalized, first inquiry can be send only for processed items");
    }

    send(item);

    return Response.SUCCESS;

}


As for all those conditions that are tested in the cascading if blocks? Well, there's not much you can do about that. They are guard conditions. Your only option is to perhaps incorporate the cascade somewhere else, but all that does is shift the logic.

I would live with it. It is clear, flows logically, and so on.

Code Snippets

public final class Response {

    public static final Response SUCCESS = new Response();

    private final String  comment;

    private Response() {
        this.comment = null;
    }

    public Response (String comment) {
        this.comment = Objects.requireNonNull(comment, "Fail responses require a non-null comment");
    }

    public String comment() {
        return comment;
    }

    public boolean isSuccessful() {
        return this == SUCCESS;
    }

}
public Response sendFirstInquiryEmail(long itemId) {

    Item item = itemService.findById(itemId);

    if (item == null) {
        return new Response("Can't find item");
    }

    if (State.NEW != item.getState()) {
        return new Response("Can't send first inquiry to customer for items not in 'NEW' state");
    }

    if (!item.hasNumber()) {
        return new Response("Can't find ticket in OTRS");
    }

    if (!item.isActive()) {
        return new Response("Item is not active anymore");
    }

    if (!item.isReady()) {
        return new Response("Item is not finalized, first inquiry can be send only for processed items");
    }

    send(item);

    return Response.SUCCESS;

}

Context

StackExchange Code Review Q#91281, answer score: 3

Revisions (0)

No revisions yet.