4

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?

Improve this question
7
  • 4
    It's only confusing if you the difference between references and objects isn't deeply ingrained in your brain.
    – user7043
    Commented Aug 9, 2013 at 18:37
  • Make it an IEnumerable<int> instead.
    – Steven Evers
    Commented Aug 9, 2013 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.
    Commented Aug 9, 2013 at 18:43
  • @SteveEvers Why? What if I need to use it as a list? ;)
    – It'sNotALie.
    Commented Aug 9, 2013 at 18:43
  • 3
    @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
    Commented Aug 9, 2013 at 18:55

3 Answers 3

Reset to default
4

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

Improve this answer
0
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.

Improve this answer
4
  • 1
    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
    Commented Aug 9, 2013 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
    Commented Aug 9, 2013 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
    Commented Aug 9, 2013 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
    Commented Aug 9, 2013 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.

Improve this answer
3
  • I know. However, should I do it is the main point of my question.
    – It'sNotALie.
    Commented Aug 9, 2013 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
    Commented Aug 9, 2013 at 18:29
  • It's similar, but doesn't answer my question as it's talking about parameters and locals.
    – It'sNotALie.
    Commented Aug 9, 2013 at 18:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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