Does this property make sense

  • Thread starter Thread starter Tony Johansson
  • Start date Start date
T

Tony Johansson

Hello!

I'm reading in a book and here is a property that doesn't make sense at all.
the code look like this.
public List<FoodItem> FoodList
{
get
{
FoodItem[] listCopy = null;
listCopy = new FoodItem[]{};
m_foodItemList.CopyTo(listCopy);
return m_foodItemList;
}

set
{
m_foodItemList = value;
}
}

This code create a local copy in an array named listCopy from the original
collection m_foodItemList.
The code then return the original collection m_foodItemList.
So what is the point to create a local array that will go out of scope when
the original collection m_foodItemList is returned.

//Tony
 
I'm reading in a book and here is a property that doesn't make sense at all.
the code look like this.
public List<FoodItem> FoodList
{
get
{
FoodItem[] listCopy = null;
listCopy = new FoodItem[]{};
m_foodItemList.CopyTo(listCopy);
return m_foodItemList;
}

set
{
m_foodItemList = value;
}
}

This code create a local copy in an array named listCopy from the original
collection m_foodItemList.
The code then return the original collection m_foodItemList.
So what is the point to create a local array that will go out of scope when
the original collection m_foodItemList is returned.
Click to expand...

book.MoveTo(trash);

:-)

BTW, I would expect the method to crash, because the array
has zero elements.

Arne
 
Arne Vajhøj said:
I'm reading in a book and here is a property that doesn't make sense at
all.
the code look like this.
public List<FoodItem> FoodList
{
get
{
FoodItem[] listCopy = null;
listCopy = new FoodItem[]{};
m_foodItemList.CopyTo(listCopy);
return m_foodItemList;
}

set
{
m_foodItemList = value;
}
}

This code create a local copy in an array named listCopy from the
original
collection m_foodItemList.
The code then return the original collection m_foodItemList.
So what is the point to create a local array that will go out of scope
when
the original collection m_foodItemList is returned.
Click to expand...

book.MoveTo(trash);

:-)

BTW, I would expect the method to crash, because the array
has zero elements.

Arne
Click to expand...

I mean assume that the collection m_foodItemList
has some items. I still doesn't understand why to create a local array copy
that goes out of scope
when this property returns ?
Can you?

//Tony
 
Arne Vajhøj said:
I'm reading in a book and here is a property that doesn't make sense at
all.
the code look like this.
public List<FoodItem> FoodList
{
get
{
FoodItem[] listCopy = null;
listCopy = new FoodItem[]{};
m_foodItemList.CopyTo(listCopy);
return m_foodItemList;
}

set
{
m_foodItemList = value;
}
}

This code create a local copy in an array named listCopy from the
original
collection m_foodItemList.
The code then return the original collection m_foodItemList.
So what is the point to create a local array that will go out of scope
when
the original collection m_foodItemList is returned.
Click to expand...

book.MoveTo(trash);

:-)

BTW, I would expect the method to crash, because the array
has zero elements.
Click to expand...

I mean assume that the collection m_foodItemList
has some items. I still doesn't understand why to create a local array copy
that goes out of scope
when this property returns ?
Can you?
Click to expand...

No.

And it is when the list has elements I expect problems
copying data to an array with zero elements.

Arne
 
Arne Vajhøj said:
Arne Vajhøj said:
On 25-01-2011 10:41, Tony Johansson wrote:
I'm reading in a book and here is a property that doesn't make sense at
all.
the code look like this.
public List<FoodItem> FoodList
{
get
{
FoodItem[] listCopy = null;
listCopy = new FoodItem[]{};
m_foodItemList.CopyTo(listCopy);
return m_foodItemList;
}

set
{
m_foodItemList = value;
}
}

This code create a local copy in an array named listCopy from the
original
collection m_foodItemList.
The code then return the original collection m_foodItemList.
So what is the point to create a local array that will go out of scope
when
the original collection m_foodItemList is returned.

book.MoveTo(trash);

:-)

BTW, I would expect the method to crash, because the array
has zero elements.
Click to expand...

I mean assume that the collection m_foodItemList
has some items. I still doesn't understand why to create a local array
copy
that goes out of scope
when this property returns ?
Can you?
Click to expand...

No.

And it is when the list has elements I expect problems
copying data to an array with zero elements.

Arne
Click to expand...

What do mean with this text that you wrote in the previous mail ?
And it is when the list has elements I expect problems
copying data to an array with zero elements.
Click to expand...

//Tony
 
What do mean with this text that you wrote in the previous mail ?
Click to expand...

Read the help for List<T>.CopyTo(). Look at the description of when it will
throw an ArgumentException.
 
Arne Vajhøj said:
"Arne Vajhøj"<[email protected]> skrev i meddelandet
On 25-01-2011 10:41, Tony Johansson wrote:
I'm reading in a book and here is a property that doesn't make sense at
all.
the code look like this.
public List<FoodItem> FoodList
{
get
{
FoodItem[] listCopy = null;
listCopy = new FoodItem[]{};
m_foodItemList.CopyTo(listCopy);
return m_foodItemList;
}

set
{
m_foodItemList = value;
}
}

This code create a local copy in an array named listCopy from the
original
collection m_foodItemList.
The code then return the original collection m_foodItemList.
So what is the point to create a local array that will go out of scope
when
the original collection m_foodItemList is returned.

book.MoveTo(trash);

:-)

BTW, I would expect the method to crash, because the array
has zero elements.

I mean assume that the collection m_foodItemList
has some items. I still doesn't understand why to create a local array
copy
that goes out of scope
when this property returns ?
Can you?
Click to expand...

No.

And it is when the list has elements I expect problems
copying data to an array with zero elements.
Click to expand...

What do mean with this text that you wrote in the previous mail ?
And it is when the list has elements I expect problems
copying data to an array with zero elements.
Click to expand...
Click to expand...

If the list has 7 elements and the CopyTo method has to
copy them to an array with zero elements I would expect
something bad to happen.

The docs state:

<quote>
ArgumentException

The number of elements in the source List is greater than the number of
elements that the destination array can contain.
</quote>

Arne
 
Hello!

I'm reading in a book and here is a property that doesn't make sense at all.
the code look like this.
public List<FoodItem> FoodList
{
get
{
FoodItem[] listCopy = null;
listCopy = new FoodItem[]{};
m_foodItemList.CopyTo(listCopy);
return m_foodItemList;
}

set
{
m_foodItemList = value;
}
}

This code create a local copy in an array named listCopy from the original
collection m_foodItemList.
The code then return the original collection m_foodItemList.
So what is the point to create a local array that will go out of scope when
the original collection m_foodItemList is returned.

//Tony
Click to expand...

Yes, that property does not make any sense at all. Since it got both get
and set accessors, the code can be just like this,

public List<FoodItem> FoodList
{
get
{
return m_foodItemList;
}
set
{
m_foodItemList = value;
}
}

If the author intent is to return a copy of the current list (does not
allow current list to be modified), he should get rid of the set accessor.

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

or better, wrap the current list in a ReadOnlyCollection and return that
instead

List<FoodItem> m_foodItemList;
ReadOnlyCollection<FoodItem> m_readonlyFoodItemList;

inside contructor...
{
m_foodItemList = ...
m_readonlyFoodItemList = new
ReadOnlyCollection<FoodItem>(m_foodItemList);
}

then the property...

public ReadOnlyCollection<FoodItem> FoodList
{
get
{
return m_readonlyFoodItemList;
}
}
 
I'm reading in a book and here is a property that doesn't make sense
at all.
the code look like this.
public List<FoodItem> FoodList
{
get
{
FoodItem[] listCopy = null;
listCopy = new FoodItem[]{};
m_foodItemList.CopyTo(listCopy);
return m_foodItemList;
}

set
{
m_foodItemList = value;
}
}

This code create a local copy in an array named listCopy from the
original
collection m_foodItemList.
The code then return the original collection m_foodItemList.
So what is the point to create a local array that will go out of scope
when
the original collection m_foodItemList is returned.
Click to expand...

Yes, that property does not make any sense at all. Since it got both get
and set accessors, the code can be just like this,

public List<FoodItem> FoodList
{
get
{
return m_foodItemList;
}
set
{
m_foodItemList = value;
}
}
Yep.

If the author intent is to return a copy of the current list (does not
allow current list to be modified), he should get rid of the set accessor.

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

or better, wrap the current list in a ReadOnlyCollection and return that
instead

List<FoodItem> m_foodItemList;
ReadOnlyCollection<FoodItem> m_readonlyFoodItemList;

inside contructor...
{
m_foodItemList = ...
m_readonlyFoodItemList = new ReadOnlyCollection<FoodItem>(m_foodItemList);
}

then the property...

public ReadOnlyCollection<FoodItem> FoodList
{
get
{
return m_readonlyFoodItemList;
}
}
Click to expand...

Which is a very good point.

Returning a ref to a modifiable List<> that is
a private field in a class is really somewhat
breaking the encapsulation.

m_readonlyFoodItemList = new ReadOnlyCollection<FoodItem>(m_foodItemList);

can also be written as:

m_readonlyFoodItemList = m_foodItemList.AsReadOnly();

Arne
 
Arne Vajhøj said:
I'm reading in a book and here is a property that doesn't make sense
at all.
the code look like this.
public List<FoodItem> FoodList
{
get
{
FoodItem[] listCopy = null;
listCopy = new FoodItem[]{};
m_foodItemList.CopyTo(listCopy);
return m_foodItemList;
}

set
{
m_foodItemList = value;
}
}

This code create a local copy in an array named listCopy from the
original
collection m_foodItemList.
The code then return the original collection m_foodItemList.
So what is the point to create a local array that will go out of scope
when
the original collection m_foodItemList is returned.
Click to expand...

Yes, that property does not make any sense at all. Since it got both get
and set accessors, the code can be just like this,

public List<FoodItem> FoodList
{
get
{
return m_foodItemList;
}
set
{
m_foodItemList = value;
}
}
Yep.

If the author intent is to return a copy of the current list (does not
allow current list to be modified), he should get rid of the set
accessor.

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

or better, wrap the current list in a ReadOnlyCollection and return that
instead

List<FoodItem> m_foodItemList;
ReadOnlyCollection<FoodItem> m_readonlyFoodItemList;

inside contructor...
{
m_foodItemList = ...
m_readonlyFoodItemList = new
ReadOnlyCollection<FoodItem>(m_foodItemList);
}

then the property...

public ReadOnlyCollection<FoodItem> FoodList
{
get
{
return m_readonlyFoodItemList;
}
}
Click to expand...

Which is a very good point.

Returning a ref to a modifiable List<> that is
a private field in a class is really somewhat
breaking the encapsulation.

m_readonlyFoodItemList = new
ReadOnlyCollection<FoodItem>(m_foodItemList);

can also be written as:

m_readonlyFoodItemList = m_foodItemList.AsReadOnly();

Arne
Click to expand...

Here you mentioned that
Returning a ref to a modifiable List<> that is
a private field in a class is really somewhat
breaking the encapsulation.
Click to expand...

Now I wonder is it bad to return a ref to a private string array like I do
here in the FoodItem class.
Assume I only want to read the string array so is better to return a copy of
the string array perhaps.
class FoodItem
{
private string[] ingredients;

public string[] Ingredients
{
get { return ingredients; }
set { ingredients = value; }
}
}

//Tony
 
Arne Vajhøj said:
On 1/25/2011 11:41 PM, Tony Johansson wrote:
I'm reading in a book and here is a property that doesn't make sense
at all.
the code look like this.
public List<FoodItem> FoodList
{
get
{
FoodItem[] listCopy = null;
listCopy = new FoodItem[]{};
m_foodItemList.CopyTo(listCopy);
return m_foodItemList;
}

set
{
m_foodItemList = value;
}
}

This code create a local copy in an array named listCopy from the
original
collection m_foodItemList.
The code then return the original collection m_foodItemList.
So what is the point to create a local array that will go out of scope
when
the original collection m_foodItemList is returned.

Yes, that property does not make any sense at all. Since it got both get
and set accessors, the code can be just like this,

public List<FoodItem> FoodList
{
get
{
return m_foodItemList;
}
set
{
m_foodItemList = value;
}
}
Yep.

If the author intent is to return a copy of the current list (does not
allow current list to be modified), he should get rid of the set
accessor.

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

or better, wrap the current list in a ReadOnlyCollection and return that
instead

List<FoodItem> m_foodItemList;
ReadOnlyCollection<FoodItem> m_readonlyFoodItemList;

inside contructor...
{
m_foodItemList = ...
m_readonlyFoodItemList = new
ReadOnlyCollection<FoodItem>(m_foodItemList);
}

then the property...

public ReadOnlyCollection<FoodItem> FoodList
{
get
{
return m_readonlyFoodItemList;
}
}
Click to expand...

Which is a very good point.

Returning a ref to a modifiable List<> that is
a private field in a class is really somewhat
breaking the encapsulation.

m_readonlyFoodItemList = new
ReadOnlyCollection<FoodItem>(m_foodItemList);

can also be written as:

m_readonlyFoodItemList = m_foodItemList.AsReadOnly();
Click to expand...

Here you mentioned that
Returning a ref to a modifiable List<> that is
a private field in a class is really somewhat
breaking the encapsulation.
Click to expand...

Now I wonder is it bad to return a ref to a private string array like I do
here in the FoodItem class.
Assume I only want to read the string array so is better to return a copy of
the string array perhaps.
class FoodItem
{
private string[] ingredients;

public string[] Ingredients
{
get { return ingredients; }
set { ingredients = value; }
}
}
Click to expand...

That code will allow code outside the class to replace
elements of the array.

That could be considered a problem.

I would prefer:

class FoodItem
{
private List<string> ingredients;
public List<string> Ingredients
{
get { return ingredients.AsReadOnly(); }
}
public void Add(string ingr)
{
ingredients.Add(ingr);
}
}

Because that gives the FoodItem class complete
control over the private data.

Arne
 
[...]
get { return ingredients.AsReadOnly(); }
Click to expand...

Try: "return Array.AsReadOnly(ingredients)". It's a static,
non-extension method. :)
Click to expand...

That would give a compile error given that the full
code snippet was:

class FoodItem
{
private List<string> ingredients;
public List<string> Ingredients
{
get { return ingredients.AsReadOnly(); }
}
public void Add(string ingr)
{
ingredients.Add(ingr);
}
}

It will work with the OP code though.

Arne
 
On 1/27/11 12:09 AM, Arne Vajhøj wrote:
[...]
get { return ingredients.AsReadOnly(); }

Try: "return Array.AsReadOnly(ingredients)". It's a static,
non-extension method. :)
Click to expand...

That would give a compile error given that the full
code snippet was:

class FoodItem
{
private List<string> ingredients;
public List<string> Ingredients
{
get { return ingredients.AsReadOnly(); }
}
[...]
It will work with the OP code though.
Click to expand...

True. It didn't occur to me that you would have completely redefined the
problem as part of helping. I should have read more closely.
Click to expand...

Not redefined the problem, but redefined the solution a bit.

And actually the idea was not even mine - kndg introduced that, but I
completely agreed so I kept it.

I don't think I would ever have a property returning a collection
for a backing array. That simply does not make sense to me.

Arne
 
Back
Top