Proper Coding Direct Access to the backing field of a Property C#

Question

I have seen some code and thought that something seems wrong with it, so I would like to know if it is acceptable for good coding or not, my first thought is no.

Consider:

class MyClass
{
    private string m_MySuperString;
    public string MySuperString
    {
        get { return m_MySuperString; }
        set { m_MySuperString = value; }
    }

    public void MyMethod()
    {
        if (blah != yada)
        {
             m_MySuperString = badabing;
        }
    }

    public void MyOtherMethod()
    {
        if (blah == yada)
        {
            m_MySuperString = badaboom;
        }
    }
}

Is this kind of direct access to the Backing Field an acceptable practice or is it bad coding - or should I ask what is the point of the Property Accessor, and if this is done internally in a class with public members , access is allowed by multiple components - is it possible to have a crash - I would venture in a multi threaded application a crash should be expected.

Please any thoughts ? I have looked at this Link on SO and others> Why use private members then use public properties to set them?

EDIT

Let me be clear since there is good info being provided and rather respond to all answers and comments directly. I am not asking about what properties are for, not if I can do auto implemented properties, private setters, OnValueChange notifications, logic on the properties. My question is in regards to accessing that backing field directly - for example if you have say a mutlithreaded scenario - isn't the whole point of the synclock on the getters/setters - to control access to the backingfield ? Will this kind of code be acceptable in that scenario - just adding a syncLock to the getter and setter ?? Keep in mind the code in the constructor of myClass is an example - the code can be in any additional method - such as the updated class - Method1

END EDIT

Answer 1

Properties in Object Oriented Programming (OOP) help to enforce Encapsulation. The idea is that only the object itself is allowed to interact with its own data (i.e. fields). Access to the object's data from outside is only allowed through methods. In Java, for instance, you have to explicitly write get- and set-methods. Properties in C# have a special syntax that combines both of these methods in one construct, but the getters and setters are in fact methods.

This also means that an object is absolutely allowed to access its own fields directly.

There are cases, however, where property getters and setters perform additional logic. A setter might raise the PropertyChanged event or do some validation. A getter might combine several fields or yield a formatted or calculated value. If you need this additional logic to be performed, then you must access the properties instead of the fields. If a property is auto-implemented, then you have no choice (in C#), since the backing field is hidden and not accessible. (In VB it is hidden from IntelliSense but accessible from within the class.)

Answer 2

I recommend checking out Chapter 8, Section 1 of @JonSkeet's C# In Depth (from which I've shamelessly taken the below snippets for the purpose of education) for more information on automatically implemented properties. In short answer to your question, no, there's nothing wrong with this code.

Consider that the following snippet:

public string Name { get; set; }

is compiled as

private string <Name>k__BackingField;
public string Name
{
     get { return <Name>k__BackingField; }
     set { <Name>k__BackingField = value; }
}

...so the compiler is already doing the work for you that you've done above. There are ways to modify what it's doing, but those don't really answer the question. One example given in the book for thread safety is this:

//code above, plus
private static int InstanceCounter { get; set; }
private static readonly object counterLock = new object();

public InstanceCountingPerson(string name, int age) {
    Name = name;
    Age = age;
    lock (counterLock) // safe property access
    {
        InstanceCounter++;
        // and whatever else you have to do with the lock enabled
    }
}

--Which is a pattern also referenced in this SO question. However, as pointed out there, locks are (a) potentially slow, (b) might not actually ensure their job is done because they have to be released at some point, and (c) rely on the trust system, because they sort of naively assume that anything wanting to access that object will make proper use of the lock (not always true, at least not in some of the code I've seen :D ). The advantage of the getter and setter methods is that you can enforce the pattern of using the lock (read: properly encapsulate the field, as others have suggested) for any instance of your class.

Another pattern you might consider, however, is Inversion of Control. With a Dependency Injection container, you can specify the level of thread safety you are comfortable with. If you are comfortable with everyone waiting for a singleton instance of your object, you can declare that all references to the object's interface reference the same object (and must wait for the object to become available), or you can determine that a thread-safe instance of the object should be created each time it is requested. See this SO answer for more details.

Note:

Any peer-reviewed criticism of the above ideas will be graciously accepted and added to the answer, as I'm sort of a thread safety dilettante at this point.

Answer 3

In the use case described , You can define this as follows using auto-implemented properties

public string MySuperString{ get;  set ;}

you should use a backing filed if you need to do some input verification or the property is different than the internal fields for example

public string FullName{ get { return firstName + LastName} }

another benefit of using properties is you can define them in an interface , which is better in the long run for future features to be added