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

Asked 14/12, 2012 at 9:41 Answered 21/11, 2013 at 10:26

Solved class lazy-loading lazy-evaluation getter-setter

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

Inculpate answered 14/12, 2012 at 9:41 Comment(10)

imho it's ok if this side effect is clearly documented in the docs – Underlayer 14/12, 2012 at 9:44

I prefer that getter functions will never change the object's value. You can perform this check in other function and set the value of it in a setter function. Just to have a clear code. – Otolith 14/12, 2012 at 9:45

I don't think, this is a major problem. This is ok, but clear cut java doc must be provided before the method . – Childe 14/12, 2012 at 9:48

Getting a value should not have side-effects. – Doghouse 14/12, 2012 at 9:48

I think this is bad because it seems like you are mixing your presenting with your data model. What happens when your app need to be translated to a different language?. – Elisha 14/12, 2012 at 9:53

I'm not found of your change, but generally I think changing a getter is OK. After all, the getter is an abstraction that enables us to change the internal behavior w/o breaking other code. – Biologist 14/12, 2012 at 12:9

myvalue in the if is spelt with lower case 'v', twice. Is it intentional? – Adonis 14/12, 2012 at 17:55

Shouldn't the question title read, Is it bad practice for a getter to change an object's member's value?, as it is it sounds like you are asking whether it's bad for a getter's internal implementation to change, to which I believe the answer is no, it's ok to change it. – Fulsome 15/12, 2012 at 9:48

The setter should be the only thing that changes the property's value. This is to say the getter should call the setter to alter the value. Why? Because the setter may also do things to the internal state and bypassing the setter may leave the instance in a conflicted or invalid state. – Propylaeum 18/12, 2012 at 22:52

An example of where you get the prior value of a variable while also setting it to a new value is the test-and-set operation. This is typically done atomically for thread-safety and commonly operates on booleans. The x86 instruction set includes such an operation (BTS). Earliest I can remember personally is the VAX 11 BBSSI (Branch on Bit Set and Set Interlocked) instruction, from the late '70s. – Ting 16/5, 2018 at 14:32

121

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.

Hurdygurdy answered 14/12, 2012 at 9:51 Comment(8)

Just a note: I feel that getters can change the internal state of an object in the case of lazy-loading, where you specifically want to load data only when the getter gets called. – Judah 14/12, 2012 at 10:50

Fair enough, that is a valid use-case. But the situation described by OP could lead to horrible side-effects. – Hurdygurdy 14/12, 2012 at 12:12

There is nothing more annoying than dealing with a Property that changes state on the getter, and having the state change while debugging every time some component of the debugger access it. – Sideline 14/12, 2012 at 16:14

I don't think there's any issue with changing the internal state of an object in a getter. The internal state of an object is the class author's business. As a user of the class, you are not supposed to know (or care) what's happening internally, as long as the external interface is consistent. So whether he sets the value in the setter or, lazily, in the getter, this is completely valid. What's important is that the getter consistently returns N/A for null or empty values. – Paisa 14/12, 2012 at 16:29

Agree with @Laurent this is simply lazy initialization of the instance variable and is quite common practice. Not sure about checking against .isEmpty() though, or even using "N/A" as the default value (I'd probably prefer a Null Object or an exception, because the value/treatment for the default case often depends on the calling side, not just the called object) – Chive 15/12, 2012 at 15:1

@Laurent Yes of course the main concern should be that the external interface of a class is well written and consistent. But the question was about bad coding practice. I assure you I can write totally valid classes using the most horrid of coding practices. – Hurdygurdy 17/12, 2012 at 8:57

@fgysin, essentially the question is "is it ok to set a value in a getter" and the answer is yes, and it's frequently done for lazy initialization. I can't comment on his particular example since there's no context, but in general lazy initialization is not a "horrid coding practice". – Paisa 17/12, 2012 at 9:8

@Laurent I'm not saying that lazy initialization is a bad practice. Just wanted to state that the question about coding practices is somewhat independent from the question whether the final class/entity offers valid & consistent interfaces. – Hurdygurdy 17/12, 2012 at 9:57

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

Judah answered 14/12, 2012 at 9:50 Comment(2)

Why would you call your lazy loading method get...()? Calling it something like load...() seems more appropriate, doesn't it? – Immotile 19/12, 2012 at 14:39

Because the getter returns the value directly unless it has never been loaded before (hence the call to loadMyValue inside the getter). Calling the getter loadMyValue would imply (according to me) that it loads the value no matter what. – Judah 19/12, 2012 at 14:48

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

Livraison answered 14/12, 2012 at 14:53 Comment(1)

I actually prefer the accepted answer. It states that you could return the value "N/A" instead of changing the class variable first. – Livraison 12/7, 2016 at 9:40

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".

Toddy answered 14/12, 2012 at 10:4 Comment(2)

I disagree with never changing a value in a setter. The entire purpose of using getter/setters is to abstract internal and external implementations. Person.setHeightInCentimeters(int value), Person.setHeightInMeters(double value), and Person.setHeightInFeetAndInches(int feet, int inches) should all share a single internal representation, meaning that at least two of them will be storing something other than the input value(s). – Diseuse 14/12, 2012 at 14:0

ah. i'm sorry so much :( i type mismatch, never change something in getter. not setter. as you see in my code, i have change internal state of setter. i have edited. i'm sorry so much. – Toddy 14/12, 2012 at 15:25

I usually define a specific getter.

Never alter original getter:

 public String getMyValue(){
     return this.myValue
 }

And create a specific getter:

public String getMyValueFormatted(){

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

Pair answered 14/12, 2012 at 17:48 Comment(5)

How do you know which one to call when? – Joline 18/12, 2012 at 22:17

The intent of the second method is for the purpose of display to user. – Pair 19/12, 2012 at 12:14

So then the 1st one should not be a public? – Joline 19/12, 2012 at 21:36

No. Both are public. Uses first to get raw value. When needs formatted value, use the second one. Simple, you don't need complicate getter/setter methods. – Pair 20/12, 2012 at 11:45

It would be confusing for a new developer who is calling the API. 1st approach: Rename 1st public method as getMyValueOriginal and 2nd method as getMyValueFormatted. – Joline 21/12, 2012 at 20:59

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.

Aesculapian answered 14/12, 2012 at 9:48 Comment(0)

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.

Gobble answered 14/12, 2012 at 9:46 Comment(0)

I do feel this is a bad practice unless and until you explain the reason why it is so necessary for you modify the object inside the getter method instead of doing it inside the setter method.
Do you feel this cannot be done for some reason? Could you please elaborate?

Whallon answered 14/12, 2012 at 10:11 Comment(0)

Do what ever you like. After all getters and setters are just another public methods. You could use any other names.

But if you use frameworks like Spring, you are bound to use those standard names and you should never put your custom codes inside them.

Dominations answered 14/12, 2012 at 12:41 Comment(0)

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.

Otis answered 14/12, 2012 at 17:56 Comment(0)

A setter could modify as part of validation, but a getter should return the value and let the validation be done by the caller. If you do validate, then how should be documented.

Decalcomania answered 15/12, 2012 at 1:58 Comment(0)

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.

Immotile answered 14/12, 2012 at 16:14 Comment(0)

State changes in getters should be a hanging offence. It means that client code must be careful about the order in which it accesses getters and setters and to do this it must have knowledge of the implementation. You should be able to call the getters in any order and still get the same results. A related problem occurs when the setter modifies the incoming value depending on the current state of the object.

Peep answered 21/11, 2013 at 10:26 Comment(1)

Exactly! If you need some rope, please let me know. – Kamp 19/10, 2016 at 3:13

You can use some value holder for this purpose. Like Optional class in guava library.

Downgrade answered 14/12, 2012 at 9:45 Comment(0)

Hot tags

Godot Unity Godot Help Programming Godot 4.X GUI GDScript 3D 2D Physics CSharp Godot 3.X VR XR Projects C++

Recommended topics

Hot tags