Should reused exceptions types be favored over single use ones?

Question

Let's say I have Doors that are managed by a DoorService. The DoorService is in charge of opening, closing and locking the doors that are stored on the database.

public interface DoorService
{
    void open(Door door) throws DoorLockedException, DoorAlreadyOpenedException;

    void close(Door door) throws DoorAlreadyClosedException;

    /**
     * Closes the door if open
     */
    void lock(Door door) throws DoorAlreadyLockedException;
}

For the lock method, there's a DoorAlreadyLockedException and for the open method there's a DoorLockedException. This is an option but there's other options possible:

1) Use DoorLockedException for everything, is a bit awkward when catching the exception on a lock() call

try
{
    doorService.lock(myDoor);
}
catch(DoorLockedException ex) // door ALREADY locked
{
    //error handling...
}

2) Have the 2 exceptions types DoorLockedException and DoorAlreadyLockedException

3) Have the 2 exceptions types but let DoorAlreadyLockedException extends DoorLockedException

Which is the best solution and why?

Answer 1

I know people make a big deal out of using exceptions for flow control but that isn't the biggest problem here. I see a huge command query separation violation. But even that's not the worst.

No, the worst is right here:

/**
 * Closes the door if open
 */

Why the hell does everything else blow up if your assumption about the doors state is wrong but lock() just fixes it for you? Forget that this makes it impossible to lock the door open, which is absolutely possible and occasionally useful. No the problem here is you've mixed two different philosophies for dealing with incorrect assumptions. That's confusing. Don't do that. Not at the same level of abstraction with the same naming style. Ow! Take one of those ideas outside. The door service methods should all work the same way.

As for the Command Query Separation violation I shouldn't have to try to close a door to find out if it's closed or open. I should be able to just ask. The door service doesn't provide a way to do that without possibly changing the state of the door. That makes this so much worse then commands that also happen to return values (a common misunderstanding of what CQS is about). Here, state changing commands are the only way to do queries! Ow!

As for exceptions being more expensive than status codes, that's optimization talk. Fast enough is fast enough. No the real problem is that humans don't expect exceptions for typical cases. You can argue over what's typical all you like. For me the big question is how readable you're making the using code.

ensureClosed(DoorService service, Door door){
  // Need door closed and unlocked. No idea of its state. What to do?
  try {
    service.open(door)
    service.close(door)
  } 
  catch( DoorLockedException e ){
    //Have no way to unlock the door so give up and die
    log(e);
    throw new NoOneGaveMeAKeyException(e);      
  }
  catch( DoorAlreadyOpenedException e ){
    try { 
      service.close(door);
    }
    catch( DoorAlreadyClosedException e ){
      //Some multithreaded goof has been messing with our door.
      //Oh well, this is what we wanted anyway.
      //Hope they didn't lock it.
    }
  }
}

Please don't make me write code like this. Please give us isLocked() and isClosed() methods. With those I can write my own ensureClosed() and ensureUnlocked() methods that are easy to read. Ones that only throw if their post conditions are violated. I'd rather just find you've already written and tested them of course. Just don't mix them together with the ones that throw when they can't change the state. At the very least give them distinguishing names.

_{Whatever you do, please don't call anything tryClose(). That's a terrible name.}

As for DoorLockedException alone vs also having DoorAlreadyLockedException ill say this: it's all about the using code. Please don't design services like this without writing the using code and looking at the mess you're creating. Refactor and redesign until the using code is at least readable. In fact, consider writing the using code first.

ensureClosed(DoorService service, Door door){
  if( !service.isClosed(door) ){
    try{
      service.close(door);
    }
    catch( DoorAlreadyClosedException e ){
      //Some multithreaded goof has been messing with our door.
      //Oh well, this is what we wanted anyway.
      //Hope they didn't lock it.
    }
  } else {
    //This is what you wanted, so quietly do nothing. 
    //Why are you even here? Who bothers to write empty else conditions?
  }
}

ensureUnlocked(DoorService service, Door door){
  if( service.islocked(door) ){
    throw new NoOneGaveMeAKeyException(); 
  }
}

Answer 2

Making a distinction between DoorLockedException and DoorAlreadyLockedException suggests that different exception types are being used for flow control, a practice that is already widely considered an antipattern.

Having multiple exception types for something this benign is gratuitous. The additional complexity is not outweighed by the additional benefits.

Just use DoorLockedException for everything.

Better yet, simply do nothing. If the door is already locked, then it will still be in the correct state when the lock() method completes. Don't throw exceptions for conditions that are unremarkable.

If you're still uncomfortable with that, then rename your method ensureDoorLocked().

Answer 3

The subject of this question "Should recycled exceptions types be favored over single use ones?" appears to have been ignored, not only in the answers, but in the details for the questions (the answers were to the details of the question).

I agree with most of the criticism of the code the respondents have offered.

But as an answer to the headline question, I would say that you should strongly prefer re-use of 'recycled exceptions' over 'single use ones'.

In more typical use of exception handling, you have a great DISTANCE in the code between where the exception is detected and thrown, and where the exception is caught and handled. That means using private (modular) types in exception handling generally won't work well. A typical program with exception handling - will have a handful of top-level locations in the code where exceptions are handled. And as those are top-level locations, its hard for them to have detailed knowledge about narrow aspects of particular modules.

Instead, they will likely have a high level handlers for a few high level problems.

The only reason using detailed custom exception classes appears to make sense in your sample, is because (and here I'm parroting the other respondents) - you shouldn't be using exception handling for ordinary control flow.

Answer 4

An unexplored alternative. You may be modelling the wrong thing with your classes, what if you had these instead?

public interface OpenDoor
{
    public ClosedDoor close();
    public LockedDoor lock();
}

public interface ClosedDoor
{
    public OpenDoor open();
    public LockedDoor lock();
}

public interface LockedDoor
{
    // ? unlock? open?
}

Now it's impossible to attempt anything that is an error. You don't need any exceptions.

Answer 5

I'll answer the original question, rather than criticising the example.

Simply put, if you expect that the client might need to react differently for each case, it warrants a new exception type.

If you decide to use multiple exception types and you nevertheless expect that the client might in some cases want to create a common catch clause for them, you should probably create an abstract superclass that they both extend.

Answer 6

This is pseudocode but I think you will get what I mean:

public interface DoorService
{
    // ensures the door is open, uses key if locked, returns false if lock without a key or key does not fit
    bool Open(Door door, optional Key key) throws DoorStuckException, HouseOnFireException;

    // closes the door if open. already closed doors stay closed. Locked status does not change.
    void Close(Door door) throws throws DoorStuckException, HouseOnFireException;

    // closes the door if open and locks it
    void CloseAndLock(Door door, Key key) throws DoorStuckException, HouseOnFireException;
}

Yes, exception type reuse makes sense, if you use exceptions for truly exceptional things. Because truly exceptional things are mostly cross-cutting concerns. You were using Exceptions basically for control flows and that leads to those problems.