Using mutable objects as constructor arguments

Question

What is the best practice to pass objects as constructor arguments? Passing mutable objects can lead to unexpected results.

A simple example. We expect 200, but get 10000 calling the TestMethod():

public class Test
{
    public int TestMethod()
    {
        var variable1 = new SomeMeasurements
        {
            Width = 10,
            Height = 20
        };
        var obj1 = new MyRectangle(variable1);

        // <... more code...>

        variable1.Height = 1000; // a local variable was reused here, and it's field was changed

        // <... more code...>

        return obj1.GetArea();
    }
}

public class SomeMeasurements
{
    public int Width { get; set; }
    public int Height { get; set; }
}


public class MyRectangle
{
    SomeMeasurements _arg;

    public MyRectangle(SomeMeasurements arg)
    {
        _arg = arg;
    }

    public int GetArea()
    {
        return _arg.Width * _arg.Height;
    }
}

In this case the error is obvious, but with more complex classes debugging can be tedious. Several things how to fix this have crossed my mind:

option 1. Fix TestMethod() - it mustn't change variable1 after creating MyRectangle.

option 2. Fix class SomeMeasurements - turn it into a struct:

public struct SomeMeasurements
{
    public int Width { get; set; }
    public int Height { get; set; }
}

option 3. Fix class SomeMeasurements - make it immutable:

public class SomeMeasurements
{
    public SomeMeasurements(int width, int height)
    {
        Width = width;
        Height = height;
    }

    public int Width { get; }
    public int Height { get; }
}

option 4. Fix class MyRectangle body - it mustn't use mutable objects:

public class MyRectangle
{
    int _height;
    int _width;

    public MyRectangle(SomeMeasurements arg)
    {
        _height = arg.Height;
        _width = arg.Width;
    }

    public int GetArea()
    {
        return _width * _height;
    }
}

option 5. Make SomeMeasurements ICloneable and use it's Clone() in MyRectangle constructor.

Any of these options have it's flaws - it might be hard to avoid reusing variable1, MyRectangle can be more complex to turn it into a struct, MyRectangle can be external and you might not change it at all, etc. What is the most correct way to fix this?

See also http://stackoverflow.com/questions/4327108/what-is-the-meaning-of-data-hiding — Tim Barrass, Jan 21 '16 at 09:42

Tim Barrass · Answer 1 · 2016-01-21T09:45:59.577

2

Generally you should be passing services that conform to a certain interface, or immutable objects only in constructors. The constructor should take a copy of any mutable data passed to it if you want to protect it from external changes.

Once the data goes through the constructor it should be considered part of the new instance's state, and shouldn't be available for modification outside of that instance.

Your options 3,4 seem most useful. Option 2 would fix the problem because you pass a copy of the data into the constructor. Option 1 may be out of your control in many contexts.

edited Jan 21 '16 at 09:45

answered Jan 21 '16 at 09:37

Tim Barrass

4,813
2
29
55

I think you'd need to post the code for GetArea() to answer that – Tim Barrass Jan 21 '16 at 09:40
I didn't change MyRectangle at all, I just changed "class" to "struct" in SomeMeasurements – enkryptor Jan 21 '16 at 09:43

score 1 · Accepted Answer · answered Jan 21 '16 at 10:27

It depends on the relationship between the classes, and what they are designed to do.

If you consider a StreamReader class constructed from a Stream instance, that Stream is expected to continue to be "it's own" mutable class with its own set of responsibilities while the reader deals with its mutability in a given way. There is an ongoing relationship between the two objects, and if one does something to the Stream here, one expects it to affect the reader.

In that case we obviously just hold a reference to the Stream passed to the constructor.

In other cases an object passed to represent the initial state of the object being created. There isn't an ongoing relationship between the two.

Here it's best to copy either the object passed or its fields. (When it comes to micro-opts, copying the fields makes the initial construction very slightly slower and the uses of them very slightly faster).

Which case you are dealing with is something that is part of what you are designing, in that you can decide to make a class work either way. Some cases clearly have to be one or the other (in the StreamReader example it would make no sense to ever not hold on to the Stream you were dealing with), but often there is a choice. Favour the principle of least surprise, and if you still can't make up your mind favour the copying approach where there is no ongoing relationship between the objects as your dependencies are now simpler.

Using mutable objects as constructor arguments

2 Answers2