Solution for CA2227 or better approach?

Here's what MSDN says about the error, and also how you can avoid it.

Here's my take on the issue.

Consider, the following class:

class BigDataClass
{
    public List<string> Data { get; set; }
}

This class will throw that exact same issue. Why? Because Collections do not need a setter. Now, we can do anything with that object: assign Data to an arbitrary List<string>, add elements to Data, remove elements from Data, etc. If we remove the setter, we only lose the ability to directly assign to that property.

Consider the following code:

class BigDataClass
{
    private List<string> data = new List<string>();
    public List<string> Data { get { return data; } } // note, we removed the setter
}

var bigData = new BigDataClass();
bigData.Data.Add("Some String");

This code is perfectly valid and in fact the recommended way to do things. Why? Because the List<string> is a reference to a memory location, that contains the remainder of the data.

Now, the only thing you cannot now do with this, is directly set the Data property. I.e. the following is invalid:

var bigData = new BigDataClass();
bigData.Data = new List<string>();

This is not necessarily a bad thing. You'll notice that on many .NET types this model is used. It's the basics of immutability. You usually do not want direct access to the mutability of Collections, as this can cause some accidental behavior that has strange issues. This is why Microsoft recommends you omit setters.

Example:

var bigData = new BigDataClass();
bigData.Data.Add("Some String");
var l2 = new List<string>();
l2.Add("String 1");
l2.Add("String 2");
bigData.Data = l2;
Console.WriteLine(bigData.Data[0]);

We might be expecting Some String, but we'll get String 1. This also means that you cannot reliably attach events to the Collection in question, so you cannot reliably determine if new values are added or values are removed.

A writable collection property allows a user to replace the collection with a completely different collection.

Essentially, if you only ever need to run the constructor, or assignment, once, then omit the set modifier. You won't need it, direct assignment of collections is against best-practices.

Now, I'm not saying never use a setter on a Collection, sometimes you may need one, but in general you should not use them.

You can always use .AddRange, .Clone, etc. on the Collections, you only lose the ability of direct assignment.

Serialization

Lastly, what do we do if we wish to Serialize or Deserialize a class that contains our Collection without a set? Well, there is always more than one way to do it, the simplest (in my opinion) is to create a property that represents the serialized collection.

Take our BigDataClass for example. If we wished to Serialize, and then Deserialize this class with the following code, the Data property would have no elements.

JavaScriptSerializer jss = new JavaScriptSerializer();
BigDataClass bdc = new BigDataClass();
bdc.Data.Add("Test String");
string serd = jss.Serialize(bdc);
Console.WriteLine(serd);
BigDataClass bdc2 = jss.Deserialize<BigDataClass>(serd);

So, to fix this, we can simply modify our BigDataClass a bit to make it use a new string property for Serialization purposes.

public class BigDataClass
{
    private List<string> data = new List<string>();
    [ScriptIgnore]
    public List<string> Data { get { return data; } } // note, we removed the setter

    public string SerializedData { get { JavaScriptSerializer jss = new JavaScriptSerializer(); return jss.Serialize(data); } set { JavaScriptSerializer jss = new JavaScriptSerializer(); data = jss.Deserialize<List<string>>(value); } }
}

Another option is always the DataContractSerializer (which is really a better option, in general.) You can find information about it on this StackOverflow question.

Thanks to @Matthew, @CraigW and @EBrown for helping me understanding the solution for this warning.

public class PO1Loop
{

    public SegmentTypes.PO1LoopSegmentTypes.PO1 PO1 { get; set; }

    public Collection<SegmentTypes.PO1LoopSegmentTypes.PID1> PIDRepeat1 { get; private set; }

    public Collection<SegmentTypes.PO1LoopSegmentTypes.PID2> PIDRepeat2 { get; private set; }

    public SegmentTypes.PO1LoopSegmentTypes.PO4 PO4 { get; set; }

    /* Max Use: 8 */
    public Collection<SegmentTypes.PO1LoopSegmentTypes.ACK> ACKRepeat { get; private set; }

    public PO1Loop()
    {
        PIDRepeat1 = new Collection<SegmentTypes.PO1LoopSegmentTypes.PID1>();
        PIDRepeat2 = new Collection<SegmentTypes.PO1LoopSegmentTypes.PID2>();
        ACKRepeat = new Collection<SegmentTypes.PO1LoopSegmentTypes.ACK>();
    }

}

When wanting to assign data to the collection types use AddRange, Clear or any other variation of method for modifying a collection.

With current VS2019 we can simply do this:

public List<string> Data { get; } = new List<string>();

This satisfies CA2227 and can be serialized/deserialized.

The deserialization works because List<> has an "Add" method, and the serializer knows how to handle a read-only collection property with an Add method (the property is read-only but not the elements) (I use Json.Net, other serializers may behave differently).

Edit: As pointed out it should be "=" and not "=>" (compiler will prevent you using "=>"). If we used "public List Data => new List();" then it would create a new list every time the property was accessed which is not what we want either.

Edit: Note that this will NOT work if the type of the property is an interface, such as IList

Edit: I think the handling of interfaces is determined by the serializer used. The following works perfectly. I'm sure all common serializers know how to handle ICollection. And if you have some custom interface that does not implement ICollection then you should be able to configure the serializer to handle it, but in that case CA2227 probably won't be triggered making it irrelevant here. (As it is a read-only property you have to assign a concrete value within the class so it should always be serializing and de-serializing a non-null value)

    public class CA2227TestClass
    {
        public IList Data { get; } = new List<string>();
    }

    [TestMethod]
    public void CA2227_Serialization()
    {
        var test = new CA2227TestClass()
        {
            Data = { "One", "Two", "Three" }
        };

        var json = JsonConvert.SerializeObject(test);

        Assert.AreEqual("{\"Data\":[\"One\",\"Two\",\"Three\"]}", json);

        var jsonObject = JsonConvert.DeserializeObject(json, typeof(CA2227TestClass)) as CA2227TestClass;

        Assert.IsNotNull(jsonObject);
        Assert.AreEqual(3, jsonObject.Data.Count);
        Assert.AreEqual("One", jsonObject.Data[0]);
        Assert.AreEqual("Two", jsonObject.Data[1]);
        Assert.AreEqual("Three", jsonObject.Data[2]);
        Assert.AreEqual(typeof(List<string>), jsonObject.Data.GetType());
    }

💡 Alternative Solution 💡

In my situation, making the property read-only was not viable because the whole list (as a reference) could change to a new list.

I was able to resolve this warning by changing the properties' setter scope to be internal.

public List<Batch> Batches
{
    get { return _Batches; }
    internal set { _Batches = value; OnPropertyChanged(nameof(Batches)); }
}

Note one could also use private set...

---

The hint's (achilleas heal) of this warning seems really pointed to libraries for the documentation says (Bolding mine):

An externally visible writable property is a type that implements System.Collections.ICollection.

For me it was, "Ok, I won't make it viewable externally...." and internal was fine for the app.