Throwing exceptions if there is not exactly one box retrieved

Question

(Note: this is originally posted as this SO question )

I'm trying to refactor the error logging and exception throwing into a method.

Original code:

private Box retrieveBox() {
    BoxParameter param = new BoxParameter("red", 10, 5, 3);
    List<Long> boxIds = boxDao.retrieveByParameter(param);

    if (boxIds.isEmpty()) {
        // A
        logger.warn("box not found");
        throw new BoxNotFoundException();
    }
    if (boxIds.size() > 1) {
        // B
        logger.warn("multiple boxes found");
        throw new MultipleBoxesFoundException();
    }

    Optional<Box> maybe = boxDao.retrieve(boxIds.get(0));
    if (maybe.isPresent()) {
        return maybe.get();
    }

    // C
    logger.warn("box not found");
    throw new BoxNotFoundException();
} 

Refactored code:

private Box retrieveBox() {
    BoxParameter param = new BoxParameter("red", 10, 5, 3);
    List<Long> boxIds = boxDao.retrieveByParameter(param);

    if (boxIds.isEmpty()) {
        // A
        logErrorAndThrowBoxNotFoundException();
    }
    if (boxIds.size() > 1) {
        // B
        logErrorAndThrowMultipleBoxesFoundException();
    }

    Long boxId = boxIds.get(0);
    Optional<Box> maybe = boxDao.retrieve(boxIds.get(0));
    if (maybe.isPresent()) {
        return maybe.get();
    }

    // C-1
//  logger.warn("box not found");
//  throw new BoxNotFoundException();

    // C-2
    logErrorAndThrowBoxNotFoundException();
    return null;  // needs to be here to make it compile
}

private void logWarningAndThrowMultipleBoxesFoundException() throws MultipleBoxesFoundException {
    logger.warn("multiple boxes found");
    throw new MultipleBoxesFoundException();
}

private void logWarningAndThrowBoxNotFoundException() throws BoxNotFoundException {
    logger.warn("box not found");
    throw new BoxNotFoundException();
} 

I really dislike the return null part in C-2, but without it, I'll have to go back to C-1, which defeats the purpose of refactoring.

Is there a better way to do it?

Answer 1

Java 8

Optional<Box> maybe = boxDao.retrieve(boxIds.get(0));
if (maybe.isPresent()) {
    return maybe.get();
}

// C
logger.warn("box not found");
throw new BoxNotFoundException();

If this is the Java 8 Optional we are talking about, you can use Optional.orElseThrow(Supplier) instead:

return boxDao.retrieve(boxIds.get(0)).orElseThrow(() -> {
    logger.warn("Box not found.");
    return new BoxNotFoundException();
});

Guava

If this is Guava we are talking about, then between the options you presented, I think it's still fine to stick with the original. Method extraction in only two places - just to 'substitute' two lines with the added complication of throw-ing different Exception types altogether - is unnecessary here. And as you have observed, having to return null does look out of place too.

Alternative

I understand you are trying to unify the exception throwing when you have either no results, or no box from the given ID. In that case, perhaps you can reorder your checks accordingly:

private Box retrieveBox() {
    BoxParameter param = new BoxParameter("red", 10, 5, 3);
    List<Long> boxIds = boxDao.retrieveByParameter(param);
    if (boxIds.size() == 1) {
        Optional<Box> maybe = boxDao.retrieve(boxIds.get(0));
        if (maybe.isPresent()) {
            return maybe.get();
        }
    }
    boolean isMultiple = boxIds.size() > 1;
    logger.warn(isMultiple ? "Multiple boxes found." : "Box not found.");
    throw isMultiple ? new MultipleBoxesFoundException() : new BoxNotFoundException();
} 

Answer 2

I'd suggest you just get rid of the throwing method and throw it in the retrieveBox method like this:

throw new BoxNotFoundException("box not found");

You should catch it outside this method and handle it, do the logging etc when you catch it. Logging is part of exception handling and should be done when the exception is being handled, not before.

Answer 3

I'll have to go back to C-1, which defeats the purpose of refactoring.

But, what was the purpose of that refactoring anyway? Extracting a method that has And in its name is just exposing the problem, not solving it: the method is responsible for too many things!

Let the caller catch these exceptions and handle them as they see fit!

private Box retrieveBox() {
    BoxParameter param = new BoxParameter("red", 10, 5, 3);
    List<Long> boxIds = boxDao.retrieveByParameter(param);

    if (boxIds.isEmpty()) {
        throw new BoxNotFoundException();
    }
    if (boxIds.size() > 1) {
        throw new MultipleBoxesFoundException();
    }

    Optional<Box> maybe = boxDao.retrieve(boxIds.get(0));
    if (maybe.isPresent()) {
        return maybe.get();
    }

    throw new BoxNotFoundException();
} 

The method is now responsible for one thing: retrieving a box. And it's throwing meaningful exceptions whenever something goes wrong!

It's the client code's job to know what to do with these exceptions. Side note, I don't know what logging framework you're using, but you should be able to simply do something like this:

try {
    box = retrieveBox();
} catch(Exception e) {
    logger.exception(e);
}

No need for a redundant "box not found!" message, when the exception type itself already conveys all the necessary information.

Answer 4

Your original code is the best to start with. There is only one little issue. I would not expect to have the logging before the throw clause. And that is because you may introduce unneccessary redundant logging in the code that will catch the exception. So the right point to log is when the exception is caught.