70 lines
2.6 KiB
Plaintext
70 lines
2.6 KiB
Plaintext
=== on 13 Dec 2016, 13:32:05 Valeri Hristov wrote:
|
|
The root problem that we are trying to solve with this rule is not the performance hit when copying an array, but the wrong assumption that the Array is an immutable data structure (it is not). Deep cloning an array could be used to achieve immutability of the collection, hence the performance problem.
|
|
|
|
|
|
A very good explanation why you should not return, or actually use arrays, unless you have to:
|
|
|
|
https://blogs.msdn.microsoft.com/ericlippert/2008/09/22/arrays-considered-somewhat-harmful/
|
|
|
|
|
|
\[~ann.campbell.2] could you have a look at this?
|
|
|
|
|
|
|
|
=== on 15 Dec 2016, 14:49:36 Ann Campbell wrote:
|
|
Per our discussion [~valeri.hristov], I'm going to leave this along for now pending the decision on whether to expand the scope of the rule to _all_ arrays.
|
|
|
|
=== on 9 Mar 2017, 10:04:16 Amaury Levé wrote:
|
|
The article and the Fxcop rules are actually covering two things:
|
|
|
|
1/ A design issue where ``++arrays++`` are mutable and so you might have unexpected behaviors.
|
|
|
|
2/ A performance issue when calling a property/method returning an array because the developer might be doing a copy (shallow or deep) of the array he is about to return.
|
|
|
|
|
|
Point #1 is easy to cover and my feeling is that this rule should become: ``++Arrays should not be used as the return type of a public method or property++``. About the description, I would say that developers tend to think an ``++Array++`` is immutable and therefore they won't anticipate the data can change over calls. I won't even specify exception cases because if the developer knows what he is doing then he is smart enough to mark the issue as ``++won't fix++``.
|
|
|
|
|
|
Point #2 might be partially covered by recommending to the developer to convert the ``++property++`` to a ``++method++`` when there is some kind of copy in it.
|
|
|
|
=== on 13 Mar 2017, 16:21:27 Ann Campbell wrote:
|
|
Updated per our discussion [~amaury.leve] and [~valeri.hristov]. Please double-check me.
|
|
|
|
=== on 13 Mar 2017, 16:31:03 Valeri Hristov wrote:
|
|
LGTM, changed the message a bit
|
|
|
|
=== on 12 Apr 2017, 16:21:07 Valeri Hristov wrote:
|
|
Here are a few ways to copy a collection or an array:
|
|
|
|
|
|
----
|
|
collection.ToList()
|
|
collection.ToArray()
|
|
array.Clone()
|
|
array.Copy()
|
|
array.CopyTo()
|
|
new List<T>(collection)
|
|
new HashSet<T>(collection)
|
|
... 10s of other collection types too, except ReadOnlyCollection<T> which keeps a reference to the original
|
|
new MyList(collection)
|
|
----
|
|
|
|
This is a case which should not raise an issue (which has many variations):
|
|
|
|
|
|
----
|
|
public string[] FemaleNames
|
|
{
|
|
get
|
|
{
|
|
if (femaleNames == null)
|
|
{
|
|
femaleNames = names.Where(IsFemale).ToArray();
|
|
}
|
|
return femaleNames;
|
|
}
|
|
}
|
|
----
|
|
|
|
|