When you have a method that is doing stuff and changes occur in the parameters, should you always return the changed object like this:

public SomeModel DoSomething(SomeModel someModel) {
[something made changes to the model in here]
return someModel;
}

This way when someone reads the occurring line when the method was used, they see return object equals parameter object, so they know there were changes to this object. On the other hand when there are multiple parameters and they all had changes, you could not use this technique.

Just return nothing:

public void DoSomething(SomeModel someModel) {
[something made changes to the model in here]
}

This way you would not know if the SomeModel parameter has changed. This could be (maybe) indicated by the method name however.

Arguments can be passed to methods for different purposes. It could be input data, an interface to be used for reporting progress or logging, a "bucket" to be filled up with results, just to pass it on further down the call stack... I probably forgot some applications. The answer to your question depends on the usage scenario.
– Martin MaatDec 6 at 19:33

3 Answers
3

I disagree. There are far too many ways in which this can fail to use it to convey information. If anything, the method name should make it clear whether it mutates the input, rather than rely on shady clues from the return type.

The name implies the action - it just makes a copy (although fails to say if it's deep or shallow, if they differ in this case). It would be rather bad if you thought that this mutates the model and returns it, just on the basis of being used to your schema.

public Collection<Model> readInput(Input in)

This method has a proper signature and it's obvious what it's doing. There's a good chance it will mutate in and will not be able to restore it to its original state (nor should it, really). If you're used to your method, you will totally misread what it's doing.

A possible "fix" would be to pass in a collection to append new models to, but then you have 2 mutated parameters.

Another "fix" is to have a local instance collection to use, but there's a myriad of reasons why that's a bad idea. I'll focus on 1 of them in particular: A fully read input stream is useless to return. There's literally nothing you can do with it, except close it. Most of the time, your output will be simply ignored.

I generally prefer void methods when dealing with methods whose main goal is to mutate their parameters or local state. A void method that does not mutate anything might as well be dead code (with the single exception of assertion methods that throw). As such, if you see a void method, you know that something will be mutated.

There is one good reason to return your mutated argument - that is making your API friendly to chaining. When you have reason to believe that the user will invoke multiple methods in a row, and there is nothing better to return, return the mutated input.

But whether you're chaining, copying, reading or consuming, the main idea is that the method signature should give you the relevant information on what it does and what side-effects it has. That will likely involve some amount of convention, but that's to be expected, really. The output will give some extra idea of how to use the result of the method, but not about what it does.

then, unless I have access to the source and read it to find out otherwise, I'd generally assume that it returns a new SomeModel, having left the original unchanged. If you modify it, the principle of least astonishment is violated:

If your method has to modify model, then make it a void method and make it clear from the name that it modifies the parameter. But I'd argue that, if possible, it's better to create a new copy from the parameter, modify that and return that copy, leaving the parameter unchanged.

It is in fact bad practice to change objects in a method that does not belong to that object, thus you should avoid it.
You could try and move the code that changes the object into the SomeModel class. This is much more intuitive for developers.

If for some reason, you don't want the code inside the SomeModel class, then I'd suggest writing an extension method. That way the consumer of the SomeModel class still can write the same code.

For either solution, you should still make it very clear in your method name that the object is changed.

How would that work? Lets say I have an account and an invoice. The invoice is mutated to "paid" while the account is mutated to "less money". How could that not happen in a method that either not belongs to invoice and/or not belongs to account?
– nvoigtDec 7 at 15:05

1

@nvoigt: Well, that brings us to another best practice. Single Responsibility: One method should not mutate an invoice to paid and then subtract money from an account. These are two different actions. I agree that they should be bundled together. In order to do that, a Transaction would be a good idea.
– Jonathan van de VeenDec 10 at 8:00