private constructor for static methods

Question

I have created the following class to validate certain values with constants. Why i am getting the following error? As the class need not to be initiated to use static methods, but still why it is trying to initiate. I am using java 1.6 Is this a good practice to do ?

public final class Approver{

    // Avoids initiating this class
    private Approver() {
    }


    private static final List<String> APPROVED_LENGTH= new ArrayList<String>() {
        {
            addAll(KM_APPROVED_LIST);
            addAll(LM_APPROVED_LIST);
        }
    };

    private static final List<String> KM_APPROVED_LIST = new ArrayList<String>() {
        {
            add("L");
            add("G");
                    // so on
        }

    };
    private static final List<String> LM_APPROVED_LIST = new ArrayList<String>() {
        {
            add("P");
            add("K");
                    // so on
        }

    };
    public static boolean isApproved(String lenth) {
        return APRROVED_LENGTH.contains(length);
    }

From another class

if(Approver.isApproved("K"))

{......}

error

Caused by: java.lang.NoClassDefFoundError: Could not initialize class ...Approver.class

It's trying to load the class, not create an instance. Is it on your classpath? — Andy Thomas, Jun 04 '13 at 14:19
Correct me if I'm wrong, but I believe the error lies in the `addAll` of the other lists when they haven't been defined yet. — , Jun 04 '13 at 14:19
The very idiom of using anonymous subclasses of `ArrayList` just to initialize it is bad practice. No serious programmer will write that. — Marko Topolnik, Jun 04 '13 at 14:21
user1595858: please pay attention to the stack trace. There's a "root cause" part in the bottom. It should say `NullPointerException`, which is so much more self-explaining. If you can't interpret stack traces, then you should post them in their entirety in the question. — BalusC, Jun 04 '13 at 14:26
@MarkoTopolnik what do you suggest then? Using static constructor is better? — user1595858, Jun 04 '13 at 14:41
You can for example have an init method where you fill all the lists. — Marko Topolnik, Jun 04 '13 at 14:52
@Marko - You're right, I misspoke. Still unnecessarily complex. — Andy Thomas, Jun 04 '13 at 15:12

Jon Skeet · Accepted Answer · 2013-06-04T14:31:55.937

If you'd looked at the rest of the error, I think you've had seen what's wrong. In this statement:

private static final List<String> APPROVED_LENGTH= new ArrayList<String>() {
    {
        addAll(KM_APPROVED_LIST);
        addAll(LM_APPROVED_LIST);
    }
};

you're using KM_APPROVED_LIST and LM_APPROVED_LIST while they're both null... which means you're calling addAll(null) which will throw a NullPointerException.

For example, here's the exception I see in a short test app:

Exception in thread "main" java.lang.ExceptionInInitializerError
        at Test.main(Test.java:43)
Caused by: java.lang.NullPointerException
        at java.util.ArrayList.addAll(Unknown Source)
        at Approver$1.<init>(Test.java:14)
        at Approver.<clinit>(Test.java:12)
        ... 1 more

At that point, it should be pretty clear what's going on.

It would be cleaner to initialize everything in a static block, IMO - it takes all the ordering concerns away - as well as avoiding the nasty anonymous classes:

private static final List<String> APPROVED_LENGTH;
private static final List<String> KM_APPROVED_LIST;
private static final List<String> LM_APPROVED_LIST;

static {
    KM_APPROVED_LIST = new ArrayList<String>();
    KM_APPROVED_LIST.add("L");
    KM_APPROVED_LIST.add("G");
    LM_APPROVED_LIST = new ArrayList<String>();
    LM_APPROVED_LIST.add("P");
    LM_APPROVED_LIST.add("K");
    APPROVED_LENGTH = new ArrayList<String>();
    APPROVED_LENGTH.addAll(KM_APPROVED_LIST);
    APPROVED_LENGTH.addAll(LM_APPROVED_LIST);
}

Alternatively, you could reorder the fields and rely on the static variable initializer ordering - but preferably using Guava to make the code much clearer:

private static final List<String> KM_APPROVED_LIST =
    Lists.newArrayList("L", "G");
private static final List<String> LM_APPROVED_LIST =
    Lists.newArrayList("P", "K");
private static final List<String> APPROVED_LENGTH =
    Lists.newArrayList(Iterables.concat(KM_APPROVED_LIST, LM_APPROVED_LIST));

I should point out that just reordering the field declarations so that APPROVED_LENGTH is declared last fixes the problem - but it's still not nice code as-is, IMO.

You might also want to consider making these immutable lists, too.

what about using List places = Arrays.asList("Buenos Aires", "Córdoba", "La Plata"); as show in http://stackoverflow.com/a/1005089/1595858 — user1595858, Jun 04 '13 at 14:56
@user1595858: Well it depends on what kind of list you want. That will give you an array-backed list which is mutable but fixed-size. If you're happy with that, fair enough. — Jon Skeet, Jun 04 '13 at 14:59
Is your initial suggestion having static constructor is also mutable right? — user1595858, Jun 04 '13 at 15:04
@user1595858: Yes, it's just using `ArrayList` - but as I say, you should consider using a truly immutable list (or set) if you don't *want* to be able to mutate it. It will make the code clearer. — Jon Skeet, Jun 04 '13 at 15:06

score -1 · Answer 2 · answered Jun 04 '13 at 14:22

-1

The error is happening because you're trying to create an anonymous inner class in a static context:

private static final List<String> APPROVED_LENGTH= new ArrayList<String>() {
    {
        addAll(KM_APPROVED_LIST);
        addAll(LM_APPROVED_LIST);
    }
};

I'm surprised that the compiler allows this to happen, but I suppose it doesn't do a lot of semantic checks (ie, verifying that the class can be instantiated). Regardless, here's how to fix it (and a better way to initialize static lists/maps in general):

private static final List<String> APPROVED_LENGTH= new ArrayList<String>();
static {
    addAll(KM_APPROVED_LIST);
    addAll(LM_APPROVED_LIST);
};

answered Jun 04 '13 at 14:22

parsifal

15
1

@JonSkeet also makes a valid comment. Once you add the static initializer, you'll probably get a compile-time error saying that `KM_APPROVED_LIST` hasn't been declared. Re-arrange the declarations. – parsifal Jun 04 '13 at 14:25
1

This is not the cause at all. Whoever upvoted this has to reconsider the upvote and go back to basic Java books. – BalusC Jun 04 '13 at 14:25
No, it's not happening because of the "anonymous inner class in a static context" - that's absolutely fine. The problem is the call to `addAll`. – Jon Skeet Jun 04 '13 at 14:28
@BalusC - Do you have an explanation for *why* this is not the cause? – parsifal Jun 04 '13 at 14:28
@parsifal: It's not the cause because it's perfectly valid code, other than passing `null` to `addAll`. – Jon Skeet Jun 04 '13 at 14:29
@JonSkeet - are you actually telling me that it's possible to create an anonymous inner class without an enclosing instance? Do you have a JLS reference to back this up? – parsifal Jun 04 '13 at 14:32
@parsifal: Yes, it's entirely valid to do this - just try it. Reorder the code so that APPROVER_LENGTH is declared last, and you'll find it's fine. I'm looking up a JLS reference now, but it's taking a while due to flaky networking. – Jon Skeet Jun 04 '13 at 14:34
1

@parsifal: JLS section 15.9.5.1 includes: "If S is not an inner class, or if S is a local class that occurs in a static context, then the anonymous constructor has one formal parameter for each actual argument to the class instance creation expression in which C is declared" - which makes it pretty clear that it's valid to occur within a static context. Basically, there's no enclosing instance. At this point it's an anonymous class, but not an anonymous *inner* class. – Jon Skeet Jun 04 '13 at 14:36
Indeed, anonymous class != inner class. See further also e.g. http://docs.oracle.com/javase/tutorial/java/javaOO/nested.html and http://docs.oracle.com/javase/tutorial/java/javaOO/anonymousclasses.html – BalusC Jun 04 '13 at 14:37
Is it better to use static constructor with right order instead of private constructor? – user1595858 Jun 04 '13 at 14:40
@user1595858: The two are entirely orthogonal. The private constructor *just* stops the class itself from being instantiated outside. It's got nothing to do with static initialization. – Jon Skeet Jun 04 '13 at 15:00

private constructor for static methods

From another class

error

2 Answers2