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.

You could create a property for this field `public SomeObject SomeObject {get; private set;}`, but yeah - having another private setter method is also valid as it keeps your variable completely private. — Vidmantas Blazevicius, Mar 18 '18 at 18:17
I don't know your full code, so i can't say if there is a "code smell" or not, but to be honest i wouldn't create an additional method just because the tool gives you a warning, if you have to assign the field from these methods then just do it. — R1PFake, Mar 18 '18 at 18:18
@CamiloTerevinto Yes, it is one of the parts that this class is composed of. — Vahid, Mar 18 '18 at 18:19
@R1PFake I will never do that either, I am just curious how these kinds of things are refactored. — Vahid, Mar 18 '18 at 18:20
Then please post a more concrete example so we can actually see what's happening — Camilo Terevinto, Mar 18 '18 at 18:20

Dour High Arch · Accepted Answer · 2018-04-02T15:56:18.193

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.

Thanks for your answer. I changed the example in my question to better convey what I want. As you can see, there are cases that returning a new instance every time I use Move() or Rotate() command will not be a good idea always. — Vahid, Mar 18 '18 at 18:30
This is a great suggestion, when possible favor immutable classes I wrote a complete article on this topic in the past http://codebetter.com/patricksmacchia/2008/01/13/immutable-types-understand-them-and-use-them/ — Patrick from NDepend team, Mar 19 '18 at 08:10

Don't assign a field from many methods

1 Answers1