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

Make collection properties not-nullable #30

Closed
sliekens opened this issue Nov 1, 2015 · 4 comments
Closed

Make collection properties not-nullable #30

sliekens opened this issue Nov 1, 2015 · 4 comments
Milestone

Comments

@sliekens
Copy link
Collaborator

sliekens commented Nov 1, 2015

Entities that have a collection property should initialize that property to an instance of an empty array. Property setters should throw when the given value is null. This is to avoid null reference exceptions when iterating over a collection property.

Example in class InfixUpgrade

private static readonly CombatAttribute[] Empty = new CombatAttribute[0];
private ICollection<CombatAttribute> attributes = Empty;

public ICollection<CombatAttribute> Attributes
{
    get { return this.attributes; }
    set
    {
        if (value == null)
            throw new ArgumentNullException();
        this.attributes = value;
    }
}

The Empty array must be cached to avoid multiple object allocations.

@sliekens sliekens added this to the v1.4.0 milestone Nov 1, 2015
@Ruhrpottpatriot
Copy link
Owner

Collection Properties should be read-only as per MS code guidelines. This allows individual items to be added or removed but the collection itself can't be changed. Your code should look about like this:

private static readonly CombatAttribute[] Empty = new CombatAttribute[0];
private ICollection<CombatAttribute> attributes = Empty;

public ICollection<CombatAttribute> Attributes
{
    get { return this.attributes; }
}

@sliekens
Copy link
Collaborator Author

sliekens commented Nov 1, 2015

The DTO converters require a public setter to set the converted collection. Changing that to list copy operations would degrade performance. Changing that to constructor arguments means not having a default constructor. I don't like either.

@sliekens
Copy link
Collaborator Author

sliekens commented Nov 1, 2015

PS: since this is gonna be a maintenance release, removing setters is out of the question.

In the same spirit, do not throw on null but fall back to the empty collection.

private static readonly CombatAttribute[] Empty = new CombatAttribute[0];
private ICollection<CombatAttribute> attributes = Empty;

public ICollection<CombatAttribute> Attributes
{
    get { return this.attributes; }
    set
    {
        this.attributes = value ?? Empty;
    }
}

@Ruhrpottpatriot
Copy link
Owner

We can change that, for 1.4, but see #32 for a 2.0 change.

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

No branches or pull requests

2 participants