Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have a situation where I am trying to retrieve an object. If the lookup fails I have several fallbacks in place, each of which may fail. So the code looks like:

try {
    return repository.getElement(x);
} catch (NotFoundException e) {
    try {
        return repository.getSimilarElement(x);
    } catch (NotFoundException e1) {
        try {
            return repository.getParentElement(x);
        } catch (NotFoundException e2) {
            //can't recover
            throw new IllegalArgumentException(e);
        }
    }
}

This looks awfully ugly. I hate to return null, but is that better in this situation?

Element e = return repository.getElement(x);
if (e == null) {
    e = repository.getSimilarElement(x);
}
if (e == null) {
    e = repository.getParentElement(x);
}
if (e == null) {
    throw new IllegalArgumentException();
}
return e;

Are there other alternatives?

Is using nested try-catch blocks an anti-pattern? is related, but the answers there are along the lines of "sometimes, but it's usually avoidable", without saying when or how to avoid it.

share|improve this question
1  
Is the NotFoundException something that is actually exceptional? –  MichaelT Apr 29 at 21:24
    
I don't know, and that's probably why I'm having trouble. This is in an ecommerce context, where products are discontinued daily. If someone bookmarks a product which is subsequently discontinued, and then tries to open the bookmark... is that exceptional? –  FiveNine Apr 29 at 21:29
    
@FiveNine in my opinion, definitely no - it is to be expected. See stackoverflow.com/questions/729379/… –  Konrad Morawski May 2 at 11:00

6 Answers 6

up vote 11 down vote accepted

The usual way to eliminate nesting is to use functions:

Element getElement(x) {
    try {
        return repository.getElement(x);
    } catch (NotFoundException e) {
        return fallbackToSimilar(x);
    }  
}

Element fallbackToSimilar(x) {
    try {
        return repository.getSimilarElement(x);
     } catch (NotFoundException e1) {
        return fallbackToParent(x);
     }
}

Element fallbackToParent(x) {
    try {
        return repository.getParentElement(x);
    } catch (NotFoundException e2) {
        throw new IllegalArgumentException(e);
    }
}

If these fallback rules are universal, you could consider implementing this directly in the repository object, where you might be able to just use plain if statements instead of an exception.

share|improve this answer
    
This is a nice way to keep it legible. Thanks. –  FiveNine Apr 30 at 15:19
    
In this context, method would be a better word than function. –  Sulthan Apr 30 at 20:20

This would be really easy with something like an Option monad. Unfortunately, Java doesn't have those. In Scala, I'd use the Try type to find the first successful solution.

In my functional-programming mindset, I'd set up a list of callbacks representing the various possible sources, and loop through them until we find the first successful one:

interface ElementSource {
    public Element get();
}

...

final repository = ...;

// this could be simplified a lot using Java 8's lambdas
List<ElementSource> sources = Arrays.asList(
    new ElementSource() {
        @Override
        public Element get() { return repository.getElement(); }
    },
    new ElementSource() {
        @Override
        public Element get() { return repository.getSimilarElement(); }
    },
    new ElementSource() {
        @Override
        public Element get() { return repository.getParentElement(); }
    }
);

Throwable exception = new NoSuchElementException("no sources set up");
for (ElementSource source : sources) {
    try {
        return source.get();
    } catch (NotFoundException e) {
        exception = e;
    }
}
// we end up here if we didn't already return
// so throw the last exception
throw exception;

This can be recommended only if you really have a large number of sources, or if you have to configure the sources at runtime. Otherwise, this is an unnecessary abstraction and you'd profit more from keeping your code simple and stupid, and just use those ugly nested try-catchs.

share|improve this answer
    
+1 for mentioning the Try type in Scala, for mentioning monads, and for the solution using a loop. –  Giorgio Apr 30 at 14:37
    
If I was on Java 8 already I would go for this, but like you say, it's a bit much for just a few fallbacks. –  FiveNine Apr 30 at 15:15

If you're anticipating that a lot of those repository calls are going to throw NotFoundException, you could use a wrapper around the repository to streamline the code. I wouldn't recommend this for normal operations, mind you:

public class TolerantRepository implements SomeKindOfRepositoryInterfaceHopefully {

    private Repository repo;

    public TolerantRepository( Repository r ) {
        this.repo = r;
    }

    public SomeType getElement( SomeType x ) {
        try {
            return this.repo.getElement(x);
        }
        catch (NotFoundException e) {
            /* For example */
            return null;
        }
    }

    // and the same for other methods...

}
share|improve this answer

At @amon's suggestion, here's an answer that's more monadic. It's a very boiled down version, where you have to accept a few assumptions:

  • the "unit" or "return" function is the class constructor

  • the "bind" operation happens at compile time, so it's hidden from the invocation

  • the "action" functions are also bound to the class at compile time

  • although the class is generic and wraps any arbitrary class E, I think that's actually overkill in this case. But I left it that way as an example of what you could do.

With those considerations, the monad translates into a fluent wrapper class (although you're giving up a lot of the flexibility that you'd get in a purely functional language):

public class RepositoryLookup<E> {
    private String source;
    private E answer;
    private Exception exception;

    public RepositoryLookup<E>(String source) {
        this.source = source;
    }

    public RepositoryLookup<E> fetchElement() {
        if (answer != null) return this;
        if (! exception instanceOf NotFoundException) return this;

        try {
            answer = lookup(source);
        }
        catch (Exception e) {
            exception = e;
        }

        return this;
    }

    public RepositoryLookup<E> orFetchSimilarElement() {
        if (answer != null) return this; 
        if (! exception instanceOf NotFoundException) return this;

        try {
            answer = lookupVariation(source);
        }
        catch (Exception e) {
            exception = e;
        }

        return this;
    }

    public RepositoryLookup<E> orFetchParentElement() {
        if (answer != null) return this; 
        if (! exception instanceOf NotFoundException) return this;

        try {
            answer = lookupParent(source);
        }
        catch (Exception e) {
            exception = e;
        }

        return this;
    }

    public boolean failed() {
        return exception != null;
    }

    public Exception getException() {
        return exception;
    }

    public E getAnswer() {
        // better to check failed() explicitly ;)
        if (this.exception != null) {
            throw new IllegalArgumentException(exception);
        }
        // TODO: add a null check here?
        return answer;
    }
}

(this won't compile... certain details are left unfinished to keep the sample small)

And the invocation would look like this:

Repository<String> repository = new Repository<String>(x);
repository.fetchElement().orFetchParentElement().orFetchSimilarElement();

if (repository.failed()) {
    throw new IllegalArgumentException(repository.getException());
}

System.err.println("Got " + repository.getAnswer());

Note that you have the flexibility to compose the "fetch" operations as you like. It will stop when it gets an answer or an exception other than not found.

I did this really fast; it's not quite right, but hopefully conveys the idea

share|improve this answer
1  
repository.fetchElement().fetchParentElement().fetchSimilarElement(); - in my opinion: evil code (in the sense given by Jon Skeet) –  Konrad Morawski Apr 30 at 9:40
    
some people don't like that style, but using return this to create chaining object calls has been around for a long time. Since OO involves mutable objects, return this is more or less equivalent to return null without chaining. However, return new Thing<E> opens the door to another capability that this example doesn't go into, so it's important for this pattern if you choose to go down this path. –  Rob Y Apr 30 at 13:35
1  
But I like that style and I'm not against chaining calls or fluent interfaces as such. There is however a difference between CustomerBuilder.withName("Steve").withID(403) and this code, because just from seeing .fetchElement().fetchParentElement().fetchSimilarElement() it's not clear what happens, and that's the key thing here. Do they all get fetched? It's not accumulative in this case, and therefore not that intuitive. I need to see that if (answer != null) return this before I truly get it. Perhaps it's just a matter of proper naming (orFetchParent), but it's "magic" anyway. –  Konrad Morawski Apr 30 at 15:48
1  
By the way (I know your code is oversimplified and just a proof of concept), it would be good to maybe return a clone of answer in getAnswer and reset (clear) the answer field itself before returning its value. Otherwise it kind of breaks the principle of command/query separation, because asking to fetch an element (querying) alters the state of your repository object (answer is never reset) and affect the behavior of fetchElement when you call it next time. Yes I am nitpicking a bit, I think the answer is valid, I wasn't the one who downvoted it. –  Konrad Morawski Apr 30 at 16:07
1  
that's a good point. Another way would be "attemptToFetch...". The important point is that in this case, all 3 methods are called, but in another case a client might just use "attemptFetch().attemptFetchParent()". Also, calling it "Repository" is wrong, because it's really modelling a single fetch. Maybe I'll fuss with the names to call it "RepositoryLookup" and "attempt" to make clear this is a one-shot, transient artifact that provides certain semantics around a lookup. –  Rob Y Apr 30 at 17:08

Another way to structure a series of conditions like this is to carry a flag, or else test for null (better yet, use Guava's Optional to determine when a good answer is present) in order to chain the conditions together.

Element e = null;

try {
    e = repository.getElement(x);
} catch (NotFoundException e) {
    // nope -- try again!
}

if (e == null) {  // or ! optionalElement.isPresent()
    try {
        return repository.getSimilarElement(x);
    } catch (NotFoundException e1) {
        // nope -- try again!
    }
}

if (e == null) {  // or ! optionalElement.isPresent()
    try {
        return repository.getParentElement(x);
    } catch (NotFoundException e2) {
        // nope -- try again!
    }
}

if (e == null) {  // or ! optionalElement.isPresent()
    //can't recover
    throw new IllegalArgumentException(e);
}

return e;

That way, you're watching the state of the element, and making the right calls based on its state -- that is, as long as you don't have an answer yet.

(I agree with @amon, though. I'd recommend looking at a Monad pattern, with a wrapper object like class Repository<E> that has members E answer; and Exception error;. At each stage check to see if there's an exception, and if so, skip each remaining step. At the end, you're left with either an answer, the absence of an answer, or an exception & you can decide what to do with that.)

share|improve this answer

First, it seems to me there should be a function like repository.getMostSimilar(x) (you should choose a more appropriate name) as there seems to be a logic which is used to find to closest or most similar element for a given element.

The repository can then implement the logic like shown in amons post. That means, the only case an exception has to be thrown is when there is no single element that could be found.

However this is of course only possible if the logics to find the closest element can be encapsulated into the repository. If this is not possible please provide more information how (by which criteria) the closest element can be chosen.

share|improve this answer
    
answers are to answer the question, not to request clarification –  gnat Apr 29 at 22:33
    
Well, my answer is solving his problem as it shows a way to avoid the nested try/catches under certain conditions. Only if these conditions are not met we need more information. Why is this not a valid answer? –  valenterry Apr 30 at 6:31

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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