GW2.NET icon indicating copy to clipboard operation
GW2.NET copied to clipboard

Changing Collection properties to conform to CA1819 and CA2227

Open Ruhrpottpatriot opened this issue 9 years ago • 2 comments

As of now our collection properties violate the CA1819 and CA2227 rules for managed code. We should change the collection properties to conform to those rules, as seem exemplary below. This is the recommended way to do things and should not affect performance in any significant way.

public ICollection<CombatAttribute> Attributes { get; private set; } = new Collection<CombatAttribute>(0);

public void SetCollection(IEnumerable<CombatAttribute> attributes)
{
    if(attributes == null)
    {
        throw new ArgumentNullException();
    }
    this.Attributes = attributes;
}

Since the C# compiler generates instance methods in the MSIL for properties, we can remove the setter and replace it with a method which just assigns the collection. This shold not degrade performance

References: #30

Ruhrpottpatriot avatar Nov 02 '15 11:11 Ruhrpottpatriot

 = new Collection<CombatAttribute>(0);

This adds some amount of overhead since most of the time we do need to call SetCollection with a non-empty collection. To minimize the overhead, it's important that the empty collection is cached, readonly and immutable. A static readonly array with length 0 is perfect for that.

(The side effects described by CA1819 do not apply when the array length is 0: zero-length arrays are immutable)

sliekens avatar Nov 02 '15 11:11 sliekens

Yeah, I was too lazy to write a constructor and backing field. Simple example to illustrate my point.

Ruhrpottpatriot avatar Nov 02 '15 20:11 Ruhrpottpatriot