Bad design decision to throw an exception from an accessor?

I have read some answers re the pro's and con's of throwing an exception within an accessor, but I thought I would broach my specific question with an example:

public class App {
  static class Test {       
    private List<String> strings;

    public Test() {
    }

    public List<String> getStrings() throws Exception {
        if (this.strings == null)
            throw new Exception();

        return strings;
    }

    public void setStrings(List<String> strings) {
        this.strings = strings;
    }
}

public static void main(String[] args) {
    Test t = new Test();

    List<String> s = null;
    try {
        s = t.getStrings();
    } catch (Exception e) {
        // TODO: do something more specific
开发者_StackOverflow    }
  }
}

getStrings() is throwing an Exception when strings has not been set. Would this situation be better handled by a method?

Yes, that's bad. I suggest initializing the strings variable in the declaration, like:

private List<String> strings = new ArrayList<String>();

and you could replace the setter with something like

public void addToList(String s) {
     strings.add(s);
}

So there's no possibility of a NPE.

(Alternatively don't initialize the strings variable in the declaration, add the initialization to the addToList method, and have the code using it check getStrings() to see if it gets null back.)

As to why it's bad, it's hard to say with such a contrived example, but it seems like there's too much state-changing and you're penalizing the user of this class for it by making the user handle the exception. Also there's the issue that once methods in your program start throwing Exception then that tends to metastasize all over your codebase.

Checking that this instance member gets populated needs to be done somewhere, but I think it should be done somewhere that has more knowledge of what is going on than in this class.

It really depends on what are are doing with the list you get. I think a more elegant solution would be to return a Colletions.EMPTY_LISTor, as @Nathan suggests, an empty ArrayList. Then the code that uses the list can check if it is empty if it wants to, or just work with it like with any other list.

The difference is the distinction between there are no strings and there is no list of strings. The first should be represented by an empty list, the second by either returning null or throwing an exception.

The correct way to handle this, assuming it is a requirement that the list be set elsewhere before calling the getter, is as follows:

public List<String> getStrings() {
    if (strings == null)
        throw new IllegalStateException("strings has not been initialized");

    return strings;
}

Being an unchecked exception, it isn't required that you declare it on the method, so you don't pollute the client code with lots of try/catch noise. Also, because this condition is avoidable (ie the client code can ensure the list has been initialized), it is a programming error, and therefore an unchecked exception is acceptable and preferable.

In general, this is a design smell.

If it really is an error that strings is not initialized, then either remove the setter, and enforce the constraint in a constructor, or do as Bohemian says and throw an IllegalArgumentException with an explanatory message. If you do the former, you get fail-fast behaviour.

If it is not an error is strings is null, then consider initialising the list to an empty list, and enforce it to be not-null in the setter, similarly to the above. This has the advantage that you don't need to check for null in any client code.

for (String s: t.getStrings()) {
  // no need to check for null
}

This can simplify client code a lot.

EDIT: Ok, I've just seen that the you are serializing from JSON, so you can't remove the constructor. So you'll need to either:

1. throw an IllegalArgumentException from the getter
2. if strings is null, return a Collections.EMPTY_LIST.
3. or better: add a validation check just after the deserialization where you specifically check the value of strings, and throw a sensible exception.

I think you've exposed a more important question behind the original one: should you work to prevent exceptional cases in getters and setters?

The answer is yes, you should work to avoid trivial exceptions.

What you have here is effectively lazy instantiation that is not at all motivated in this example:

In computer programming, lazy initialization is the tactic of delaying the creation of an object, the calculation of a value, or some other expensive process until the first time it is needed.

You have two problems in this example:

1. Your example is not thread safe. That check for null can succeed (i.e., find that the object is null) on two threads at the same time. Your instantiation then creates two different lists of strings. Non-deterministic behavior will ensue.

2. There is no good reason in this example to defer the instantiation. It's not an expensive or complicated operation. This is what I mean by "work to avoid trivial exceptions": it is worth investing the cycles to create a useful (but empty) list to ensure that you aren't throwing delayed detonation null pointer exceptions around.

Remember, when you inflict an exception on outside code, you're basically hoping that developers know how to do something sensible with it. You're also hoping that there isn't a third developer in the equation whose wrapped everything in an exception eater, just to catch and ignore exceptions like yours:

try {
    // I don't understand why this throws an exception.  Ignore.
    t.getStrings();
} catch (Exception e) {
    // Ignore and hope things are fine.
}

In the example below, I am using the Null Object pattern to indicate to future code that your example hasn't been set. However, the Null Object runs as non-exceptional code and, therefore, doesn't have the overhead and impact on the workflow of future developers.

public class App {   
    static class Test {            
        // Empty list
        public static final List<String> NULL_OBJECT = new ArrayList<String>();

        private List<String> strings = NULL_OBJECT;

        public synchronized List<String> getStrings() {
            return strings;
        }      

        public synchronized void setStrings(List<String> strings) {         
            this.strings = strings;     
        } 
    }  

    public static void main(String[] args) {     
        Test t = new Test();      

        List<String> s = t.getStrings();

        if (s == Test.NULL_OBJECT) {
            // Do whatever is appropriate without worrying about exception 
            // handling.

            // A possible example below:
            s = new ArrayList<String>();
            t.setStrings(s);
        }

        // At this point, s is a useful reference and 
        // t is in a consistent state.
    }
}

I would consult the Java Beans specification on this. Probably would be better not to throw a java.lang.Exception but instead use the java.beans.PropertyVetoException and have your bean register itself as a java.beans.VetoableChangeListener and fire the appropriate event. It is a tad overkill but would correctly identify what you are trying to do.

It really depends on what you are trying to do. If you have attempting to enforce the condition that the setting has been called prior to the getter being called, there might be a case for throwing an IllegalStateException.

I'd suggest to throw the exception from the setter method. Is there any special reason why wouldn't it better to do so?

public List<String> getStrings() throws Exception {
    return strings;
}

public void setStrings(List<String> strings) {
    if (strings == null)
        throw new Exception();

    this.strings = strings;
}

Also, depending on the particular context, wouldn't it be possible to simply pass the List<String> strings directly by the constructor? That way maybe you wouldn't even need a setter method.