Is it bad practice to have my getter method change the stored value?

Solution 1:

I think it is actually quite a bad practice if your getter methods change the internal state of the object.

To achieve the same I would suggest just returning the "N/A".

• Generally speaking this internal field might be used in other places (internally) for which you don't need to use the getter method. So in the end, the call to foo.getMyValue() could actually change the behaviour of foo.

Alternatively, the translation from null to "N/A" could be done in the setter, i.e. the internal value could be set to "N/A" if null is passed.

A general remark:
I would only add states such as "N/A" if they are expected by some API or other instance relying on your code. If that is not the case you should rely on the standard null types that are available to you in your programming language.

Solution 2:

In my opinion, unless you are doing lazy-loading (which you are not in that case), getters should not change the value. So I would either:

Put the change in the setter

public void setMyValue(String value) {
    if(value == null || value.isEmpty()){
        this.myValue = "N/A";
    } else {
        this.myValue = value;
    }
}

Or make the getter return a default value if value not set properly:

public String getMyValue() {
    if(this.myvalue == null || this.myvalue.isEmpty()){
        return "N/A";
    }    
    return this.myValue;
}

In the case of lazy-loading, where I would say that changing your members in a getter is fine, you would do something like:

public String getMyValue() {
    if (this.myvalue == null) {
        this.myvalue = loadMyValue();
    }    
    return this.myValue;
}

Solution 3:

No. You're doing two things here. Getting and setting.

Solution 4:

Yes. It's a bad practice.

Why?

When the value is set (in a constructor or setter method), it should be validated, not when a getter method is called. Creating a private validate* method for this is also a good idea.

private boolean validateThisValue(String a) {
     return this.myValue != null && !this.myValue.isEmpty();
}

public void setThisValue(String a) {
    if (validateThisValue(a)) {
        this.myValue = a;
    } 
    else {
        // do something else
        // in this example will be
        this.myValue = "N/A";
    }
}

And, in the getter method, never ever change the state of the object. I have worked on some projects, and the getter often must be made const: "this method cannot change internal state".

At least, if you do not want to complicate things, in the getter method, you should return "N/A" rather than change internal state and set myValue to "N/A".