2

yesterday I spent some time trying to find a bug. Long story short, finally I realized that it was because of this constructor:

public Triangle(List<Vertex> vertices) {
    this._values = vertices;
}

I tried to initialize an object with a list of values and the object just took a reference to my object instead of getting the values from list. If I don't abandon the list that I passed as a parameter and use it later for something else like initializing something else with the same values or if I decide to clear it and fill with new values, I obviously destroy the state of my Triangle object without knowing it.

My first reaction was to "fix the bug" in the constructor but then I started thinking if it's really the way it should be. What's the good practice that covers things like that? In general, what should I think about constructors/init methods that take a list of values? Should they leave it intact? Am I allowed to reuse the list and whose fault is it when it leads to an error?

I mean, I obviously can do something like that: 

var triangle = new Triangle(new List<Vertex>(vertices));

but shouldn't it be done by the creators of the Triangle class already?

I would like to know some guidelines on that. Thanks.

Dyppl
  • 12,161
  • 9
  • 47
  • 68
  • @Timwi: okay, i fixed the error. I really think that this is the case when the actual code doesn't matter because I don't ask about this specific case but rather about the way things should be in general, and this is just an example – Dyppl Feb 18 '11 at 15:20

6 Answers6

4

Yes, the receiving class (Triangle) should make a copy, unless the design is to intentionally share the List.

Sharing can be useful but is the exception. I don't think a Triangle wants to share its List of vertices with something else.

Note that it could still be sharing the vertices (elements).

H H
  • 263,252
  • 30
  • 330
  • 514
2

Personally I agree with Henk; you should create a copy.

    /// <summary>
    /// Initialises a new instance of the <see cref="Triangle"/> class that 
    /// contains elements copied from the specified collection.
    /// </summary>
    /// <param name="vertices">
    /// The collection of vertices whose elements are to be copied.
    /// </param>
    public Triangle(IEnumerable<Vertex> vertices)
    {
        this.Vertices = new List<Vertex>(vertices);
    }

Whatever you choose just make sure you document it so consumers know what behaviour to expect.

Therefore consumers know that they can safely call new Triangle(vertices).

Siy Williams
  • 2,396
  • 1
  • 17
  • 26
2

C# is a pass-by-value language, but since a list is a reference type, it passes its reference by value. As you stated, this means you are passing a shared reference of the list to the constructor of your class. Modifications made anywhere in the code will affect the same list.

It depends on the desired behavior of your class as to what is the appropriate action. If you want to make a deep copy, the easiest way is to just allocate a new list in the constructor and pass in the IEnumerable reference to the list's constructor.

If you want to share the reference, it is a completely valid solution, just make sure you document your class (or name your class) appropriately.

Jordan Parmer
  • 36,042
  • 30
  • 97
  • 119
1

Passing a List object to the constructor would be considered poor design in this case. Perhaps a better solution would be to use a method

class Triangle
{
    List<Vertex> Vertices = new List<Vertex>(); // The triangle owns the vertex collection...

    public void SetVertices(IEnumerable<Vertex> vertices)
    {
        this.Vertices.Clear();

        this.Vertices.AddRange(vertices);
    }
}
MattDavey
  • 8,897
  • 3
  • 31
  • 54
  • Because it invites problems such as the one described by OP. The Triangle class should take ownership of the collection its using. Also, It does not require the added functionality of a List. In this case the bare minimum required functionality is that Triangle can enumerate over a collection of vertices, so IEnumerable is more cohesive, and not tightly coupled to any particular collection implementation. – MattDavey Feb 21 '11 at 13:18
1

I'd say that this is a documentation issue. The documentation, even if it's just the intellisense docs, should say whether the class is initialized using the values from the given list, or if it will use the given list directly. Given any mutable reference type, this is a valid question and should be documented.

Lacking proper documentation, I'd say it's up to you, the consumer of the class, to protect yourself against undocumented behaviors. You have two choices:

  1. Find out for yourself what documentation should have told you. You can use either Reflector or simple experimentation to determine what the code does with the mutable object you pass it.

  2. Protect yourself against the class's behavior, whatever it may be. If a class takes a mutable object, don't reuse that object. This way, even if the class's behavior changes later, you're secure.

In your specific case, I don't think that the Triangle class is wrong. It's constructor could have taken an IEnumerable<Vertex>1 and initialized a member List<Vertex> with those values, but instead, it's designer chose to take a List<Vertex> directly and use that. That could have been a performance-based decision.

1 To be complete, if a bit pedantic, I should mention that even if it took an IEnumerable<Vertex>, you could still run into this same issue. The class could still store and reuse a reference to this object, and therefore be sensitive to changes later made to the list. In this case, however, I would consider the Triangle class to be broken. Convention states, with few exceptions, that a method or constructor that takes an IEnumerable will use it once and then discard it.

P Daddy
  • 28,912
  • 9
  • 68
  • 92
0

What you need is a Clone or deep copy of the List.

Refer this answer for cloning a list

And this for more about deep copies, in general

Community
  • 1
  • 1
Robin Maben
  • 22,194
  • 16
  • 64
  • 99