Property that is set only within protected constructor

Question

I have following class, and wondering if declaring ListOfItems as public with a private setter is a proper way (from the OOP best practice point of view).

public abstract class Game
{
    public List<int> ListOfItemss { get; private set; }

    protected Game()
    {
    }

    protected Game(string gameInput)
    {
        ListOfItems = new List<int>();
        var gameParser = new GameParser();
        ListOfItems = gameParser.ParseGameInputString(gameInput);
    }

    public abstract int Bowl(int items);
}

Or should I made it via private field i.e.

public abstract class Game
{
    private List<int> _listOfItems;

    public List<int> ListOfItemss 
    {
            get { return _listOfItems; }
            private set { _listOfItems = value; }
        }

    protected Game()
    {
    }

    protected Game(string gameInput)
    {
        ListOfItems = new List<int>();
        _listOfItems = new List<int>(); 
        var gameParser = new GameParser();
        _listOfItems = gameParser.ParseGameInputString(gameInput);
    }

    public abstract int Bowl(int items);
}

-

Updated based on the suggestions: Based on the suggestions below, here is the code I'm think to have in the final version:

public abstract class Game
{
    public IReadOnlyCollection<int> ListOfItems { get; } = new List<int>();

    protected Game()
    {
    }

    protected Game(string gameInput)
    {
        var gameParser = new GameParser();
        ListOfItems = gameParser.ParseGameInputString(gameInput);
    }

    public abstract int Bowl(int items);
}

Both are the same thing - the only difference is that you control the private field in the second instance. You should implement either - the good thing is that if you implement it as a property and not a field (which is either of the above methods) you are able to adjust the implementation of the property without breaking other code/refactoring. — Charleh, Jan 16 '20 at 17:36
FYI, you don't need to set it to a new list when you immediately set it to something else. And even worse in the second one where you set it to two new lists before setting it to it's final value. — juharr, Jan 16 '20 at 17:37
Yeah and as the posted answer says - if you want the assignment to be in the constructor and it's never going to be anywhere else in the code you can just make it readonly by removing the setter. You can still add items to the list, you just can't assign a new list. — Charleh, Jan 16 '20 at 17:38
Just be careful exposing the list instance. You have no control over how it's used once that property is read. — madreflection, Jan 16 '20 at 17:39
Good point @madreflection Do you think it's better to change the access modifier to internal or protected? — KVN, Jan 16 '20 at 17:41
Note that making the `List` setter private only prevents assignment to the property, but clients can still call `ListOfItems.Clear()` and `ListOfItems.Add()` — Rufus L, Jan 16 '20 at 17:45
That depends on how the consumer needs to use the object. If you need to give the consumer read-only access to the items, consider wrapping the field (code block #2) in a `ReadOnlyCollection` and returning that. If you don't want the consumer to see the items, omit the property and just keep the field. — madreflection, Jan 16 '20 at 17:45
Sorry @Charleh can you please elaborate more on the "if you implement it as a property and not a field (which is either of the above methods)" - do you mean both of my methods implemented via Property or both of them implemented via Field? — KVN, Jan 16 '20 at 17:45
Thanks @RufusL and if I remove the setter for this property - will it prevent other clients from Adding/Clearing the list? — KVN, Jan 16 '20 at 17:48
Thanks @madreflection the idea is that I'm allowing the consumers to read the list and then use it (if needed) by their classes. But I do not want them to modify this list in anyway. — KVN, Jan 16 '20 at 17:49
Then returning a `ReadOnlyCollection` instead of `List` would be the way to go. Ideally, you would declare the property as `IReadOnlyList`. — madreflection, Jan 16 '20 at 17:50

nalka · Answer 1 · 2020-01-16T19:01:51.213

1

You can just remove the setter : public List<int> ListOfItemss { get; }

Edit : As mentionned in the comments, you can get more infos about the difference between public List<int> ListOfItemss { get; } and public List<int> ListOfItemss { get; private set; } here

edited Jan 16 '20 at 19:01

answered Jan 16 '20 at 17:35

nalka

1,894
11
26

For the difference between the two (with, or without setter) https://stackoverflow.com/a/36792426/5062791 – ColinM Jan 16 '20 at 17:38

Andrew Rands · Accepted Answer · 2020-01-16T18:05:10.880

1

Remove the setter (which effectively makes it private to constructors). As Rufus has pointed out, if you don't want this list modifying outside the class, don't expose it as a list. Also, you allow the construction of a Game with a null ListOfItemss. I'd make that default constructor initialise the list, like the other c'tor does. This makes them consistent and avoids nasty null reference exceptions if you try to access game.ListOfItemss:

public abstract class Game
{
    public IEnumerable<int> ListOfItemss { get; } = new List<int>();

    protected Game()
    {
    }

    protected Game(string gameInput)
    {
        var gameParser = new GameParser();
        ListOfItemss = gameParser.ParseGameInputString(gameInput);
    }

    public abstract int Bowl(int items);
}

edited Jan 16 '20 at 18:05

answered Jan 16 '20 at 17:48

Andrew Rands

154
9

1

Clients can still modify the list contents in this case – Rufus L Jan 16 '20 at 17:51
As he allows construction without content, that would make sense. If there was just the parameterised constructor, you'd want the public property to be IEnumerable – Andrew Rands Jan 16 '20 at 17:54
He stated (in the comments) that *"I do not want them to modify this list in anyway"*, so I guess the list gets populated elsewhere from within the class (?). – Rufus L Jan 16 '20 at 17:56
1

It's an abstract class, so exposing the property to deriving classes likely was the intended means of populating the list. Making the list property `protected` and having a separate, `public` read-only list/collection would shore things up. – madreflection Jan 16 '20 at 18:13

score 1 · Answer 3 · answered Jan 17 '20 at 07:00

Frankly, you shouldn't be providing external access to that list at all, this violates encapsulation. Even though the property is declared IReadOnlyList<int>, there's nothing stopping someone from doing (List<int>)game.ListOfItemss. Private setter or not doesn't really matter, as long as you're limiting access to the internals. If you need to access items of your list externally, expose it through controlled methods such as an indexer.

class Option0
{
    private readonly List<int> items;
    public int this[int index] => this.items[index];
}

Only your class or perhaps derived classes should be allowed access to this list.

With that being said, if you want to expose it as a truly read only list, expose the read only view of it only.

class Option1
{
    private readonly List<int> items;
    public IReadOnlyList<int> Items => items.AsReadOnly();
    public Option1()
    {
        this.items = ...;
    }
}

class Option2
{
    private readonly List<int> items;
    public IReadOnlyList<int> Items { get; }
    public Option2()
    {
        this.items = ...;
        this.Items = this.items.AsReadOnly();
    }
}

Property that is set only within protected constructor

3 Answers3