Is it bad practice to have my getter method change

Question

Is it bad practice to change my getter method like version 2 in my class.

Version 1:

 public String getMyValue(){
     return this.myValue
 }

Version 2:

 public String getMyValue(){

    if(this.myValue == null || this.myValue.isEmpty()){
       this.myValue = "N/A";
    }

    return this.myValue;
 }

Answer 1

I think it's better to initialize this.myValue = "N/A". And subsequent calls to setMyValue should modify the this.myValue according to your business conditions.
The getMyValue shouldn't modify in any way this.myValue. If your needs are to return a certain value, you should return that value (like "N/A") and not alter this.myValue . Getters must not modify member's value.

Answer 2

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.

Answer 3

This actually highly depends on the contract you want to enforce with your get()-method. According to design-by-contract conventions the caller has to make sure that the preconditions are met (which means doing a validation in a setter method often is actually bad design) and the callee (I do not know if that's the correct english term for that, i.e., the called one) makes sure that the post conditions are met.

If you define your contract so that the get()-method is not allowed to change the object then you are breaking your own contract. Think about implementing a method like

public isValid() {
    return (this.myvalue == null || this.myvalue.isEmpty());
}

Advantage of this approach is that you do not have to check wether the return of your get() is "N/A" or something else. This also can be called before calling set() to validate that you do not insert illegal values into your object.

If you want to set a default value you should do that during initialization.

Answer 4

I would change better the setter method so, if the value is null or empty, the N/A is assigned to the attribute. So, if you use the attribute in other methods inside the class (v.g. toString()) you will have the intended value there.

Alternatively, change the setter method to launch an exception when the value being set is not right, so the programmer is forced to improve its handling prior to setting the value.

Other than that, it is ok.

Answer 5

absolutely yes, it's a bad pratice.

Imagine you communicate accross network with a third party (remoting, COM, ...), this will increase the round-trip and then hit application performance.

Answer 6

Yes. it's a bad practice.

Why ?

When the value is imported into object (in constructor, or setter method), those value will be validated, not validate until getter method has been called. And, if someone is careful, they will create an entire private validate method for this value too.

private boolean validateThisValue(String a) {
   // return true or false
   // in this example will be
   if(this.myvalue == null || this.myvalue.isEmpty()){
       return false;
   }
   else {
       return true;
   }
}

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 getter method, never ever change the state of value. I have worked on some project, and the getter often must be set to const : this method cannot change internal state.

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

Hope this help :)