Immutable pure data classes with public setters on properties

Question

I'm putting together some classes as a model for some information (that I'm currently pulling from a website).

The classes are implemented in C# - because in the current version of F# there are no autoimplemented properties. The logic to fill these classes from the website is going to be writen in F#. (Because I like F#, and it works nice for webscraping.)

So since I'm working functionally (I seems to always work functionally there days even in C#). I don't want these objects to be mutable. Setting mutable fields is ugly in F# (and for good reason, mutable objects are evil).

So all there data fields have only private setters but this makes my constructors long. I planned to be using named arguments in them anyway, but still 5 arguments is a lot. And I don't think F# supports object property initialisers anyway.

public class PopularitySplitClassOptionSet : IClassOptionsSet
{
 public PopularitySplitClassOptionSet (string description, IEnumerable<ClassOption> popularClasses, IEnumerable<ClassOption> unpopularClasses, int reqPreferences, int minUnpopularPreferences)
 {
 PopularClasses = popularClasses;
 UnpopularClasses = unpopularClasses;
 RequiredPrefereces = reqPreferences;
 MinUnpopularPrefereces = minUnpopularPreferences;
 }
 public IEnumerable<ClassOption> Classes
 {
 get 
 {
 return PopularClasses.Concat(UnpopularClasses);
 }
 }
 public int RequiredPrefereces { get; private set; }
 public int MinUnpopularPrefereces { get; private set; }
 public int MaxPopularPrefereces { 
 get 
 {
 return RequiredPrefereces - MinUnpopularPrefereces;
 }
 }
 public IEnumerable<ClassOption> PopularClasses { get; private set; }
 public IEnumerable<ClassOption> UnpopularClasses { get; private set; }
}

Edit: Is this good practice? Am I thinking this right?

Also: I wonder if:

public readonly int RequiredPrefereces { get; private set; }

would be better? Or is that not actually a thing you can do?

Related questions:

• C#, immutability and public readonly fields
• How to avoid "too many parameters" problem in API design?

• jdmichal: So what's the question you would like answered?
• Frames Catherine White: This is Code Review.SE. I thought the question was implict. Is this good practice>
• ANeves: @Oxinabox But this makes my constructors long. I wouldn't say half a dozen lines is long; but even if it is, so what?
• jdmichal: @Oxinabox There are still many different aspects of code that can be reviewed. In order to make a question-answer format viable, targetted questions are needed.
• Frames Catherine White: By my constuctors long, i mean: "Makes my constructors take lots of parameters"

Answer 1

This is what I program when I want "immutable" classes in C#. You do have an access leak right now though, in that PopularClasses and UnpopularClasses both set and get the underlying object, allowing modification of that object, thus making the class immutable. (This is a bigger problem for set than get, as get would require figuring out which collection type is actually being held. Set you already know because you set it.)

List<string> popularClasses = new List<string>;
popularClasses.add("hello");
pscos = new PopularitySplitClassOptionSet(..., popularClasses, ...);
popularClasses.add("sup"); // This is now added to pscos.
List<string> getAlsoLeaks = (List<string>)(pscos.PopularClasses);
getAlsoLeaks.add("yo"); // This is also added.

You will need to copy on get and set to avoid this. Usually I use LinkedList<T> for this purpose, unless I will need random access, in which case I use List<T>. Since you're only currently using IEnumerable<T>, I'm assuming that random access isn't an issue and using LinkedList<T>.

private LinkedList<ClassOption> popularClasses;
public IEnumerable<ClassOption> PopularClasses
{
 get
 {
 foreach (ClassOption classOption in list.popularClasses)
 yield return classOption;
 // OR:
 // return new LinkedList<ClassOption>(this.popularClasses);
 }
 set
 {
 this.popularClasses = new LinkedList<ClassOption>(value);
 }
}

• Frames Catherine White: Huh, normally I remember the posibility if casing back to the original type. I forgot this time, because I planned on filling them with F# types (which are immutable, except array), in particular i planned on filling them with straight Seq (which is an alias for IEnumerable), but of course, the mutable Array implements Seq.
• jdmichal: @Oxinabox I'll have to apologize for not being too familiar with F#'s types. If you're going to plug in immutable things, then obviously you won't need to worry about it in your class. Just be sure to comment such!
• Frames Catherine White: No you're right. Microsoft.FSharp.Collections.SeqModule.ReadOnly(Seq) exists for just that purpose. why doucment, when i can enforce?

Answer 2

Here's a take where the collection objects are passed back as read-only to the caller and the member variables are completely immutable:

public sealed class PopularitySplitClassOptionSet : IClassOptionsSet
{
 private readonly string description;
 private readonly IEnumerable<ClassOption> popularClasses;
 private readonly IEnumerable<ClassOption> unpopularClasses;
 private readonly int requiredPrefereces;
 private readonly int minUnpopularPrefereces;
 private readonly IEnumerable<ClassOption> classes;
 private readonly int maxPopularPrefereces;
 public PopularitySplitClassOptionSet(
 string description,
 IEnumerable<ClassOption> popularClasses,
 IEnumerable<ClassOption> unpopularClasses,
 int reqPreferences,
 int minUnpopularPreferences)
 {
 if (description == null)
 {
 throw new ArgumentNullException("description");
 }
 if (popularClasses == null)
 {
 throw new ArgumentNullException("popularClasses");
 }
 if (unpopularClasses == null)
 {
 throw new ArgumentNullException("unpopularClasses");
 }
 if (minUnpopularPreferences > reqPreferences)
 {
 throw new ArgumentOutOfRangeException(
 "minUnpopularPreferences",
 minUnpopularPreferences,
 "The minimum number of unpopular preferences may not exceed the number of required preferences.");
 }
 this.description = description;
 this.popularClasses = popularClasses.ToList().AsReadOnly();
 this.unpopularClasses = unpopularClasses.ToList().AsReadOnly();
 this.requiredPrefereces = reqPreferences;
 this.minUnpopularPrefereces = minUnpopularPreferences;
 this.classes = this.popularClasses.Concat(this.unpopularClasses).ToList().AsReadOnly();
 this.maxPopularPrefereces = this.requiredPrefereces - this.minUnpopularPrefereces;
 }
 public string Description
 {
 get
 {
 return this.description;
 }
 }
 public IEnumerable<ClassOption> Classes
 {
 get
 {
 return this.classes;
 }
 }
 public int RequiredPrefereces
 {
 get
 {
 return this.requiredPrefereces;
 }
 }
 public int MinUnpopularPrefereces
 {
 get
 {
 return this.minUnpopularPrefereces;
 }
 }
 public int MaxPopularPrefereces
 {
 get
 {
 return this.maxPopularPrefereces;
 }
 }
 public IEnumerable<ClassOption> PopularClasses
 {
 get
 {
 return this.popularClasses;
 }
 }
 public IEnumerable<ClassOption> UnpopularClasses
 {
 get
 {
 return this.unpopularClasses;
 }
 }
}

I think a five or six line constructor is completely acceptable. In fact, my revision includes some sanity checks on the parameters. The second revision does the calculations in the constructor since the values are immutable (no need to do those concats and subtraction in the properties).

• jdmichal: +1. I always forget about that wraskly AsReadOnly method! Also, this is what my constructors usually look like too, as far as error checking goes. You get used to it after a while; it stops looking "long" and starts looking "right" instead.
• Frames Catherine White: Why are you calculating these things in the constructor? IEnumereable.Concat and subtraction are O(1) operations. Ie they are basically free. It would seem clearer to have them in the properties
• Frames Catherine White: Also Is this a reasonable way to make my IEnumerable's Readonly: Microsoft.FSharp.Collections.SeqModule.ReadOnly(classes) msdn.microsoft.com/en-us/library/ee340408.aspx
• Jesse C. Slicer: @Oxinaboxas to your first question - because they're immutable and invariant. Let the calculation be done and no more. To your second question, I'm afraid I've never seen that method before as I'm not big into F#, sorry.