Why does resharper suggest readonly fields

Question

Why is ReSharper suggesting readonly field for 'settings' in my example below?

If I understand correctly, you should use readonly modifier if you change this field only in constructor, but in my example I also change it in another method in the same class.

What am I missing?

public partial class OptionsForm : Form
{
    private Settings settings;

    public OptionsForm(Settings s)
    {
        settings = s;
    }

    private void SaveData()
    {
        settings.ProjectName = TextBoxProject.Text;
    }
}

Tonguing

Ok, and how to disable that? I couldn't find in options... – Compellation

Answer 1

When a reference type is declared as readonly, the pointer is immutable, but not the object it points to. This means that:

• a reference type data member can be initialized in order to point to an instance of a class, but once this is done it's not possible to make it point to another instance of a class outside of constructors
• the readonly modifier has no effect on the object the readonly data member points to.

Read a detailed article on this

Mark a C# class data member as readonly when it’s read only

Zorn

How does this answer the question: "Why is ReSharper suggesting readonly field for 'settings' in my example below?" ? – Dacosta

The link mentioned in the above reply is not working. I think it is moved to developer-corner.com/2009/01/… – Doodlesack

I agree with @zvolkov. Is it explained in the link? If so, please paraphrase the link here. – Cryotherapy

"settings" IS read only. You never say "settings = someOtherSettings" or "settings = new Settings()". "ProjectName" is NOT "settings". "settings" is the POINTER that points to some OBJECT in memory. "ProjectName" is a property of that OBJECT, NOT of the POINTER "settings". – Maledict

Answer 2

Remember the primary reason for coding standards and design patterns is to make to easier for people to understand your code.

By marking a field as “readonly” you are telling the reader of the class that they don’t need to consider how the value of the field is changed.

However as the object the readonly field points to can have it’s state change, marking a field as readonly can be misleading at times. So think about weather it helps the reader (e.g a person) of your code understand your design or not.

If the values in the object that the field points to changes within the object lifetime, then I don’t think a field should be marked read-only. (E.g the object pointed to should be behave as if it is immutable by the time the contractor on your class is called)

(However there are some exceptions, e.g it is OK to use a read-only field to point to a logger even thought the logger does change the state of the log file.)

Cocci

So you are suggesting that in my case 'readonly' modifier would just confuse the reader? – Tonguing

@simon, yes, as the values in settings get changed, I would not mark it readonly. – Cocci

I don't think that's a good interpretation. The readonly modifier makes the reference immutable, it has specific uses; what your blockquote says isn't right at all. Also, why have you decided logger is an acceptable special-case? – Deanadeanda

@nicodemus13, as you can't read the log files from ILogger, you can't see that it's state has changed, therefore do you care that it state may have changed. – Cocci

Maybe my 'isn't right at all', isn't right at all :) @ ian: thanks. – Deanadeanda

Answer 3

ReSharper suggests making "settings" readonly:

readonly private Settings settings;

public OptionsForm(Settings s)
{
    settings = s;
}

because, upon scanning your code it concludes that your "settings" field only occurs in the constructor for this same class.

If howerver you were to provide a partial class or some other code in this class that modified "settings", it would no longer suggest it was readonly.

Once marked readonly the compiler will flag as warnings various misuses of the field, such as:

The left-hand side of an assignment must be an l-value

which is the same error you get when you attempt to assign a value to a constant.

also the use of ref and out parameter modifiers are limited.

By following ReSharpers suggestion you will be warned by the compiler if you attempt to misuse a field that you really intend not to change after initialization.

Condensable

Answer 4

You aren't changing settings outside of the constructor, the object is the same one in SaveData. The object properties might be changing but not the object reference, so it seems to make sense from a Resharper perspective.

Badillo

Answer 5

The SaveData() method does not change the settings variable, it changes one its properties. The contents of settings (what it refers to) is only set in the constructor.

Surd

Answer 6

Actually, you're right and Resharper is wrong. A field should only be marked readonly if it is immutable in its entirety. In your example if you make it readonly and enable Microsoft's Code Analysis it will warn you that Settings has mutable properties.

Dacosta

Not true. I suggest you look at msdn.microsoft.com/en-us/library/acdd6hb7%28VS.71%29.aspx – Millepede

Blowdart, your MSDN link defines the behavior of the keyword/compiler without going into specifics of value types vs. reference types, nor considering the SEMANTICS of the keyword. Now, the keyword works perfectly fine for value types (and immutable reference types) but has interesting implications for mutable reference types. Let's say the field is a hashtable. Will flagging the field as readonly make the hashtable immutable? No! You can't change the pointer to point to another hashtable but you STILL can change the data. That's why you shouldn't use it on mutable reference types. – Dacosta

Answer 7

This seems to be a bit strange, I can't imagine that Eric Lippert et al, didn't consider the obvious fact that making a reference immutable doesn't make instance pointed to by the reference immutable, though the mentioned Code Analysis rule does support the view of those above (http://msdn.microsoft.com/en-us/library/ms182302(v=VS.100).aspx).

It still doesn't really make any sense. If mutable instances should not be readonly, then, in my view, it should be a compile-time error, seems rather pointless otherwise.

I can see the use of making a reference immutable, rather than the instance it points to.

Deanadeanda