views:

197

answers:

7

I am a little confused about how much I SHOULD do with properties. I have heard that properties should always represent a logical property of the class. Get and Set should almost never throw exceptions with the exception of ArgumentOutOfRange. Is that true? Is the following example totally wrong?

public bool DeviceRegistered
{
    get{ return _Registered;}
    set
    {
        if(value)
        {
            RegisterDevice();
            _Registered = true;
        }
        else
        {
            UnRegisterDevice();
            _Registered = false;
        }
    }
}

Also, If a method in the same class wants to change the value of a property should it go through the set accessor of the property or just modify the private variable _Registered directly?

If you have any additional advice when using properties please include! Thanks

+6  A: 

Here is a link to the Design Guidelines for properties from the MSDN. Make special note of the Property vs Method section.

From my own personal experience, you shouldn't use properties to do a lot of work. They should be returning information that has already been retrieved. I'm currently working on a code base which has a lot of lazy loaded properties that retrieve information from a web service. Looking at the properties of a class while debugging causes all of the properties to be evaluated, causing functional evaluation to time out and the ASP.NET process to crash.

Richard Nienaber
What would you recommend doing for lazy-loaded properties instead?
Greg
What we've settled on is using services to populate the properties. Most of the time, we've found the object was only used once per request anyway. If it is used more than once, the service stores it in a cache (e.g. Http.Items). This cache retrieval logic can also be abstracted away and be shared amongst services as needed.
Richard Nienaber
@Greg: Make the private backing field a `System.Lazy<T>`, `return _lazyValue.Value;` from the property, and mark the property as `[DebuggerBrowsable(DebuggerBrowsableState.Never)]`.
280Z28
@280Z28: .NET 4.0 only: http://msdn.microsoft.com/en-us/library/dd642331(VS.100).aspx
John Saunders
@John Saunders: You can grab it under MS-PL in the MEF source distribution (link 1), or you can use the better one in System.Threading.dll from the Reactive Extensions for .NET (link 2). 1) http://mef.codeplex.com/ 2) http://msdn.microsoft.com/en-us/devlabs/ee794896.aspx
280Z28
@280Z28: Thanks. It needed that qualification, since it's not in .NET 3.5.
John Saunders
A: 

In this case, since you call another method when the property is changed, if you want to keep that functionality then set it using the Accessor. If it's just storing a value, you are ever so slightly better off using the _Registered variable directly.

Shawn Steward
+1  A: 

A narrow answer: I like using read-only properties when I've got a value that takes a bit of work to get, but that I want the "rest of the world" (even callers inside the same class) to think of as a variable whose value is just always available. The property will do the work to get the value and then simply return (perhaps with optimizations like caching/etc).

The alternative would be a "get" method, and that's fine... but I like using a property when I don't want the caller burdened with the idea that there's work involved in fetching/figuring the value.

lance
+4  A: 

In this case I think it is more logical to use methods because you are performing an action.

private volatile bool _DeviceRegistered;
private readonly object _Lock = new object();

public bool DeviceRegistered
{
    get { return _DeviceRegistered; }
}

public void RegisterDevice()
{
    lock (_Lock) // A good idea to lock
    {
        // ........
        _DeviceRegistered = true;
    }
}

public void UnregisterDevice()
{
    lock (_Lock)
    {
        // ........
        _DeviceRegistered = false;
    }
}
ChaosPandion
You'd probably also want to lock in your get property accessor so that the "........" isn't in progress (possibly on a separate thread) when the property is read.
Jesse C. Slicer
@Jesse - The question is would we want to tie up other threads while registering the device? I think to be safe you are right, but it really depends on code we haven't seen.
ChaosPandion
A: 

I will grant you that often it makes sense to have a property do more than just hold a value, but in my opinion, your example is horrible. I would have a method that registers/unregisters the device and a simple getter for the current state. My general rule is that if I'm performing an action, I use a method, but if I'm merely changing a value, then a property is more appropriate. Now, how the property handles that may involve doing some computation or I/O, but the important thing is the expectation of the class consumer.

public void Register()
{
  ...do some stuff...
  this.Registered = true;
}

public void Unregister()
{
  ...do some stuff...
  this.Registered = false;
}

public bool Registered { get; private set; }

Also, I tend to force even the class code to go through the property -- this isolates any changes that I might make in how the property operates to the property code itself. Other parts of the same class need not know how the property works. There will be exceptions obviously -- like when you want or need to avoid some computation that the property performs, but still need the value to be updated -- but that's my general rule.

tvanfosson
A: 

To address the issue of the using the field directly or the property accessor/mutator, I am in favour of the property. If you should have a default value beeing returned in the accessor or raise property change events (or the like) in the mutator you will bypass this functionality by accessing the field directly. If an extending class overrides the property, you may inadvertantly bypass the extending class if you access the field directly.

There are cases where field access (private) is the way to go, but I always favour the property, unless there is a good reason to access the field.

MaLio
A: 

Here are some rules that I've realized over time. Most are my opinions but I like to think they are good ideas. :) Edit: I just realized that most of these are covered in the Property Usage guidelines.

Getters

  • You should prefer that getters not alter state. If you have a getter that does alter program state, mark it with [DebuggerBrowsable(DebuggerBrowsableState.Never)].
  • Prefer that getters can be called from any thread.
  • Prefer that getters be trivially computed, since they are used in such a way that leads people to believe they won't incur a performance penalty. If they could take some time to execute, mark them with either the DebuggerBrowsable attribute like above or with [DebuggerDisplay("Some display text")] (where the text is trivially computed). Forgetting the latter can have a detrimental impact on debugger performance.
  • Getters can throw at least the following exceptions:
    • InvalidOperationException
    • ObjectDisposedException

Setters

  • Prefer that whenever you set two properties back-to-back, it doesn't matter which comes first.
  • If the setter has a precondition that cannot be tested through publicly exposed properties, it should be marked protected or private.
  • It's ok to restrict calling setters to a particular thread, but if you do so it needs to be documented and your object should either implement ISynchronizeInvoke or expose a Dispatcher property.
  • Setters can throw at least the following exceptions:
    • An ArgumentException (ArgumentNullException, ArgumentOutOfRangeException, etc. as appropriate)
    • InvalidOperationException
    • ObjectDisposedException
280Z28