Skip to main content
corrected bad formatting
Source Link
Rick Davin
  • 6.8k
  • 1
  • 20
  • 32

public class User { // Intentionally a very simplified DTO class public string Name { get; set; } public bool IsAdmin { get; set; } }

public class User
{
    // Intentionally a very simplified DTO class
    public string Name { get; set; }
    public bool IsAdmin { get; set; }
}

public class User { // Intentionally a very simplified DTO class public string Name { get; set; } public bool IsAdmin { get; set; } }

public class User
{
    // Intentionally a very simplified DTO class
    public string Name { get; set; }
    public bool IsAdmin { get; set; }
}
Show why a private setter is not the right answer
Source Link
Rick Davin
  • 6.8k
  • 1
  • 20
  • 32

UPDATE #2 WHY A PRIVATE SETTER IS USELESS

OP commented asked why not use a private setter on a list? The answer is because while someone cannot change the reference to the overall list, they can still change individual items.

Example code:

A very simple User class

public class User { // Intentionally a very simplified DTO class public string Name { get; set; } public bool IsAdmin { get; set; } }

Some class that works with some users. Note no user is an Admin.

public class SomeClassWithUsers
{
    public List<User> UserList1 { get; private set; }

    private List<User> _users = new List<User>();
    public IReadOnlyList<User> UserList2 => _users;

    public static SomeClassWithUsers CreateSample()
    {
        var x = new SomeClassWithUsers();
        x.CreateSampleUsers();
        return x;
    }

    public void CreateSampleUsers()
    {
        _users = new List<User>()
        {
            new User() {Name = "Alice", IsAdmin = false },
            new User() {Name = "Bob", IsAdmin = false },
            new User() {Name = "Carl", IsAdmin = false },
            new User() {Name = "Dan", IsAdmin = false },
            new User() {Name = "Eve", IsAdmin = false },
        };

        UserList1 = _users.ToList();  // independent copy
    }
}

Okay, so we have 2 different user lists. Are both of them protected from external changes? No. Even though UserList1 has a private setter, someone can still alter individual items.

Example:

static void Main(string[] args)
{
    var x = SomeClassWithUsers.CreateSample();

    // Even though UserList1 has a private setter, I can still change individual members.
    for (var i = 0; i < x.UserList1.Count; i++)
    {
        x.UserList1[i] = new User() { Name = $"Evil {x.UserList1[i].Name}", IsAdmin = true };
    }

    Console.WriteLine("UserList1 has been modifed!");
    foreach (var user in x.UserList1)
    {
        Console.WriteLine($"{user.Name} {(user.IsAdmin ? "IS" : "is NOT")} an Admin.");
    }

            // But I cannot altger UserList2 in any way since it is properly marked as a IReadOnlyList.
            // You cannot compile the code below.
            //for (var i = 0; i < x.UserList2.Count; i++)
            //{
            //    x.UserList2[i] = new User() { Name = $"Evil {x.UserList1[2].Name}", IsAdmin = true };
            //}

    Console.WriteLine("\nUserList2 remains unchanged.");
    foreach (var user in x.UserList2)
    {
        Console.WriteLine($"{user.Name} {(user.IsAdmin ? "IS" : "is NOT")} an Admin.");
    }

    Console.WriteLine("\nPress ENTER key to close");
    Console.ReadLine();
}

Console output:

UserList1 has been modifed!
Evil Alice IS an Admin.
Evil Bob IS an Admin.
Evil Carl IS an Admin.
Evil Dan IS an Admin.
Evil Eve IS an Admin.

UserList2 remains unchanged.
Alice is NOT an Admin.
Bob is NOT an Admin.
Carl is NOT an Admin.
Dan is NOT an Admin.
Eve is NOT an Admin.

Press ENTER key to close

UPDATE #2 WHY A PRIVATE SETTER IS USELESS

OP commented asked why not use a private setter on a list? The answer is because while someone cannot change the reference to the overall list, they can still change individual items.

Example code:

A very simple User class

public class User { // Intentionally a very simplified DTO class public string Name { get; set; } public bool IsAdmin { get; set; } }

Some class that works with some users. Note no user is an Admin.

public class SomeClassWithUsers
{
    public List<User> UserList1 { get; private set; }

    private List<User> _users = new List<User>();
    public IReadOnlyList<User> UserList2 => _users;

    public static SomeClassWithUsers CreateSample()
    {
        var x = new SomeClassWithUsers();
        x.CreateSampleUsers();
        return x;
    }

    public void CreateSampleUsers()
    {
        _users = new List<User>()
        {
            new User() {Name = "Alice", IsAdmin = false },
            new User() {Name = "Bob", IsAdmin = false },
            new User() {Name = "Carl", IsAdmin = false },
            new User() {Name = "Dan", IsAdmin = false },
            new User() {Name = "Eve", IsAdmin = false },
        };

        UserList1 = _users.ToList();  // independent copy
    }
}

Okay, so we have 2 different user lists. Are both of them protected from external changes? No. Even though UserList1 has a private setter, someone can still alter individual items.

Example:

static void Main(string[] args)
{
    var x = SomeClassWithUsers.CreateSample();

    // Even though UserList1 has a private setter, I can still change individual members.
    for (var i = 0; i < x.UserList1.Count; i++)
    {
        x.UserList1[i] = new User() { Name = $"Evil {x.UserList1[i].Name}", IsAdmin = true };
    }

    Console.WriteLine("UserList1 has been modifed!");
    foreach (var user in x.UserList1)
    {
        Console.WriteLine($"{user.Name} {(user.IsAdmin ? "IS" : "is NOT")} an Admin.");
    }

            // But I cannot altger UserList2 in any way since it is properly marked as a IReadOnlyList.
            // You cannot compile the code below.
            //for (var i = 0; i < x.UserList2.Count; i++)
            //{
            //    x.UserList2[i] = new User() { Name = $"Evil {x.UserList1[2].Name}", IsAdmin = true };
            //}

    Console.WriteLine("\nUserList2 remains unchanged.");
    foreach (var user in x.UserList2)
    {
        Console.WriteLine($"{user.Name} {(user.IsAdmin ? "IS" : "is NOT")} an Admin.");
    }

    Console.WriteLine("\nPress ENTER key to close");
    Console.ReadLine();
}

Console output:

UserList1 has been modifed!
Evil Alice IS an Admin.
Evil Bob IS an Admin.
Evil Carl IS an Admin.
Evil Dan IS an Admin.
Evil Eve IS an Admin.

UserList2 remains unchanged.
Alice is NOT an Admin.
Bob is NOT an Admin.
Carl is NOT an Admin.
Dan is NOT an Admin.
Eve is NOT an Admin.

Press ENTER key to close
Explain what OP should change to make list readonly
Source Link
Rick Davin
  • 6.8k
  • 1
  • 20
  • 32

UPDATE READ ONLY LISTS

In the comments you say you cannot use a read-only list. I disagree. What I want to walk away with is that you may have objects internal to a class that allow certain things, but what you expose publicly to others should be more restrictive. This is particular true with something as sensitive as bank accounts.

With just a small change, you can have it both ways:

public class BankTransactionRepository : IBankTransactionRepository
{
    // Mock DB
    private List<BankTransaction> _transactions = new List<BankTransaction>();
    public IReadOnlyList<BankTransaction> BankTransactions => _transactions;

    public BankTransactionRepository()
    {
        _transactions = new List<BankTransaction>();
    }

    public void InsertTransaction(BankTransaction bankTransaction)
    {
        _transactions.Add(bankTransaction);
    }

    // more code 

}

Within the class, you would be interacting with object _transactions. But publicly you restrict what others can do with those transactions. The important thing is not the specific code, but rather the reasoning of why you want to do this.

Also, while I appreciate the speedy upvote from yesterday, I would suggest you not be too quick to accept an answer. Give it a day to see if others would chime in.

UPDATE READ ONLY LISTS

In the comments you say you cannot use a read-only list. I disagree. What I want to walk away with is that you may have objects internal to a class that allow certain things, but what you expose publicly to others should be more restrictive. This is particular true with something as sensitive as bank accounts.

With just a small change, you can have it both ways:

public class BankTransactionRepository : IBankTransactionRepository
{
    // Mock DB
    private List<BankTransaction> _transactions = new List<BankTransaction>();
    public IReadOnlyList<BankTransaction> BankTransactions => _transactions;

    public BankTransactionRepository()
    {
        _transactions = new List<BankTransaction>();
    }

    public void InsertTransaction(BankTransaction bankTransaction)
    {
        _transactions.Add(bankTransaction);
    }

    // more code 

}

Within the class, you would be interacting with object _transactions. But publicly you restrict what others can do with those transactions. The important thing is not the specific code, but rather the reasoning of why you want to do this.

Also, while I appreciate the speedy upvote from yesterday, I would suggest you not be too quick to accept an answer. Give it a day to see if others would chime in.

accidentatlly hit submit., continue editing
Source Link
Rick Davin
  • 6.8k
  • 1
  • 20
  • 32
Loading
Source Link
Rick Davin
  • 6.8k
  • 1
  • 20
  • 32
Loading