C# – Should Variables Be Readonly if Modified Only in Constructor?

ccoding-style

Background:

I was writing some code. It looked something like this:

class X
{
    private List<int> _myList = new List<int>();
    public void MyMethod(int x)
    {
        _myList.Add(x);
    }
}

R# suggested that I make _myList readonly. I wasn't so sure, as though I technically could make it readonly, I think it would be slightly confusing to do so, as I'm still modifying the list, just not reassigning it. So while I could make it readonly, I'm not sure if I should.

Question:

Should I make variables like this readonly? Why? If not, why does R# suggest this?

Best Answer

If you think it's confusing, as you've said, then don't do it. If other members of your team that are likely going to need to work with this code don't have the difference between references and objects sufficiently ingrained in their brains then don't do it to avoid confusing them. If you all understand the concepts so effectively that this isn't confusing at all, and you think it might benefit you to convey that the reference to the list isn't changed, then mark it as read only.

Keep in mind that your team doesn't always need to do the same thing as everyone else on the planet. Do what works best for the people who will actually need to touch this code (with some basic considerations for people who will need to touch the code that aren't yet on the team.)

Related Solutions

C# – const vs. readonly for a Singleton

Given your code, I have two concerns.

First, readonly references to mutable child objects (like collections) do not make the properties of the child immutable.

public class Foo
{
   public readonly List<int> Bar = new List<int>{1,2,3};
} 

...

var foo = new Foo();

//You may enumerate Bar and read its indices
foreach(int baz in foo.Bar) Console.WriteLine(baz.ToString());

var bob = foo.Bar[1];

//this is illegal because the reference is readonly
foo.Bar = new List<int>{4,5,6};

//but the properties of the immutable reference are still mutable, so this is fine:
foo.Bar.Clear();
foo.Bar.Add(4);
foo.Bar.Add(5);
foo.Bar.Add(6);

If you want a truly immutable child object, you have to make the reference immutable AND make sure the object itself is immutable. For that, you typically want to create or use a read-only version of your child and return that instead:

public class Foo
{
   private readonly List<int> bar = new List<int>{1,2,3};

   public ReadOnlyCollection<int> Bar { get { return bar.AsReadOnly(); } }
} 

...

var foo = new Foo();

//You may still enumerate and index Bar
foreach(int baz in foo.Bar) Console.WriteLine(baz.ToString());

var bob = foo.Bar[1];

//this is illegal because there's no setter
foo.Bar = new List<int>{4,5,6};

//And these are also illegal because ReadOnlyCollection doesn't expose any such methods:
foo.Bar.Clear();
foo.Bar.Add(4);
foo.Bar.Add(5);
foo.Bar.Add(6);

Second, the locked, double-checked instantiation of the singleton is (much) better than nothing for making it thread-safe, but there are still some cases where this can fail to prevent double-instantiation. Microsoft instead recommends use of a static readonly field with instantiation defined:

public sealed class MySingl
{
   public static readonly MySingl Instance = new MySingl();

   private MySingl() { ... }
}

Microsoft guarantees an implementation like this to be thread-safe in the Microsoft CLR; statics are instantiated just-in-time and the CLR has internal mechanisms to block other threads that need this reference while it's being created.

Method Flags as Arguments vs Member Variables – Best Coding Practices

On the other hand, this member variable isn't really representing any object state (as it's neither set nor available outside), but is really a hidden additional argument for the "inner" private method.

That reinforces my gut feeling upon reading your question: try to keep this data as local as possible, i.e. don't add it to the object state. This reduces the state space of your class, making your code simpler to understand, test and maintain. Not to mention that avoiding shared state makes your class more threadsafe too - this may or may not be a concern to you right now, but it may make a huge difference in the future.

Of course, if you need to pass around such flags excessively, it may become a nuisance. In this case, you may consider

creating parameter objects to group related parameters together into a single object, and/or
encapsulating this state and the associated logic into a separate family of strategies. The viability of this depends on the language: in Java, you need to define a distinct interface and a (possibly anonymous) implementation class per each strategy, whereas in C++ or C# you may use function pointers, delegates or lambdas, which simplifies the code considerably.