Is it true I should not do "long running" things in a property accessor?

Question

And if so, why? and what constitutes "long running"?

Doing magic in a property accessor seems like my prerogative as a class designer. I always thought that is why the designers of C# put those things in there - so I could do what I want.

Of course it's good practice to minimize surprises for users of a class, and so embedding truly long running things - eg, a 10-minute monte carlo analysis - in a method makes sense.

But suppose a prop accessor requires a db read. I already have the db connection open. Would db access code be "acceptable", within the normal expectations, in a property accessor?

Answer 1

A db read in a property accessor would be fine - thats actually the whole point of lazy-loading. I think the most important thing would be to document it well so that users of the class understand that there might be a performance hit when accessing that property.

Answer 2

I don't see what the problem is with that, as long as you provide XML documentation so that the Intellisense notifies the object's consumer of what they're getting themselves into.

I think this is one of those situations where there is no one right answer. My motto is "Saying always is almost always wrong." You should do what makes the most sense in any given situation without regard to broad generalizations.

Answer 3

You can do whatever you want, but you should keep the consumers of your API in mind. Accessors and mutators (getters and setters) are expected to be very light weight. With that expectation, developers consuming your API might make frequent and chatty calls to these properties. If you are consuming external resources in your implementation, there might be an unexpected bottleneck.

For consistency sake, it's good to stick with convention for public APIs. If your implementations will be exclusively private, then there's probably no harm (other than an inconsistent approach to solving problems privately versus publicly).

Answer 4

It is just a "good practice" not to make property accessors taking long time to execute. That's because properties looks like fields for the caller and hence caller (a user of your API that is) usually assumes there is nothing more than just a "return smth;"

If you really need some "action" behind the scenes, consider creating a method for that...

Answer 5

Like you mentioned, it's a surprise for the user of the class. People are used to being able to do things like this with properties (contrived example follows:)

foreach (var item in bunchOfItems)
    foreach (var slot in someCollection)
        slot.Value = item.Value;

This looks very natural, but if item.Value actually is hitting the database every time you access it, it would be a minor disaster, and should be written in a fashion equivalent to this:

foreach (var item in bunchOfItems)
{
   var temp = item.Value;
   foreach (var slot in someCollection)
      slot.Value = temp;
}

Please help steer people using your code away from hidden dangers like this, and put slow things in methods so people know that they're slow.

There are some exceptions, of course. Lazy-loading is fine as long as the lazy load isn't going to take some insanely long amount of time, and sometimes making things properties is really useful for reflection- and data-binding-related reasons, so maybe you'll want to bend this rule. But there's not much sense in violating the convention and violating people's expectations without some specific reason for doing so.

Answer 6

A database access in a property getter is fine, but try to limit the amount of times the database is hit through caching the value.

There are many times that people use properties in loops without thinking about the performance, so you have to anticipate this use. Programmers don't always store the value of a property when they are going to use it many times.

Cache the value returned from the database in a private variable, if it is feasible for this piece of data. This way the accesses are usually very quick.

Answer 7

In addition to the good answers already posted, I'll add that the debugger automatically displays the values of properties when you inspect an instance of a class. Do you really want to be debugging your code and have database fetches happening in the debugger every time you inspect your class? Be nice to the future maintainers of your code and don't do that.

Also, this question is extensively discussed in the Framework Design Guidelines; consider picking up a copy.

Answer 8

This isn't directly related to your question, but have you considered going with a load once approach in combination with a refresh parameter?

class Example
    {
        private bool userNameLoaded = false;
        private string userName = "";
        public string UserName(bool refresh)
        {
            userNameLoaded = !refresh;   
            return UserName();
        }
        public string UserName()
        {
            if (!userNameLoaded)
            {
                /*
                userName=SomeDBMethod();
                */
                userNameLoaded = true;                    
            }
            return userName;         
        }
    }