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?

  • 1
    How do you define "best?" In C# we don't even have checked exceptions. We believe that this makes our development simpler and more reliable. – Robert Harvey Jul 17 at 2:01
  • 2
    How different are the resolutions to AlreadyLocked and Locked? – whatsisname Jul 17 at 2:13
  • @RobertHarvey I consider exceptions part of the logic of the method. The best solution is the one that is the most explicit and most logical. Unlike in C#, we are promoting explicitation over convenience by forcing the catch of exceptional case and making sure no ones calls a method without having a full understanding of what's going on. – Winter Jul 17 at 3:28
  • 1
    These methods should simply be returning a status code. You can inform your users based on the code you get back, and get a performance boost for free (exceptions are expensive, about 1000 to 10000 times more expensive than returning a status code). – Robert Harvey Jul 17 at 3:48
  • 1
    @RobertHarvey "exceptions are expensive, about 1000 to 10000 times more expensive than returning a status code" I've done some analysis on this in Java and the cost of exceptions is often way over-stated. They 'can' be expensive if you have very deep stacks. IIRC, I had to bump up the max stack size significantly to be able to create stacks large enough to make exceptions slow enough to worry about. Even stack pigs like Spring don't do that. As long as you aren't throwing and catching in tight loops, it's premature optimization to avoid exceptions. – JimmyJames Jul 18 at 16:43
up vote 21 down vote accepted

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(); 
  }
}
  • The lock() method that is calling close() was just me being lazy when writing the question, but thanks for making me aware of that! I won't fall for it when writing real code. In my case, the 3 methods are never really called successively. It's just: call one, if there's no exception good, otherwise show a translated dialog and possibly logout (if InvalidTokenException). I have isLocked() and isOpen() methods and they are used in the control flow of the server but they shouldn't be used as a way to check if an error dialog should be shown, that's a job for an exception. – Winter Jul 17 at 14:06
  • For your last point well, yes the using code looks a bit better with DoorAlreadyLockedException but it makes twin classes. – Winter Jul 17 at 14:07
  • 2
    If by twin classes you mean the exceptions themselves let me show you my favorite use of inheritance: public class DoorAlreadyLockedException inherits DoorLockedException {}. Define a class in one line just for the sake of giving it a good name. Choose what you inherit from wisely. I much prefer to inherit from as high up as I can, while not being pointlessly redundant, to avoid a long inheritance chain. – candied_orange Jul 17 at 14:29
  • I feel like renaming the NoOneGaveMeAKey to NoOneGaveMeAKeyException just to be pendantic. Otherwise I agree with the main point which is that before processing, we need to be able to know the state of the object. – Walfrat Jul 18 at 13:30
  • 2
    Devil's advocate - just having isOpen/isClosed isn't good enough. Think about the filesystem. You can check the file does/does not exist, but that can still change by the time you try to read or write it, and an IOException will result. Maybe in this context there really is the chance the state is invalid when you open or close the door. – Tom G Jul 18 at 13:58

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().

  • 4
    Please don't overuse the "exceptions for flow control" meme, if the two situations, for whatever reason, warrant dramatically different courses of action to resolve, then having different exceptions makes sense. Not clear from the example if that's the case though. – whatsisname Jul 17 at 2:15
  • 1
    @whatsisname: If it's a meme that has no basis in fact, then post your own answer to that effect. – Robert Harvey Jul 17 at 2:54
  • 1
    @RobertHarvey Exceptions are used for the "control flow" of the error dialogs and error logging. The main logic application is obviously not run though the catch clauses. I do not think making a distinction between DoorLockedException and DoorAlreadyLockedException suggests that, it's just about having a much more significative error handler for the lock() method at the cost of having an almost duplicated class. – Winter Jul 17 at 3:26
  • 1
    If you call lock() and the door is locked, you already know that the door was already locked. You don't need to be told what you already know. – Robert Harvey Jul 17 at 3:31
  • 2
    @ewan: Oh, ffs. Call it anything you want; an enumeration, a result, whatever. It's the right approach. – Robert Harvey Jul 17 at 15:06

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.

  • 1
    "In more typical use of exception handling, you have a great DISTANCE" We aren't analyzing what is typical. We're designing a way for it to work. Many good designs catch their exceptions at the very next level in the call stack and in an extremely limited scope. That shouldn't be dismissed as flow control when all they do is recover from the problem. The best exception designs start by planing how to recover before defining the exceptions. Then the exception names don't have to be about reporting the problem. They're about finding the solution. You can use the msg string to report the problem. – candied_orange Jul 17 at 20:09
  • "We aren't analyzing what is typical. We're designing a way for it to work.", Agreed, that is what you were doing. It is not what I was doing. If you look at the headline, it asked one question. If you look at the body of the message, it asked three different (related) questions. You answered the body questions (to a degree - really- more you criticized the sample code). I answered the headline question. – Lewis Pringle Jul 17 at 22:23
  • BTW, you claim "Many good designs catch their exceptions at the very next level in the call stack and in an extremely limited scope". I'm not sure I would fully disagree with that. But I have to say, after 40 years of programming, I'm having a hard time coming up with an example to support that claim. I realize its a bit of a spin-off (and I maybe not handling this correctly - new to stackoverflow) - but would you mind offering an example or two where such localized try/catch handling is the best solution? It is certainly not a typical/common pattern. – Lewis Pringle Jul 17 at 22:25
  • An example huh? Let me introduce you to Jon Skeet – candied_orange Jul 17 at 22:33
  • OK, that's not at all an example of what you claimed. You said "Many good designs catch their exceptions at the very next level in the call stack and in an extremely limited scope". Your example was just translating an API that only produced exceptions on error into one that produced some sentinel value instead. Maybe this is just a wording difference between the two of us. I wouldn't describe that as a "good design" but a 'localized hack to get around an inconvenient design choice in a tool you are using'. – Lewis Pringle Jul 17 at 22:55

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.

  • But in most applications you can't keep the reference to those objects, as the database can be changed anytime. Good creative effort though – Winter Jul 18 at 21:28

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.

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.

  • But those exception types are really undescriptives. What does it even mean to have a door "stuck" when you can only open, close or lock a door? – Winter Jul 19 at 13:25
  • This is just swallowing the exceptions and does nothing with them. This is not appropriate in a context where you wan't to know what's going on. This feels like the C# way to do of my Java example. There's a reason why I'm working with Java. – Winter Jul 19 at 13:27
  • Well, that it's stuck. It cannot move. So you cannot open it and you cannot close it. It's not a logical state you need to fix by calling another method, it's an exceptional state, that you cannot solve through the door service. – nvoigt Jul 19 at 13:27
  • And it's not swallowing anything, it's not producing an exception for a non-exceptional program state. If my wife ask me to please close the balcony door and I get up and find out it's already closed, I don't run around and scream and panic. I just go back and say "it's closed now". – nvoigt Jul 19 at 13:30
  • Exactly, with checked exceptions, the exception can't get back to the start of the program is has to be catched by the caller. On the call you just ignore the exception or log a warning and you are good. Also what you call "exceptional exceptions" are just RuntimeExceptions. DoorStuckException and HouseOnFireException should not be checked, they aren't meant to be catched on all calls. – Winter Jul 19 at 13:35

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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