Bad design decision to throw an exception from an accessor?

Question

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
    }
  }
}

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

Answer 1

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.