is it a bad programming practice to return a new object in a property

Question

I've always had this question and I think it is time to settle the answer, is it a heavy load to do this :

    public Rect Bounds
    {
        get 
        {
            var Top = (_targetPosition + _translationY) - _size / 2.0f;
            return new Rect(callOutStarMargin, Top, _size, _size); 
        }
    }

as an alternative of this :

    private Rect _boundsRect = new Rect();

    public Rect Bounds
    {
        get 
        {
            var Top = (_targetPosition + _translationY) - _size / 2.0f;
            _boundsRect.Y = Top;
            return _boundsRect;
        }
    }

doing this I'm avoiding to create a new Rect everytime the property is invoked, I'm not sure if doing this in the first way would create an extra memory/speed overweight, if lets say this property is accessed 1 or 2 times on a list of 100 items each second.

So which one should I choose ?

EDIT:

I think I'm not being clear here, my real question is How heavy is it making this new instructions of simple objects like Rect, How efficient is C# garbage collector for example in this scenario of 100 new rects per second , and the cpu cycle difference between one or the other way

Answer 1

Properties are syntactic sugar for approximating fields. They allow us to implement constraints or logic in our class while preserving the abstraction barrier of a POCO. The typical property in .NET does not change each time we "observe it". The semantics of auto-properties are clearly defined to use a compiler generated backing field, so properties should stick to those semantics rather than redefine them.

Creating a new instance each getter call is a mega-bug waiting to happen.

It is also needless expense to accomplish nothing but a smaller object. new() is certainly more expensive than a simple test and branch, especially if its a new reference type on the heap, so it isn't an optimization anyway.

The misleading aspect of the example is it makes this approach look not so dangerous, because of talk of "immutability", and the fact that we don't usually modify Bounds properties, or other struct fields.

There are three big problems with this approach:

public Something Something
{
    get 
    {
        return new Something();   // bad - bug waiting to happen
    }
}

1. It is inconsistent with real fields or typical .NET properties
2. It is a liability, needlessly allocating objects. A simple lambda across a collection of rectangles would generate pathological GC churn for large collections.
3. It is misleading and impossible to reason about without looking at the implementation. One should never have to look into the implementation of an intrinsic such as a field. Nobody will expect that we've redefined the semantics of a lazy property.

One would not expect the following to be an infinite loop:

while(obj.Something.X < max)
    obj.Something.X++;

Regarding performance and GC, the point isn't whether the GC is efficient. The point is the GC shouldn't even be in this conversation. Imagine your code was reused in the Bing search engine. (Not a fan of Bing, just referring to a high performance .NET system)

If there is a need for this, its best to use a method that clearly signifies that is what you are doing.

Answer 2

What you may want to keep in mind is that callers may assume that the value of a property does not change unless it has been explicitly assigned. This isn't necessarily a smart assumption, but nevertheless it is one that will be made.

Many programmers would expect the following test to pass:

var b1 = foo.Bounds;
var b2 = foo.Bounds;
Assert.AreEqual(b2, b1);

Now, for something like a Rect, this probably isn't a major problem, especially if it's immutable (as a Rect normally would be) and either a value type or has a value-equality operator. In this case, it really doesn't matter if two different calls get two different instances because it's very difficult to write code that implicitly depends on them being the same instance.

Also, lazy instantiation is almost always fine:

public Rect Bounds
{
    get
    {
        if (bounds == null)
            bounds = GetBounds(...);
        return bounds;
    }
}

...because subsequent calls will get the same instance, barring any multi-threading problems.

But things can get hairy when a property actually returns a different instance every time, and that instance is a fairly complex and especially mutable object.

For example, let's say you return a Stream:

public Stream OutputStream
{
    get { return new FileStream(...); }
}

This is fairly horrible because someone's very likely to write code like:

foo.OutputStream.Write(data1, 0, data1.Length);
foo.OutputStream.Write(data2, 0, data2.Length);

And depending on exactly how you've implemented OutputStream, one of two nasty things will happen, either there will be a file-locking exception on the second call, if OutputStream always references the same file, or the caller might unintentionally write data to two different files (if, let's say, OutputStream actually creates a new file each time), subsequently corrupting both files.

Don't expect callers to save the return value of a property. They often won't. So if you're considering writing a property that returns a different object each time, make sure that there aren't any serious risks to having two instances floating around at the same time and possibly being swapped or lost.

I've spent many a painful hour debugging code where it looked like everything was working correctly but it turned out I was only working on a copy of the correct instance. Iterators with deferred execution are an especially pernicious subset of this problem; it's not uncommon to foreach through it once and change some property of each element, then foreach through it again and find that everything has "reset"!

One last thing - you should never new up an object that requires cleanup from a property accessor. There's almost no chance at all that the caller will clean it up.

Answer 3

It's not uncommon, or bad, to return new objects from properties (as immutable types do this all the time).

Ultimately, it's not an issue until you've measured it and found it to be an issue.

Answer 4

There are three things you need to consider when deciding if a property should send a new object or a reference to a maintained object, and each of these will vary by language and runtime.

1. What will avoid inappropriate errors. If you pass a reference to a private object, it's theoretically possible that downstream consumers of your class may hit an error when your object is deleted or updated and they still have a reference. (If this isn't an issue on your platform, don't worry about it.)

2. What will avoid unacceptable performance. As with the above, you may get a performance hit by creating new objects on each reference instead of passing the same object around. This can vary very substantially even within the same language: early Java implementations had expensive object-creation, while modern Java creates and destroys objects with far better performance.

3. What will those who come after you expect. If neither of the above are an issue, you should follow whatever is traditionally done in your project. It'd be aggravating to expect a stored reference to your object's Top object to update when it doesn't, or vice-versa.

Personally, I grew up on JavaScript, and so prefer object references that persist themselves, and primitives that don't. But that's just me, and the languages I do most of my work in now have considerably different behavior.

Answer 5

The simple test I would suggest for whether it should be acceptable for a property (vs a method) to a new object in a given situation is whether callers would be expected to care whether they receive a new object or a pre-existing one that happened to have the right characteristics. If callers would expect to receive a new object, then the code in question should be a method. If callers would expect to receive a pre-existing object, then the code should supply one. Code which may return a new object should only be a property if callers will not particularly expect to receive a new object, but won't mind if they do get one.

Generally, there are two situations where callers will have such non-expectation:

1. The first call will lazily create the object which will be returned by that and all future calls, and nothing about the object will indicate when it was actually created. Further, code should ensure that there's no way two calls could return different objects, even if they are performed simultaneously by different threads [use locking or CompareExchange to ensure this].

2. The object being constructed is immutable, and callers are expected to care only about the data contained therein, rather than its identity. Strings are a prime example of this.

For performance reasons, any properties which return objects that are expensive to create should cache the most-recently-created object to ensure that consecutive repeated calls which should report the same state can reuse the same object; it may sometimes be useful to cache additional objects in cases where a property's state might revert to an earlier value, but the performance benefit of doing that in different situations may range anywhere from "worse than nothing" to "drastic". Except when caching would be impractical, however, the question of whether to use a property or method should be primarily determined by semantics.

Answer 6

You should return a new object. Always.

The overhead is minimal in all modern languages. But even if it weren't, reusing an existing static object is tantamount to returning the value in a global variable. And that will lead to BAD THINGS.

Even if program is not threaded/concurrent, its very feasible that a function (or different functions in a call chain) will want to examine the bounds of multiple rectangles, logically at the same time. If you have just the one global object, you will get very wrong answers. If your program is at all multi-threaded, things will get even worse. Either way, errors will happen and it will be confusing to figure out why a simple "what are the bounds of this?" call does not seem to have a temporally stable, consistent set of values.