Wrong design decision to exclude an exception from an accessor?

Question

I read several answers of re pro and con that you selected an exception in the accessory, but I thought that I would consider a 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() throws an Exception when strings not been set. Will this situation be better handled by the method?

Answer 1

Yes this is bad. I suggest initializing the string variable in the declaration, for example:

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

and you can replace the installer with something like

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

Therefore, there is no NPE capability.

(Alternatively, do not initialize the string variable in the declaration, add initialization to the addToList method and use the code to check if it returns.)

As for why this is bad, it's hard to say with such a far-fetched example, but it seems that there are too many state changes, and you punish the user of this class for him, forcing the user to handle the exception. There is also a problem that once methods in your program start throwing Exception then that tends to metastasize throughout your code base.

Checking that this instance is populating, something needs to be done, but I think it should be done somewhere that has more knowledge of what is happening than in this class.

Answer 2

It really depends on what you do with the list you get. I think a more elegant solution would be to return Colletions.EMPTY_LIST or, 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 there is no difference between the lines and the list of lines. The first should be represented by an empty list, the second by returning null or an exception.

Answer 3

The right way to handle this, assuming the list should be set up 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, you do not need to declare it in the method, so you will not pollute the client code with a lot of try / catch noises. In addition, since this condition can be avoided (i.e., client code can guarantee that the list has been initialized), this is a programming error, and therefore an unchecked exception is acceptable and preferable.

Answer 4

In general, this is a designer smell.

If this is actually an error, the lines are not initialized, either remove the installer, or force the restriction in the constructor, or do as Bohemian says, and throw an IllegalArgumentException with an explanatory message. If you do the first, you get erratic behavior.

If this is not an error, the strings are NULL, and then consider initializing the list to an empty list and force it to set a non-zero value in the installer, similar to the one above. This has the advantage that you do not need to check for null in any client code.

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

This can greatly simplify client code.

EDIT: Well, I just saw that you are serializing from JSON, so you cannot remove the constructor. Therefore, you need to either:

• throw an IllegalArgumentException from getter
• If the strings are null, return Collections.EMPTY_LIST.
• or better: add a validation check immediately after deserialization, where you specifically check the value of the strings and throw a reasonable exception.

Answer 5

I think you are faced with a more important question behind the original one: should you work to prevent exceptional cases in getters and setters?

Answer: yes, you must work to avoid trivial exceptions.

That you are here is an effectively lazy instance that is in no way motivated in this example:

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

There are two problems in this example:

• Your example is not thread safe. This check for null can be successful (i.e., find that the object is null) on two threads simultaneously. Then your instance creates two different list of strings. Non-deterministic behavior will occur.

• In this example, there is no good reason to defer instantiation. This is not an expensive or complicated operation. This is what I mean by “work on eliminating trivial exceptions”: it is worth spending cycles creating a useful (but empty) list to make sure that you do not throw substitution null replacement exceptions around.

Remember that when you introduce an exception from external code, you basically hope that the developers know how to do something reasonable with it. You also hope that there is no third developer in the equation who wrapped everything in eater for an exception, 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 use the Zero Object Sample to indicate to the future code that your example has not been installed. However, Null Object works as non-exclusive code and, therefore, has no 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. } } 

Answer 6

I would consider Java Beans . It is probably best not to throw java.lang.Exception, but instead use java.beans.PropertyVetoException and register your bean as java.beans.VetoableChangeListener and fire the corresponding event. This is too much, but it will correctly determine what you are trying to do.

Answer 7

It really depends on what you are trying to do. If you are trying to enforce the condition that this parameter was called before the recipient is called, a case may arise to throw an IllegalStateException.

Answer 8

I suggest throwing an exception to the setter method. Is there any special reason why it is better not to do this?

 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 specific context, is it not possible to simply pass List<String> strings directly to the constructor? You might not even need the setter method.