about returning a reference to something

[Tony Johansson]

Hello!

Assume I have a class where I have some fields which could be of type
string[], List<T> and member of
a single Car object for example. To all these is the class holding a
reference.

Now to my question if some client class need to a reference to all these for
example should I then always
make a copy of the referenced object and then return the reference to the
copy.
Like I do here.
public List<FoodItem> FoodList
{
get
{
return new List<FoodItem>(m_foodItemList);
}
}

If I do that I will of course loose performance.
So what is your opinion in this matter ?

//Tony

 

[Arne Vajhøj]

Assume I have a class where I have some fields which could be of type
string[], List<T> and member of
a single Car object for example. To all these is the class holding a
reference.

Now to my question if some client class need to a reference to all these for
example should I then always
make a copy of the referenced object and then return the reference to the
copy.
Like I do here.
public List<FoodItem> FoodList
{
get
{
return new List<FoodItem>(m_foodItemList);
}
}

If I do that I will of course loose performance.
So what is your opinion in this matter ?

I would still recommend:

public List<FoodItem> FoodList
{
get
{
return m_foodItemList.AsReadOnly();
}
}

because that one does not copy the data - it only
wraps access to the data.

This means that a client that only needs to read data
does not copy. A client that needs to modify data can
then create a copy of data and do whatever.

But I think that is better than allways creating
a copy just in case someone want to modify a copy.

Arne

 

[Tony Johansson]

Assume I have a class where I have some fields which could be of type
string[], List<T> and member of
a single Car object for example. To all these is the class holding a
reference.

Now to my question if some client class need to a reference to all these
for
example should I then always
make a copy of the referenced object and then return the reference to the
copy.
Like I do here.
public List<FoodItem> FoodList
{
get
{
return new List<FoodItem>(m_foodItemList);
}
}

If I do that I will of course loose performance.
So what is your opinion in this matter ?

I would still recommend:

public List<FoodItem> FoodList
{
get
{
return m_foodItemList.AsReadOnly();
}
}

because that one does not copy the data - it only
wraps access to the data.

This means that a client that only needs to read data
does not copy. A client that needs to modify data can
then create a copy of data and do whatever.

But I think that is better than allways creating
a copy just in case someone want to modify a copy.

Arne

This cause problem look at the two classes I have here and at the end I have
a method called InitFoodItems that I use from the GUI class. This method
InitFoodItems I use to add some items to a checkedListBox.
The first problem is that I have a set and a get for the Ingredients
property.
The second problem is that I use InitFoodItems method in a way that a
string[] is required as you can see here
checkedListBox.Items.Add(string.Format(foodItem.Name + " (" +
string.Join(",", foodItem.Ingredients) + ")"));


***********************************
Class FoodManager
**********************************
class FoodManager
{
private List<FoodItem> m_foodItems = new List<FoodItem>();
public FoodManager()
{}

public int Count
{
get { return m_foodItems.Count; }
}

public void Add(FoodItem foodItem)
{
m_foodItems.Add(foodItem);
}

public List<FoodItem> GetFoodItemList()
{
return new List<FoodItem>(m_foodItems);
}

public void Reset()
{
m_foodItems.Clear();
}

public FoodItem this[string name]
{
get
{
return (FoodItem)m_foodItems.Find(delegate(FoodItem item) {
return item.Name == name; }).GetCopy();
}
}
}

***********************************
Class FoodItem
**********************************
public enum FoodItemNames
{
MixedFood, Vegeterian, Meat
}

class FoodItem : IAnimalFood
{
private string name;
private string[] ingredients;
private string[] copyOfingredients;

public FoodItem(string name, params string[] ingredients)
{
this.name = name;
this.ingredients = ingredients;
}

#region IAnimalFood Members

public string Name
{
get { return name; }
set { name = value; }
}

public string[] Ingredients
{
get
{
copyOfingredients = new string[ingredients.Length];
Array.Copy(ingredients, copyOfingredients, ingredients.Length);
return copyOfingredients;
}
set { ingredients = value; }
}

public bool IsGoodFor(EaterType eaterType)
{
bool status = false;

switch (eaterType)
{
case EaterType.Carnivora:
if (name.ToLower() ==
FoodItemNames.Meat.ToString().ToLower())
status = true;
else
status = false;

break;
case EaterType.Herbivore:
if (name.ToLower() ==
FoodItemNames.Vegeterian.ToString().ToLower())
status = true;
else
status = false;

break;
case EaterType.Omnivorous:
if (name.ToLower() ==
FoodItemNames.MixedFood.ToString().ToLower())
status = true;
else
status = false;

break;
}
return status;
}
#endregion

#region ICloneable Members

public object GetCopy()
{
return MemberwiseClone();
}

#endregion
}


private void InitFoodItems()
{
foodManager.Add(new FoodItem(FoodItemNames.MixedFood.ToString(),
"carrots", "beef", "milk"));
foodManager.Add(new FoodItem(FoodItemNames.Vegeterian.ToString(),
"cauliflower", "celery", "broccoli","fruits", "leaves", "roots","seeds"));
foodManager.Add(new FoodItem(FoodItemNames.Meat.ToString(),
"meat"));

foreach (FoodItem foodItem in foodManager.GetFoodItemList())
{
checkedListBox.Items.Add(string.Format(foodItem.Name + " (" +
string.Join(",", foodItem.Ingredients) + ")"));
}
}

//Tony

 

[Arne Vajhøj]

Assume I have a class where I have some fields which could be of type
string[], List<T> and member of
a single Car object for example. To all these is the class holding a
reference.

Now to my question if some client class need to a reference to all these
for
example should I then always
make a copy of the referenced object and then return the reference to the
copy.
Like I do here.
public List<FoodItem> FoodList
{
get
{
return new List<FoodItem>(m_foodItemList);
}
}

If I do that I will of course loose performance.
So what is your opinion in this matter ?

I would still recommend:

public List<FoodItem> FoodList
{
get
{
return m_foodItemList.AsReadOnly();
}
}

because that one does not copy the data - it only
wraps access to the data.

This means that a client that only needs to read data
does not copy. A client that needs to modify data can
then create a copy of data and do whatever.

But I think that is better than allways creating
a copy just in case someone want to modify a copy.

This cause problem look at the two classes I have here and at the end I have
a method called InitFoodItems that I use from the GUI class. This method
InitFoodItems I use to add some items to a checkedListBox.
The first problem is that I have a set and a get for the Ingredients
property.
The second problem is that I use InitFoodItems method in a way that a
string[] is required as you can see here
checkedListBox.Items.Add(string.Format(foodItem.Name + " (" +
string.Join(",", foodItem.Ingredients) + ")"));

I don't understand what this has to to with the
FoodList property in FoodManager.

But there some thing with your code that puzzeles me.

class FoodItem : IAnimalFood
{
private string name;
private string[] ingredients;
private string[] copyOfingredients;

public FoodItem(string name, params string[] ingredients)

Why not use the enum instead of a string as name??

{
this.name = name;
this.ingredients = ingredients;
}

#region IAnimalFood Members

public string Name
{
get { return name; }
set { name = value; }
}

public string[] Ingredients
{
get
{
copyOfingredients = new string[ingredients.Length];
Array.Copy(ingredients, copyOfingredients, ingredients.Length);
return copyOfingredients;
}
set { ingredients = value; }
}

This exposes a lot of data to the external world.

And I do not like the copy data on get and just take ref in set.

public bool IsGoodFor(EaterType eaterType)
{
bool status = false;

switch (eaterType)
{
case EaterType.Carnivora:
if (name.ToLower() ==
FoodItemNames.Meat.ToString().ToLower())

First: there is a String method for comparing case insensitive, which
you should use instead of ToLower==ToLower.

Second: if you used enum instead of string everything would be easier.

status = true;
else
status = false;

break;
case EaterType.Herbivore:
if (name.ToLower() ==
FoodItemNames.Vegeterian.ToString().ToLower())
status = true;
else
status = false;

break;
case EaterType.Omnivorous:
if (name.ToLower() ==
FoodItemNames.MixedFood.ToString().ToLower())
status = true;
else
status = false;

break;
}
return status;
}
#endregion

#region ICloneable Members

public object GetCopy()
{
return MemberwiseClone();
}

#endregion
}


private void InitFoodItems()
{
foodManager.Add(new FoodItem(FoodItemNames.MixedFood.ToString(),
"carrots", "beef", "milk"));
foodManager.Add(new FoodItem(FoodItemNames.Vegeterian.ToString(),
"cauliflower", "celery", "broccoli","fruits", "leaves", "roots","seeds"));
foodManager.Add(new FoodItem(FoodItemNames.Meat.ToString(),
"meat"));

This where you should use enum.

foreach (FoodItem foodItem in foodManager.GetFoodItemList())
{
checkedListBox.Items.Add(string.Format(foodItem.Name + " (" +
string.Join(",", foodItem.Ingredients) + ")"));

I definitely don't like this.

You should use Items.AddRange and DisplayMember!

See below.

}
}

Example with CheckedListBox, List and DisplayMember:

public class Foobar
{
public string A { get; set; }
public string B { get; set; }
public string AB { get { return A + "#" + B; } } // <----
}

....

List<Foobar> lst = new List<Foobar>();
lst.Add(new Foobar{ A="A1", B="B1" });
lst.Add(new Foobar{ A="A2", B="B2" });
checkedListBox1.Items.AddRange(lst.ToArray());
checkedListBox1.DisplayMember = "AB"; // <----

Arne

 

[Arne Vajhøj]

[...]
But I think that is better than allways creating
a copy just in case someone want to modify a copy.

I agree that returning the ReadOnlyCollection<T> (either as that type or
IList<T>) is a relatively efficient approach when feasible. However,
it's important to note that the underlying data structure _is_ still
there, and _can_ still be modified by its owner.

This can cause trouble in a variety of ways. Multi-threaded code is one
obvious scenario. But you'd also need to be careful to avoid modifying
the underlying collection while the read-only wrapper is being
enumerated (i.e. if the iterating code for whatever reason wound up
calling something that could modify the collection).

A true copy of the data structure avoids these issues. Many other
alternatives (such as those I mentioned) will have similar problems that
a read-only wrapper would.

It is a pretty fundamental characteristic of C# that when you
have a ref, then potentially something can change what the
ref is pointing to.

It should be the callers responsibility to clone if the
caller consider it necessary.

You can't clone in all cases and it inconsistent just to clone
in the case of returning a collection property.

Arne

 

[Arne Vajhøj]

[...]

A true copy of the data structure avoids these issues. Many other
alternatives (such as those I mentioned) will have similar problems that
a read-only wrapper would.

It is a pretty fundamental characteristic of C# that when you
have a ref, then potentially something can change what the
ref is pointing to.

That's not the point. The issue here is that the thing returned is being
exposed as a read-only collection. But it's only read-only to the code
with the reference to that actual object. The reference to the mutable
collection is implicit, and not actually known by the code receiving the
read-only collection.

And?

In general code should not assume that just because it can not
modify something then nobody can modify it. That is basic C# and
even basic OOP.

And if they actually read the documentation for AsReadOnly and
ReadOnlyCollection then they would know that it is not the case.

I don't think it is good to clone just to prevent people
not understanding OOP and not reading the docs from
being surprised.

Sometimes yes, sometimes no. It just depends on the API definition. My
point is that the library types themselves do not help enforce these
kinds of issues. It's important to be aware of them and make sure the
code adheres to the contracts, implied and explicit.

You can of course always argue that as long as the semantics
is documented, then it is acceptable.

But I would consider it better to have the code that knows
it need a copy do the clone instead of having some other
code do the clone for both those that need a copy and
those that do not.

I have no idea what you mean.

You can not clone everytime a ref is handed out to something
that may be modified.

And doing it for collections or readonly collections but not
in other cases is inconsistent.

As far as inconsistencies go, so what? It's great to follow standard
conventions, but the fact is that different pieces of code will have
different design and implementation needs. Not all code that uses
collections will be implemented the same way as all other code that uses
collections.

Consistent behavior makes it a lot easier to work with
the API, because one can learn just the rules instead
of the individual cases.

Arne

 

[Arne Vajhøj]

[...]
I don't think it is good to clone just to prevent people
not understanding OOP and not reading the docs from
being surprised.

You are welcome to your opinion. But it's just that: an opinion.

Others, including myself, hold the opinion that the fewer potential
surprises that exist in an API, even if the potential surprises are
documented, the better the API.

Actually I agree with that.

Unless which is the case here that it make people that
understand OOP and read the docs surprised.

That's exactly what you are arguing. See your quoted statement above.

Actually it is not.

I don't understand how you get from not caring for people
that does not read the framework docs to that you can construct
your API as you want as long that it is documented.

Yes, you've made that clear.


Only in the same way that having a type that is specifically designed to
contain multiple instances of other types is also inconsistent.

It is not a problem at all for collection types to be treated
differently from other types in certain situations. And there is no
reason to hold to a foolish consistency, just for the sake of being
consistent.

Consistency is in itself a benefit.

Remember the point from above about avoiding surprises.

Consistency is a key element to achieve that.

Collection types are consistently different from other types in a number
of ways. Being different is not the same as being inconsistent, when one
takes a broader view.

Collection are different from other types in some regards, because
that is given by their purpose.

It is not given by their purpose that properties returning
collections return copies while properties returning
other types return ref to the original.

Arne

 

[Arne Vajhøj]

[...]
I don't understand how you get from not caring for people
that does not read the framework docs to that you can construct
your API as you want as long that it is documented.

Since I never wrote that,

Based on where I wrote about nor caring about people
not reading what List AsReadOnly you concluded that I was
arguing that that as long as the semantics is documented, then
it is acceptable.

That seems to be the same.

[...]
It is not given by their purpose that properties returning
collections return copies while properties returning
other types return ref to the original.

Actually, properties of most classes always return copies, primarily by
virtue of those properties having value types.

Some classes return reference types, and in many of those cases the
reference types are even specifically intended to be modified. But it's
hardly a uniform behavior across all classes. This is not an area where
there's any truly consistent examples to be consistent with in the first
place.

I would be surprise if the number of ref types being returned
as a copy by a property is over 1% of total ref types being
returned by a property.

Arne

 

[Arne Vajhøj]

On 2/2/11 10:58 AM, Arne Vajhøj wrote:
[...]
I don't understand how you get from not caring for people
that does not read the framework docs to that you can construct
your API as you want as long that it is documented.

Since I never wrote that,

Based on where I wrote about nor caring about people
not reading what List AsReadOnly you concluded that I was
arguing that that as long as the semantics is documented, then
it is acceptable.

I have no idea what that paragraph is even supposed to mean. It's not
comprehensible as a normal English sentence, and I am unable to decipher
it.

There is a typo - "nor" instead of "not". And there is missing a "does"
after List AsReadOnly.

For better or worse, your level of surprise is not a useful metric here.

We can pick 10 random properties and check if you think I am biased?

Arne