Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing Collection properties to conform to CA1819 and CA2227 #32

Open
Ruhrpottpatriot opened this issue Nov 2, 2015 · 2 comments
Open

Comments

@Ruhrpottpatriot
Copy link
Owner

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

@sliekens
Copy link
Collaborator

sliekens commented Nov 2, 2015

 = 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)

@Ruhrpottpatriot
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants