Smarter setter? Good or Bad Idea?

Question

Hi there

In a GWT solution. (so this is java code that is then compiled to javascript). There are of course some classes.

Is it a good idea to make the setter check for Null on a String field?

something like this

public void setSomeField(String someField){
  if (null != someField)
    this.someField = someField;
  else
    this.someField = String.Empty;
}

Is this a good or bad idea? On the one had it will make coding easier as i wont have to check for null , on the other hand it would make me probably forget that I have to do this for other strings.

Thoughts? Thanks

Answer 1

I say if such a logic is needed in your application, the setter is the place to put it. The main reason to have a get/set wrap around a private var is to be able to put logic around the access.

To answer the question of to default or not to default: In my application it made sence to have a set of properties fall back to string.empty for display reasons. Although people could argue that the view should then cover these possibilities and check for nulls and display nothing, it was a lot of bloat all over my pages to do a check on every property.

That's why I started implementing SafeXX properties. So say I had 'myObj.Name' that could possibly have a null value, there would also be a property 'myObj.SafeName' that caught the null in the getter and returned a string.empty in stead. The little naming convention gives away that it is not the regular getter.

Answer 2

In general, it is not a good idea to check for null values because the caller (the one who invokes the setter) may really want to set the value to null.

Suppose you query for 'someField' with this:

select * from ... where someField is null

If you set it as the empty string, the query above would fail.

Answer 3

If null really should be changed to "" for a valid reason (for example, it might mean "I don't care" and the default could be ""), go for it(but document it).

Otherwise, like if you just caught a NullPointerException and are trying to fix it this way, don't do it. If callers use obviously invalid values, the exception should be raised as soon as possible so that the caller notices the problem and fixes it before it bubbles up to a catastrophic, unexplainable error in a probably completely unrelated component.

Answer 4

Here's something to consider. Would you expect this unit test to pass or fail?:

yourClass.setSomeField(null);
assertNull(yourClass.getSomeField());

If you're changing the null value to an empty string and returning that in getSomeField, then the client now has to check two conditions when testing...a String and a null String. Not a big deal, but what happens if you've got twenty String properties in the class...you'd probably better try to be consistent amongst all of the setters, and if you're not then the reason should probably be more obvious than just the documentation saying so.

There are certain conventions around getters and setters; certain expectations. If I call a setter on an object, then I usually expect the getter to return what I set. I don't expect it to return some representation of what I passed in that is more convenient for the class to work with internally. I don't care about the internals of the class, and don't want to.

Answer 5

If you don't want a field set to null, then don't set it to null.

This can be a good idea if you have no control over the code doing the setting, but if you do, it better to fix the problem at the source rather than put in work arounds like this.

Answer 6

That is hard to answer. On the first look it seems to make the usage better because you don't have to check for null all the time. But you loose the quality of null that means nothing is assigned. If you do String.Empty you already have ambiguity if someone gave you a String.Empty as parameter. Maybe it doesn't matter.

I personally (if at all) wouldn't do it in the setter. Inside your class null should have a value of its own. If you are for convenience a getter

return (this.someField != null) ? this.someField: String.Empty;

will do. You would keep the null internally to deal with but the outside has a more convenient method.

Generally and personally speaking I wouldn't do it. It looks good at first and makes a lot of things harder at later time.