Map error messages to conditions

Question

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 conditions with errors, so that I can call some validate() which can return the response with a message.

Answer 1

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.

Answer 2

Years ago I might have suggested you make a ResponseException and throw it instead of returning string-based errors with your Response class. I think this is the only truly distinct alternative to your list of if statements. But handling exceptions adds complexity, often hurts clarity, and can make isolating code difficult for testing and other purposes. Exceptions have proven difficult for the caller/client of your code to handle effectively. Bill Venners once said, "Throw exceptions at developers, not at code" meaning that if the calling code could recover from an error, don't throw an exception.

A much better solution is to refactor this using an Or which is a more general-purpose version of your Response class. It is also the functional alternative to throwing exceptions:

Or<Item,String> getValidItem(long itemId) {

    Item item = itemService.findById(itemId);

    return (item == null)                 ? Or.bad("message 1") :
           (State.NEW != item.getState()) ? Or.bad("message 2") :
           !item.hasNumber()              ? Or.bad("message 3") :
           !item.isActive()               ? Or.bad("message 4") :
           !item.isReady()                ? Or.bad("message 5") :
           Or.good(item);
}

public Or<Item,String> sendEmail(long itemId) {

    Or<Item,String> response = getValidItem(itemId);

    if (response.isGood()) {
        send(response.good());
    }
    return response;
}

Now, getValidItem() is purely functional. You could use Haskel-like monads to make sendEmail() appear referentially transparent, but it is sending an email after all. State-separation adds its own complexity, so I would leave it. At least we've broken off a chunk of pure function which should make it easier to test and understand (because we have completely isolated that code from the rest of the system and from the world).

There is probably no good and substantially different way around the big list of if statements in Java. I re-wrote that part using the ternary operator as a more evaluative/functional/brief alternative, but it is not really so different.

I like using the ternary operator for very simple if/else blocks like these, but using if works in some situations where ? : becomes hopelessly confusing. if makes it easier to debug any exceptions that are thrown because the line number in the stack trace is more helpful when you break statements into multiple lines. The ternary operator is effectively a single line.

Answer 3

Returning a Response object that encodes either no error or one error is an anti-pattern. If the action succeeds, return nothing. If the action fails, throw an exception.

Answer 4

The following is what I consider an extension of @OldCurmudgeon's ansswer with three significant deviations/diclaimers:

1. It's uses Java 8's Stream and Predicate.
2. It turns your Response class into an enum.
3. Each Response enum value interacts solely with an Item object, i.e. not to be used as a 'generic' Response object.

If you are not fine with any of the above, then Response.fail()... just take the following as cursory reading... :)

---

So, the enum approach isn't too bad an idea, since your original implementation did impose some kind of ordering for the validation steps. Seeing how each validation is chained to an Item object, and seeing how a Response object constructed with a description is returned from within each if statement when the validation fails, got me thinking...

• What if a Response can validate itself against an Item in an orderly fashion?
• In Java 8 lingo, what if Response implements the Predicate interface so that it can test() an Item?
• Also, what if the description is simply the textual representation of my Response values?
• What would all these ideas look like?

public enum Response implements Predicate<Item> {
    NOT_FOUND(i -> i != null, "Can't find item"),
    NOT_NEW(i -> i.getState() == State.NEW, "Can't send first inquiry to customer for items not in 'NEW' state"),
    NO_TICKET_NUMBER(Item::hasNumber, "Can't find ticket in OTRS"),
    NOT_ACTIVE(Item::isActive, "Item is not active anymore"),
    NOT_READY(Item::isReady, "Item is not finalized, first inquiry can be send only for processed items"),
    SUCCESS(i -> true, "Validation OK");

    private final Predicate<? super Item> successCase;
    private final String failReason;

    private Response(Predicate<? super Item> successCase, String failReason) {
        this.successCase = successCase;
        this.failReason = failReason;
    }

    @Override
    public boolean test(Item t) {
        return successCase.negate().test(t);
    }

    @Override
    public String toString() {
        return failReason;
    }
}

Returning failReason from toString() is self-explanatory, so I'll explain why we take in the successful condition aka Predicate, but negate() it when we do our test()-ing...

1. Readability, in an unconventional sense. The constructor arguments can be read as "test for this condition, if it fails use this reason". That's why the arguments are named successCase and failReason (ok, only the SUCCESS case doesn't make much sense for failReason).
2. So that I can use method references for half the cases.
3. So that I can clearly distinguish the SUCCESS case through the negation, by making sure we will not preemptively return it first as described below.

The entire sendFirstInquiryEmail() then becomes just:

private static Response sendFirstInquiryEmail(long itemId) {
    Item item = itemService.findById(itemId);
    Response result = EnumSet.allOf(Response.class).stream()
            .filter(r -> r.test(item)).findAny().orElse(Response.SUCCESS);
    if (result == Response.SUCCESS) {
        send(item);
    }
    return result;
}

In steps:

• Stream on our Response values.
• Filter for those where doing our test() returns true since we are checking for any validation failures ('failure') first.
• findAny()... find any Response value corresponding to the failure. This can be findFirst() too.
• If there are no failures, findAny() will yield an Optional.empty(), so this is when we do an orElse() to return our Response.SUCCESS value.

Can we test this? Of course! Just one more thing... we started with three significant deviations/disclaimers, and I need a fourth for the test to work properly:

4. Item is an interface

If it's not, then consider the following as more cursory reading too if you have not yet done so. :D

public class ResponseTest {

    enum TestItem implements Item {
        NOT_FOUND(null),
        NOT_NEW(State.OLD), // assuming State.OLD exists
        NO_TICKET_NUMBER(false, false, false),
        NOT_ACTIVE(true, false, false),
        NOT_READY(true, true, false),
        SUCCESS(true, true, true);

        private final State state;
        private final boolean hasNumber;
        private final boolean isActive;
        private final boolean isReady;

        private TestItem(State state) {
            this(state, false, false, false);
        }

        private TestItem(boolean hasNumber, boolean isActive, boolean isReady) {
            this(State.NEW, hasNumber, isActive, isReady);
        }

        private TestItem(State state, boolean hasNumber, boolean isActive,
                boolean isReady) {
            this.state = state;
            this.hasNumber = hasNumber;
            this.isActive = isActive;
            this.isReady = isReady;
        }

        @Override
        public State getState() {
            return state;
        }

        @Override
        public boolean hasNumber() {
            return hasNumber;
        }

        @Override
        public boolean isActive() {
            return isActive;
        }

        @Override
        public boolean isReady() {
            return isReady;
        }

        public void verify() {
            assertThat(testSendFirstInquiryEmail(state == null ? null : this),
                    equalTo(Response.valueOf(name())));
            System.out.println("Verified for: " + Response.valueOf(name()));
        }
    }

    @DataProvider(name = "test-cases")
    public Iterator<Object[]> getTestCases() {
        return EnumSet.allOf(TestItem.class).stream().map(v -> new Object[] { v })
                .iterator();
    }

    @Test(dataProvider = "test-cases")
    public void test(TestItem item) {
        item.verify();
    }

    private static Response testSendFirstInquiryEmail(Item item) {
        return EnumSet.allOf(Response.class).stream()
                .filter(r -> r.test(item)).findAny().orElse(Response.SUCCESS);
    }
}

The use of the TestItem enum is to easily generate different Item values that will cause the validation to fail at a rule. More importantly, the names on TestItem and Response matches so that we can do a nifty name() trick in our assertion:

        // Hamcrest Matchers library
        assertThat(testSendFirstInquiryEmail(state == null ? null : this),
                equalTo(Response.valueOf(name())));

What that describes is to call our test method testSendFirstInquiryEmail() with the current TestItem item (or null - the checking of state == null is just a convenient way of testing this), and assert that the Response value returned has the same name.

@Test and @DataProvider annotations come from TestNG to handle parameterized testing, and what they do are to iteratively test each TestItem by calling its verify() method.

Test output snippet:

Verified for: Can't find item
Verified for: Can't send first inquiry to customer for items not in 'NEW' state
Verified for: Can't find ticket in OTRS
Verified for: Item is not active anymore
Verified for: Item is not finalized, first inquiry can be send only for processed items
Verified for: Validation OK
PASSED: test(NOT_FOUND)
PASSED: test(NOT_NEW)
PASSED: test(NO_TICKET_NUMBER)
PASSED: test(NOT_ACTIVE)
PASSED: test(NOT_READY)
PASSED: test(SUCCESS)

Answer 5

I would recode the list of ifs into a list of tests and implement them as an enum. There are other techniques but enums offer a nice succinct way of doing this.

public static class Response {

    private String comment;
    private final boolean successful;
    // Special one meaning success and you can use == on it.
    public static final Response SUCCESS = new Response(true).withComment("All good.");

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

    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;
    }

}

enum Test {

    NotNull {

                @Override
                Response test(Item item) {
                    if (item == null) {
                        return Response.fail().withComment("Can't find item");
                    }
                    return Response.SUCCESS;
                }

            },
    NewState {

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

            },
    HasNumber {

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

            },
    IsActive {

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

            },
    IsReady {

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

            };

    abstract Response test(Item item);
}

public Response sendFirstInquiryEmail(long itemId) {

    Item item = itemService.findById(itemId);

    for (Test t : Test.values()) {
        Response r = t.test(item);
        if ( r != Response.SUCCESS) {
            return r;
        }
    }
    // All tests succeeded! Send it.
    send(item);

    return Response.SUCCESS;

}

Note also I have made a single SUCCESS object like @rolfl suggested.