When using a C# property, should the get or the set portion of it massage the underlying data (if there is a need)?

Question

In the code base I was maintaining I found this exact class, pasted below. In logPath property, gets does some work. I would think that it is better to do the work in set - that way it will be done only once. However, partly because this is how the class is written, partly because it is an xml-mapped property, and partly because I am afraid that I might miss something in a debugger, I have doubts.

Additionally, if an element never existed in the xml, and it happened to be optional, then I think I will get a null for the value. I might actually want to differentiate between having no element and receiving empty value. I suppose I can have a private bool member which can help me detect that - that would be an argument for doing work in set rather than get. So, code optimizers work hard these days, so performance is rarely a true concern. It is more of a "figure this out once and do not think about it later" things. This is just one example, and properties frequently do some massaging.

Would you say that it is always better to do work in set? In get? It depends? A mixed style would not bother you a single bit as long as it works?

Thanks.

namespace MyNamespace
{
    using System;
    using System.Xml.Serialization;

    /// <summary>
    /// The LoggingListener class encapsulates the "logListener" 
    ///      element of config file, and puts the "logPath" 
    ///      attribute in a file path string.
    /// </summary>
    public class LoggingListener
    {
        private string logPathValue;

        /// <summary>
        /// Gets or sets the LOCAL file path to a log file 
        /// which will be written during operation of the Updater.
        /// </summary>
        [XmlAttribute("logPath")]
        public string LogPath
        {
            get
            {
                return this.logPathValue == null ? 
                         String.Empty : this.logPathValue;
            }

            set
            {
                this.logPathValue = value;
            }
        }
    }
}

EDIT: In this given sample ... if the log file is not there, then no logging should take place.

Answer 1

the only way to be sure that logPathValue is initialized is to do it yourself...

public class LoggingListener
{
    private string logPathValue = string.Empty;

Answer 2

I'd certainly prefer consistency. But the fact is in cases like this it often will not matter. I'm sure the original developer's intent was to avoid the infuriating NullReferenceException bug resulting from attempting to access the LogPath property of some LoggingListener object -- in which case, it probably just seemed most sensible to put the null check in the get (since that's where the exception was thrown).

In general, I'd agree with you that perhaps it makes the most sense to put it in the set -- but then, there's no guaranteeing LogPath will never return null, as the private member could have been set to null from within the class, or perhaps it was never set at all (as Kevin pointed out).

I tend to go with a somewhat hybrid approach: make the property read-only, and make sure it gets set to something non-null in the constructor:

public class LoggingListener {
    private readonly string _logPath;

    public LoggingListener(string logPath) {
        _logPath = logPath ?? string.Empty;
    }

    public string LogPath {
        get { return _logPath; }
    }
}

Whether this is an acceptable compromise obviously depends on your specific needs. Also, whether the property should really never be null after all is certainly debatable, depending on the scenario, as you've already remarked.

Answer 3

I personally don't like when property changes assigned value in either get or set accessor. It changes expected property behavior:

var value = null;
var listener = new LoggingListener();
listener.LogPath = value;
if(listener.LogPath != value)
{
    // how could we get here?
}

Instead, I prefer to clearly decide, whether property can accept null or not. If it can, it shouldn't do any work in get/set, if yes, it should neither return null nor accept it as a value.

If there is no way to prevent assignment of null, than i would prefer to handle this case in set accessor.

Answer 4

With getters and setters I follow a few rules:

1. No side effects - don't modify 'B' when you put a value in 'A'
2. What I put in 'A' comes out of 'A', if you need to modify 'A' expose a new read-only property
3. If you don't like my value for 'A', tell me now not later when I call a method
4. Outside of a data model, do not accept or return null

For your example I prefer seeing the following:

public class LoggingListener
{
    private string logPathValue = String.Empty;

    [XmlAttribute("logPath")]
    public string LogPath
    {
        get { return logPathValue; }
        set
        {
            if(value == null) throw new ArgumentNullException();
            this.logPathValue = value;
        }
    }
}

Yet it's clear to me why you ask, it really about the behavior of "XmlAttribute" and the XmlSerializer. You can use the "DefaultValueAttribute" from the ComponentModel namespace, or use an XSD to provide the defaults, or expose a new property and/or method. You could also try creating an interface to separate concerns, something like the following:

public class LoggingListener : ILogListenerSettings
{
    private string logPathValue;

    [XmlAttribute("logPath")]
    public string LogPath
    {
        get { return logPathValue; }
        set { logPathValue = value; }
    }

    string ILogListenerSettings.FullLogPath
    {
        get
        {
            string path = logPathValue;
            if(String.IsNullOrEmpty(path))
                path = Environment.CurrentDirectory;
            path = Path.GetFullPath(path);
            Directory.Create(path);
            return path;
        }
    }
}