Is mutating object-parameters in a method(in Java) a bad practice?

Question

I have a question about mutating method-paramaters(which are objects) in a method.

I read and heard multiple times that it is a bad practice to mutate a object in a method which was passed in as a paramater. As example:

public void modifyList(List<Object> list) {
    list.add(new Object());
}

Instead, the passed in Object should be copied, the mutation should be performed on the copied object and the copied object should be returned. As example:

public List<Object> getModifiedList(List<Object> list) {
    List copy = new List<Object();
    //Make a deep copy if it would be necessary
    for(Object o : list) {
        copy.add(o.clone());
    }
    //Modify the copy
    copy.add(new Object());
    //return the copy
    return copy;
}

I understand that the second method has less potential for side effects because it doesn't mutate the input parameter.

But is this really the way to go? The performance would suffer because a lot of deep copys have to be created. Also it would cost a lot of time implement Copy-Constructors and implement clone-methods for all classes. Also it would increase the LOC immense.

In practice I don't see this pattern(copy the method-parameter) often.

Could somebody with a lot of experience(working as a programmer/software developer for years) answer this?

Greetings Maverin

Gastrology

it's a bad practice only if your object method must be thread-safe – Rsfsr

Ideally, you should not modify an object passed as an argument, unless you make it very clear that is what is expected e.g. modifyList would be expected to modify the list. – Banks

The bad practice here is assuming you can call add on a List: it's an optional method, so there is no guarantee it will succeed without an exception (and there is no way of knowing this, either). – Fingered

@AndyTurner @throws UnsupportedOperationException - if list does not support the add operation :-) – Foxtail

@Foxtail and there is no way of knowing if it will throw that without trying it. – Fingered

@AndyTurner The contract of Collection says that add must throw an UOE if it is not a supported operation. – Foxtail

@Foxtail but you have no means of knowing whether a given instance of List supports add without calling add. All you can do is catch the exception, and hope that you've got failure atomicity. – Fingered

@AndyTurner Of course, I was only suggesting to add a javadoc entry to the modifyList method. It's then the caller's responsibility to deal with the problem. – Foxtail

Answer 1

Both methods are fine and could be the correct choice depending on your use case. Just make sure that you name them in a way that makes the intent clear and write some javadoc.

It's then up to the developer to decide whether having the original list mutated is ok or not, and if not, pass a copy or use a different method.

For example, this method from the JDK mutates an existing list, but its intent and documentation are very clear.

Foxtail

That the JDK does something in a certain way says nothing! Many archaic code and bad practices survive because of 'backwards compatibility'. The most infamous, I think, being Dimension which is a mutable object while ideally it should be immutable (and every method like setSize has to make defensive copies as a result). Of course this is just a rant because there are some legit cases of parameter-mutating methods, yours included. – Pyrope

Answer 2

It's up to your use-case, as you have already implied. Making your components immutable brings many advantages such as better encapsulation, thread safety, avoiding having invalid state etc.. Of course, in this way you implement a performance hit. But someone with great experience wrote a chapter about this, which I can only recommend:

Effective Java -

Item 50: Make defensive copies when needed.

There he recommends:

You must program defensively, with the assumption that clients of your class will do their best to destroy its invariants.

and also:

In summary, if a class has mutable components that it gets from or returns to its clients, the class must defensively copy these components. If the cost of copy would be prohibitive and the class trusts its clients not to modify the components inappropriately, then the defensive copy may be replaced by documentation outlining the client's responsibility not to modify the affected components.

Baines

I think you've got this the wrong way round: the method being called isn't the client. Your method is the one destroying the invariants (or not). – Fingered

Is this comment about the answer or the question? (I'm confused) It depends on the perspective to point out the client. Bloch had the example of a Period being implemented using two dates. Clients are the classes that use the Period objects. And if your Period class is not well designed, you end up having clients misusing your logic(because you allowed them to do so). – Baines

@EiriniGraonidou Breaking invariant means: you have allowed a client to modify the internal state of your class and bring it into an inconsistent state. In the op's example, the class' internal state is not exposed - the method could actually be static. – Foxtail

@assylias, now I understand what was meant. I guess I referred generally to immutability at the class-design level. On the other hand, I can see now, that there is a very small scope where one needs to do this locally-inside one method-. Please, let me know if you consider this answer misleading. – Baines