2

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?

  • 4
    It's only confusing if you the difference between references and objects isn't deeply ingrained in your brain. – user7043 Aug 9 '13 at 18:37
  • Make it an IEnumerable<int> instead. – Steven Evers Aug 9 '13 at 18:42
  • @delnan I know the difference, it's just more of a style question. It's confusing to put it as readonly as although the reference is readonly, the value is not. – It'sNotALie. Aug 9 '13 at 18:43
  • @SteveEvers Why? What if I need to use it as a list? ;) – It'sNotALie. Aug 9 '13 at 18:43
  • 2
    @SteveEvers The OP doesn't state that he isn't modifying the contents of the list after creation. There is only an indication that the reference to the list is not modified after creation. In fact, the OP state that the contents of the list are modified after creation ("I'm still modifying the list"), so he's thinking that having readonly there would be confusing. Apparently it has confused you, so that would be a point in the "probably shouldn't add readonly" column. – Servy Aug 9 '13 at 18:55
3

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

3

I say yes, add the readonly. For two reasons:

  • if you (or a teammate in the future) try to reassign to that variable in a later method, it will give you a compile-time error. R# will note the error even earlier. So, it's best thought of as an indicator and enforcer of intent. Even if you intend to reassign it later, it forces you to think about the design a little more and will not hurt much to remove it.

  • the compiler and/or JITter can use the single-assignment knowledge conveyed to potentially provide optimizations. Not guaranteed, but I do it for the first reason - this is a possible bonus.

  • How would you deal with the fact that marking it as readonly can confuse others into thinking that the list itself isn't modified? It's already been shown that it can happen. – Servy Aug 9 '13 at 19:09
  • The real answer is that C# should add what C++ folks call a "deep-constness" (or deep immutability) idiom. Or use the proper classes/interfaces, as the comment says. Ignorance of the framework isn't really an excuse, in my opinion. I get confused by some code I read; when that happens I research why it is what it is. Not rocket surgery. – Jesse C. Slicer Aug 9 '13 at 19:16
  • To a degree that addresses someone thinking, "Why is he adding an item to this list, it's read-only, how can he do that?" What it doesn't address is something like, "Oh, that list is ReadOnly, so I can write this method under the assumption that its contents will never change, rather than needing to take a copy of the list." The first forces the person to look into what readonly really means. The later results in the coder creating bad code without realizing that it doesn't work. That's something to be avoided. – Servy Aug 9 '13 at 19:18
  • 1
    My tendency is to prefer readonly, since I like my code to restrict me as much as possible. Now I just need to get into the habit of sealing classes. Of course, I primarily work on code that is compiled wholesale on any use, so if I change my mind later, removing readonly will not add much in the way of extra work. – Brian Aug 9 '13 at 21:26
1

You are getting the suggestion because you aren't changing the list itself, just the contents. If you reassigned the list entirely somewhere it would not be readonly, but because you are simply updating the contents the list itself doesn't change.

This is similar to how you can have a readonly object, but still change values on that object. The member variable does not change, but the object itself is still mutable.

  • I know. However, should I do it is the main point of my question. – It'sNotALie. Aug 9 '13 at 18:24
  • 1
    If you don't plan on adding a method that allows callers to set the list itself, then yes. It doesn't hurt anything and will cause an error if you do reassign it. You might want to read through the reasoning in http://programmers.stackexchange.com/questions/48413/ for a similar question in Java. – Cameron Aug 9 '13 at 18:29
  • It's similar, but doesn't answer my question as it's talking about parameters and locals. – It'sNotALie. Aug 9 '13 at 18:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.