Is it generally considered a bad practice to alter the object's state via its getter method?

Question

I've been thinking about this for a while during my past development.

I am kind of against doing such things, I've normally declared a separate method that explicitly handle the logic.

E.g.

If a Class has a method which return a list of objects, I think it is a bad idea to modify this collection directly via its getter method.

 Public Class ReportGenerator{

    private List<Configurations> configurations;

    List<Configuration> getConfigurations (){
      return this.configurations;
    }

    }

I think by doing the following is bad practice:

getConfigurations().remove(1); //remove a configuration via a getter

I would use the following appoach:

 Public Class ReportGenerator{

            private List<Configurations> configurations;

          public  List<Configuration> getConfigurations (){
              return Collections.unmodifiableList(this.configurations);
            }


          public void removeConfiguration(int index) //logic explicitly handle the remove
              { 
              this.configurations.remove(index);
            }

          }

However there is one more thing just comes to my mind is that what if Configuration object also has getter and setter, then you can't prevent others by doing

  reportGenerator.getConfigurations().get(0).settTitle("test");

They can still change the state of Configuration object by using the getter method of ReportGenerator, which I think it shouldn't allow that. But if we want to prevent this we would have to declare more methods in the ReportGenerator class to forward the call to the Configuration object, and use defensive copy.

So my question is that if you have a object A which contains another object B using composition and they both have getter and setter, do you prefer to change object B's state via A's getter method? E.g.

A.getB().setTitle("");

or do you prefer to add method to A to change B's state (basically forwarding call to B)

E.g.

A.setBTitle("");

and internally in this method, A call B.setTitle("");

Sorry about the long question, I think I am not really sure what exactly I want to ask. Hope you can understand. :P

Answer 1

That may work for smaller classes, but I would shudder at how you would have to main a more complex object hierarchy. Imagine if A suddenly had several objects as fields. Do you think it would be a good idea to right a wrapper around each of them? And then think of doing so as each object becomes more complex. Sometimes doing A.getB().getC().setThingamabob() is easier than writing A.setSomething() which calls B.setCField() which calls C.setThingamabob().

If you think an object should not be mutable, remove the setters. If a consumer of A does not need to change B at all, do not provide the getB() method and instead provide something similar to A.setThingy(), where it does not mention that whatever is being set is part of B.

Your API contract does not necessarily mean that your classes HAVE to provide getters/setters for all of its properties. If the properties are not meant to be read/modified directly by consumers, do not provide ways of doing so.

Answer 2

When you write code its about clearity more than anything. You can imo do what you need, even change the instances state however I would refrain fromb doing so if you can. Unless you have a good case fot it i.e. using lazy initialization (creating an object or value the first time you access it).

At the end it really depends but I would try not to change the object state on getter.

Looks like other people also think this way

.. When is it ok to change object state (for instance initialization) on property getter access?

Answer 3

The answer to whether it is bad practice is : "It depends".

On the one hand, a class that returns references to its internal state is a "leaky abstraction". This is bad for a number of theoretical reasons:

• Revealing the classes implementation details in the API encourages dependencies on those details and that makes it harder to change them. (This isn't categorically a problem here, but if you design the API so that modifying something returned by a getter is the standard way to change things, then this could be difficult to deal with ...)

• If the details are revealed as an immutable data structure, the client can still see "live" changes, and this can be a problem if the application is multi-threaded.

• If the details are revealed as a mutable data structure, the client could change the state of the class directly, bypassing any hypothetical checks, etcetera that the class normally performs.

• If the class is "security sensitive" in some contexts, you've got all sorts of extra "vectors" for breaking security if the abstraction is leaky.

On the other hand, making an abstraction non-leaky can be expensive:

• Unmodifiable collection wrappers have to be allocated, and they add an extra level of method calling.

• Returning a copy incurs the additional overhead of copying the data. And in a multi-threaded environment, you may need to lock the source data structure while you make the copy.

Finally, if the API needs to allow the client to make changes, doing it via a leaky abstraction might actually give you a simpler API design. The problem of leakiness can be mitigated by (say) making the relevant methods "package private".

So to sum up, it is a trade-off between "good OO design" and pragmatic issues. You need to weigh these up on a case-by-case basis.

---

By the way, your question title and description are misleading. For example

  getConfigurations().remove(1); //remove a configuration via a getter

This is misleading. You are not removing a configuration "via a getter". You are removing it via the object returned by the getter!!

(I'm nitpicking, but if you want people to understand you, you need to use language that they understand.)