Don't assign a field from many methods

Question

I have a class that has a field that is being assigned a value from multiple methods.

public class Shape
{
    private Point2D m_location;

    public void Move()
    {
       m_location = ...
    }

    public void Rotate()
    {
       m_location = ...
    }

    public void Flip()
    {
       m_location = ...
    }
}

I am getting a warning from NDepend that says:

Don't assign a field from many methods

https://www.ndepend.com/default-rules/Q_Don't_assign_a_field_from_many_methods.html

I am thinking of solving this problem by creating a separate method to assign the value of the field and calling this method from the other methods that currently assign a value to the field.

Here is an example of the code:

private void SetLocation(Point2D point)
{
    m_location = location;
}

I want to know if this is a valid way to solve the problem and if it will just hide the code-smell that NDepend detected or actually fix the issue.

Answer 1

Is this a valid way to solve this problem?

No. As you suspect, this is a code smell. What NDepend is complaining about is mutable references; you have code where:

var s = new SomeObject(someInitialization);
var r = s.SomeResult();
// you now have no idea what s contains or if it is even usable any more.

The solution to this is to make SomeObject immutable and return new references instead of changing internals:

public SomeObject Something()
{
    return new SomeObject(SomethingDifferentDependingOn(this.something));
}

Now instead of your first example you have:

var s = new SomeObject(someInitialization);
var r = s.Something().Result;
// s is guaranteed to be unchanged.

Yes some times you will need mutable references. In those cases; document them and explain why they have to be mutable. Then you can override NDepend rules on a case-by-case basis to prevent it showing a warning. If you have a code smell, warn people. Do not try to hide it.

The example after your edit is quite different, but the general principle still holds. If you have only a few internal fields that all change in method calls you can still return immutable references, e.g.:

public Shape Move()
{
    return new Shape(m_location ...);
}

If you have many internal fields that don't all change, or you need to do something like share private fields you can't easily have immutable reference, but you can still avoid the warning by using accessors:

public Location
{
    get { return m_location; }
    private set { m_location = value; }
}

Then use Shape.Location exclusively in your internal methods.