IllegalStateException good practices

Question

I know what IllegalStateException is for, but I wonder if in some cases I should actually use it or it's just a matter of design.

Let's say I have a Tournament class that models a sports tournament and a tournament has a Schedule, which needs to be calculated. So unless you actually launch the resolution process that generates the schedule, you don't have a schedule, and trying to get it before this state actually happens should be treated somehow. I thought this would be an appropriate scenario where an IllegalStateException should be thrown: if the schedule hasn't been calculated yet, throw it. Let's see:

public Tournament {
    private Schedule schedule;

    public boolean solve() {
        boolean solved = solver.solve();
        if (solved)
            schedule = solver.getSolution();
        return solved;
    }

    public Schedule getSchedule() {
        if (schedule == null)
            throw new IllegalStateException("Schedule has not been calculated yet");
        return schedule;
    }

    // This udpates the current schedule with the "next" solution. 
    // Same logic regarding the ISE is applied
    public boolean nextSchedule() {
        if (schedule == null)
            throw new IllegalStateException("Schedule has not been calculated yet");
        boolean hasNextSolution = solver.update();
        if (hasNextSolution)
            schedule = solver.getSolution();
       return hasNextSolution;
    }
}

Is ISE being used appropriately here? Should changes be made in the class design instead?

What are the best practices?

Answer 1

As it stands, your class is basically an iterable.

1. getSchedule() is more or less next()
2. nextSchedule() is more or less hasNext()
3. solve is a combination hasNext() and iterator()

A standard iterator usage pattern is:

Iterator iter = item.iterator();
while(iter.hasNext()) {
  doSomething(iter.next())
}

Your usage pattern is:

if (item.solve()) {
   doSomething(item.getSchedule());
   while (item.nextSchedule()) {
       doSomething(item.getSchedule());
   }
}

So, basically, your class is a poor version of an iterable. The solution: use an iterable.

class Tournament {
    Iterable<Schedule> calculateSchedules();
}

Now, we are using a standard interface that everybody knows and can be used with generic code.

Answer 2

I recommend making it so it is impossible to use an object incorrectly.

The way you describe the problem is like this:

Widget w = ...;
w.method1();
w.method2();
// Happy happy joy joy!

However, it is possible to do this:

Widget w = ...;
w.method2();
// Stimpy you eeediot! You broke the widget!

The solution is to split your objects into more pieces, and make transient dependencies. Object A gives you object B, which gives you object C. You cannot get both B and C from A, perhaps in the wrong order.

This is essentially using static (compiler-enforced) checking to enforce temporal decoupling.

That was the general solution - in your specific example, I would simply make Schedule a required constructor argument to Tournament. If there is some circular dependency there (unclear from the code), I would create some generator or builder class which gathers the necessary data and ensures the objects are fully constructed and initialized before making them available for use.

For more information, please see this answer of mine to a similar question.

Answer 3

The best practice is to not allow the object to have an illegal state.

Sure that means you never get to throw this exception but just because it exists doesn't mean using it is a good thing.

Now sure some clever person can probably come up with a case that demands it's use but my policy here is 'avoid when at all possible'.

So how should you redesign this?

Try caching. getSchedual() doesn't even try to create a schedule. It just punishes you for asking for it before it exists. If it exists fine return it. If it doesn't why not at least try creating it?

If solver.solve() is someone else's baby I understand but if it's yours why are you mixing c style error booleans with OOP style exceptions? Pick one style and stick with it.

One last nitpick. It's almost always better to tell a behavior object to do something than it is to ask it what's going on. When I use tournament why do I have to know about the schedule? Why can't I just tell it to print who won?

Answer 4

• I think the use you are making of IllegalStateException is OK.
• Sometimes you cannot avoid having some temporal coupling. Take for example the Matcher class in the Java JDK. It throws this exception if you try to find a match on a nonexistent pattern.
• Some may argue that you should return a null schedule and let the client code handle NullPointerException or check it for null and act accordingly, but I think that would be against the design that to me is very similar to the Java Matcher/Pattern classes that use regexes to make match operations on a character sequence.

I recommend though that you add a boolean hasSchedule() method to help the client code avoid the exception. But I think that would be a nicety and not something really necessary.

Here you'll find some legitimate uses of IllegalStateException:

https://stackoverflow.com/questions/12698275/whats-the-intended-use-of-illegalstateexception