views:

390

answers:

6

Effective Java (Second Edition), Item 4, discusses using private constructors to enforce noninstantiability. Here's the code sample from the book:

public final class UtilityClass {
    private UtilityClass() {
        throw new AssertionError();
    }
}

However, AssertionError doesn't seem like the right thing to throw. Nothing is being "asserted", which is how the API defines the use of AssertionError.

Is there a different Throwable that's typically in this situation? Does one usually just throw a general Exception with a message? Or is it common to write a custom Exception for this?

It's pretty trivial, but more than anything I guess I'm just curious about it from a style and standards perspective.

+7  A: 

There is an assertion: "I'm asserting that this constructor will never be called". So, indeed, AssertionError is correct here.

Chris Jester-Young
Thanks. I guess I've just been looking at AssertionError the wrong way.
Rob Hruska
Strongly disagree here, see below. You need to be able to find your way to the place where the exception started; you also should make the exception give you, or a naive reader of the code, a good hint what happened. An AssertionError not associated with an assertion does neither.
Charlie Martin
@Charlie: surely the stack trace is indication enough? Plus an Exception will get caught by catch (Exception e), an error (rightly) won't.
cletus
I agree with cletus's comments here, the stacktrace is invaluable, and exception chaining is the best feature since sliced bread, because it allows you to preserve the stack trace far and wide. :-)
Chris Jester-Young
Incorrect, see the javadoc: http://java.sun.com/j2se/1.4.2/docs/api/java/lang/AssertionError.html
Charlie Martin
@Charlie the javadoc says "Thrown to indicate that an assertion has failed." You are looking at an assertion as "using the assert keyword" instead of "the programmer regards this as a bug"
Craig P. Motlin
You're right, I am. That's why it's called the "assert" statement. Just like I wouldn't use a NullPointerException to indicate a name that has value 0 instead of a number in range, even if the number was meant as an index.
Charlie Martin
A: 

A broken assertion means that you've broken a contract specification of your code. So it's the right thing here.

However, as I assume you'll be privately instantiating an instance, it will also call the constructor and cause an error- unless you have another constructor?

Steve B.
My understanding is that this utility class provides a number of static methods only, so the constructor will not be called.
Matthew Murdoch
Matthew is correct. Technically nothing even needs to be thrown since the constructor's private. Throwing something insures that the class itself doesn't call the constructor.
Rob Hruska
A: 

What about IllegalAcessError ? :)

OscarRyz
I'm not sure if I like that one, since it's a subclass of IncompatibleClassChangeError. But it sounds nice. :)
Rob Hruska
A: 

No no no, with all due respect to Josh Bloch, never throw an AssertionError unless it's from an assertion. If you want an AssertionError here, throw it with assert(false). Then someone reading the code can find it later.

Even better, define your own exception, say CantInstantiateUtilityClass. then you'll have code that says

try {
    // some stuff
} catch (CantInstantiateUtilityClass e) {
    // react
}

so that the reader of the catcher knows what happened.

Charlie Martin
_Strongly_ disagree. (Bloch, not Block, by the way.) The problem with "assert false" is that if you have assertions turned off, the assertion will fail to be thrown. People should find assertions by looking for AssertionError as well as assert.
Chris Jester-Young
I also strongly disagree with CantInstantiateUtilityClass as an exception type: first, it doesn't contain the word Exception or Error at the end, and second, AssertionError allows you to give an error message, and that should be used in terms of hinting people as to what happened.
Chris Jester-Young
Dammit, I've known Josh for ten years, you'd think I could spell his name. Okay, so tag Exception on if you like; but it shouldn't be an assertion error unless it's caused by an assertion. Someone new reading the code won't have a clue, and your example doesn't offer one.
Charlie Martin
-1 An exception will get caught by some (possibly random) catch (Exception e) block. An Error won't be. Creating a custom exception for something that can only happen by modifying the source code or having a custom ClassLoader is terrible advice.
cletus
-1 Why should your code have to catch CantInstantiateUtilityClass? This is something that should never happen. Which is exactly when assertions are appropriate. Others have pointed out the problem with the assert keyword.
Craig P. Motlin
Because (1) it's undesirable for a program to handle errors by giving up and dying. Either this program is catching AssertionError -- which obviates your objection -- or it handles the error by abnormal termination.
Charlie Martin
And because (2) an assertion error is defined, quote, "Thrown to indicate that an assertion has failed." http://java.sun.com/j2se/1.4.2/docs/api/java/lang/AssertionError.html . Not "thrown when J Random Programmer thinks a bad thing happened."
Charlie Martin
Calling the constructor is a bug, so we should assert that it is never called. The assert keyword is deeply flawed and should never be used. That makes "throw new AssertionError()" the new assertion mechanism.
Craig P. Motlin
I disagree, the program should give up and die if the constructor is called, since it is obviously a bug. assert(false) will also cause the program to give up and die, also with an AssertionError, so I'm not sure why you prefer the assert keyword.
Craig P. Motlin
I'm sure you'll be just thrilled when your airplane control system gives up and dies instead of trying to recover. Abnormal termination is not a a reliability strategy.
Charlie Martin
Far worse for the airplane system to muddle on in an inconsistent state no programmer considered possible. Better to give up and let the pilots take over. :)Seriously though, disabling assertions is fine if you work on pacemakers and such. Everyone else should avoid it.
Craig P. Motlin
+2  A: 

UnsupportedOperationException sounds like the best fit, though a checked exception would be even better, since it might warn someone erroneously instantiating the class at compile time.

Michael Borgwardt
Hehe, declare it "throws Throwable", and throw an AssertionError anyway. :-)
Chris Jester-Young
+1  A: 

I like including Bloch's comment:

// Suppress default constructor for noninstantiability

Or better yet putting it in the Error:

private UtilityClass()
{
    throw new AssertionError("Suppress default constructor for noninstantiability");
}
Craig P. Motlin