Does designing a method that changes the argument's (if was object) values a good practice?

Question

The premise is using a language (e.g. C#, javascript) which passes an object by reference into a method.

Assume there is an object "Player" which has a property "Level" and there is a method takes Player as parameter and its function is to increase the player's level by 1.

The method can be designed something like (c# syntax):

void AddLevelBy1(Player player)
{
 player.Level +=1;
}

The concern was this method changed the value of the argument and I could not find a generic way to indicate (or may prevent?) this.

One might get some hints from the method name "AddLevelBy1". But such a way is not reliable and if the method was name "foo" then it becomes even more ambiguous.

So is there a solution / convention for this case (indicate whether arguments get changed)? Or such method design was considered a bad practice?

Added:

Try to describe my case more clearly:

1. I have an object "Player" which was not defined by myself (cannot change).

2. I wrote a method (AddLevelBy1() in this example) which takes "Player" as parameter (I know what happened).

3. I also pass this "Player" to some other methods (e.g. foo()) which were not written by me (so this is no way to know whether the "Player" got changed).

Thanks for some answers below but what if the codes were designed in the manner I described above.

I have added a demo of the case I described: http://jsfiddle.net/54sun/srhHt/9/

Answer 1

If you have method like that, then such method should actually be part of Player class. This then clearly indicates the method might change the state of the object.

class Player
{
    void AddLevelBy1()
    {
        this.level += 1;
    }
}

player.AddLevelBy1()

The problems start when you have more than 1 objects being changed, but there is no general solution to that.

If using c#, the "out" keyword can be used to indicate the change.

This is wrong. out keyword is to indicate something completely else. See more here: https://stackoverflow.com/questions/9875333/c-sharp-passing-reference-type-directly-vs-out-parameter

Answer 2

Although your example is simplistic, and may be modeled differently (like @Euphoric suggested, or by using extension methods if the class is not in your code-base), there are a lot of use-cases where it is legitimate for a method to change the state of their parameters.

For example, builder processes, your recursive algorithms, like DFS detecting cycles.

You can use the Law of Demeter as a rule of thumb of what should a method do and not do:

• You can play with yourself.
• You can play with your own toys (but you can’t take them apart),
• You can play with toys that were given to you.
• And you can play with toys you’ve made yourself.

Of course, when a method has side-effects, it should be declared in its name: IncrementPlayerLevel(Player player) plainly states that it is going to change the level of the player...

On the other side of the design considerations, sometimes it is critical that methods do not change the state. For example - implementing an Actor model for parallel computations. When considering this, you are better off designing your state objects as immutable, so you don't need to ask yourself whether any method has changed your objects without you being aware of it.

Answer 3

As mentioned in Uri's answer, ideally your types would be immutable. Mutability complicates programs. But mainstream languages have poor support for immutability, and we have a long history of mutation which goes all the way back to the assembly days, so there's no getting around dealing with mutability sometimes. Sometimes mutating an argument is unavoidable - for example, if the class is not under your control, and it provides no way of cloning (i.e. deep copying) its values. So, it may not be a best practice, but sometimes you can't get around it.

Some have suggested making the mutator a method of the class. Even if the class is under your control, this doesn't necessarily make sense - there's an infinite number of things you might want to do with your mutable class, and thus an infinite number of functions that may need to mutate it. Secondly, not everything is a method. If you have a commutative operation, there is no "recipient" for the function; either argument is equally valid. If you have a function that operates on arguments of two different types, why should the function belong to one class or the other? Sometimes this makes no sense. If you don't need access to the class's private members, it doesn't make sense to make the function a method.

Ok, so you need to mutate the argument. What can you do to mitigate the potential damage? As you pointed out, it's critical that the function name unambiguously informs you that that's going to happen. Secondly, you should avoid creating functions that are accessors and mutators at the same time. If a function's purpose is to return some information about an object, people are going to be surprised if it also mutates that object or any other.

Uri brings up the Law of Demeter, which is an excellent guideline, but be warned that it's relevant only to abstractions. The idea is that an abstraction should hide its implementation, so chains like foo.bar.baz() are usually a sign that you're not doing a very good job there. But not everything is an abstraction! Some types (e.g. tuples, records) are simply data containers, and accessing their members is not a violation of the law, even if the end result is still a dotted chain. (Of course, if your abstraction shouldn't be exposing that tuple in the first place, it's still a problem.)