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?

It'sNotALie.
  • 205
  • 1
  • 8
  • 4
    It's only confusing if you the difference between references and objects isn't deeply ingrained in your brain. –  Aug 09 '13 at 18:37
  • Make it an IEnumerable instead. – Steven Evers Aug 09 '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 09 '13 at 18:43
  • @SteveEvers Why? What if I need to use it as a list? ;) – It'sNotALie. Aug 09 '13 at 18:43
  • The functional difference between an IEnumerable and List is the ability to add and remove items (or, more specifically that IEnumerable is immutable). Since the question indicates that the collection isn't modified after creation (only its contents) then IEnumerable is a more fitting collection type. – Steven Evers Aug 09 '13 at 18:51
  • @SteveEvers How would it be? IEnumerable has no insertion method. – It'sNotALie. Aug 09 '13 at 18:53
  • 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 Aug 09 '13 at 18:55

3 Answers3

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

Community
  • 1
Servy
  • 1,966
  • 14
  • 12
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.

Jesse C. Slicer
  • 6,002
  • 2
  • 32
  • 44
  • 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](http://programmers.stackexchange.com/questions/207782/should-i-make-a-variable-readonly-when-i-modify-it-but-i-dont-actually-set-it#comment407835_207782) that it can happen. – Servy Aug 09 '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 09 '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 09 '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 09 '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.

Cameron
  • 226
  • 1
  • 3
  • I know. However, **should I do it** is the main point of my question. – It'sNotALie. Aug 09 '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/](http://programmers.stackexchange.com/questions/48413/) for a similar question in Java. – Cameron Aug 09 '13 at 18:29
  • It's similar, but doesn't answer my question as it's talking about parameters and locals. – It'sNotALie. Aug 09 '13 at 18:44