HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Immutable pure data classes with public setters on properties

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
purewithpublicpropertiesimmutableclassessettersdata

Problem

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 popularClasses, IEnumerable unpopularClasses, int reqPreferences, int minUnpopularPreferences)
    {
        PopularClasses  = popularClasses;
        UnpopularClasses = unpopularClasses;
        RequiredPrefereces = reqPreferences;
        MinUnpopularPrefereces = minUnpopularPreferences;
    }

    public IEnumerable 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 PopularClasses { get; private set; }
    public  IEnumerable 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 d

Solution

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 popularClasses = new List;
popularClasses.add("hello");

pscos = new PopularitySplitClassOptionSet(..., popularClasses, ...);
popularClasses.add("sup"); // This is now added to pscos.

List getAlsoLeaks = (List)(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 for this purpose, unless I will need random access, in which case I use List. Since you're only currently using IEnumerable, I'm assuming that random access isn't an issue and using LinkedList.

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

Code Snippets

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.
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);
    }
}

Context

StackExchange Code Review Q#8543, answer score: 2

Revisions (0)

No revisions yet.