C# - Code Analysis 2227 Confusion

Question

I have a class property that looks as follows:

public List<Recipe> RecipeList
{
    get { return this._recipeList; }

    set
    {
        this._recipeList = value;
        OnPropertyChanged("RecipeList");
    }
}

In another method I have the following which references the property above.

private void RecipeSearch()
{
            this.RecipeList = RecipeManagerService.SearchByUnit(SearchCriteria)
                               .Where(recipe => recipe.IsApproved == true && !recipe.IsHidden).ToList();
}

Code Analysis is issuing a CA 2227 warning: Change RecipeList to be read-only by removing the setter. Could anyone tell me why?

Answer 1

Adding a public setter on a List<T> object is dangerous. You can eliminate this warning by making your setter private:

public List<Recipe> RecipeList
{
    get { return this._recipeList; }

    private set
    {
        this._recipeList = value;
        OnPropertyChanged("RecipeList");
    }
}

This will still allow your class to change this method, but no external source.

Answer 2

I think it's suggesting that usually collection properties themselves shouldn't be mutable - it's more common for the collection to be mutable, and just available via a setter.

It's only a suggestion though :)

In this case you'd use:

RecipeList.Clear();
RecipeList.AddRange(RecipeManagerService
                              .SearchByUnit(SearchCriteria)
                              .Where(r => r.IsApproved && !r.IsHidden));

Note that this won't fire the change event though... you might want to use ObservableCollection instead.

This will also mean that anyone can change the contents of the recipe list... do you definitely want that? Another alternative is to expose a ReadOnlyCollection<T> property or something like that, and only make changes within your own class. It really depends what you're trying to do though.