tags:

views:

678

answers:

14

Let's have an object created in a getter like this :

public class Class1
{
       public string Id { get; set; }
       public string Oz { get; set; }
       public string Poznamka { get; set; }

       public Object object
       {
             get
             {
                  // maybe some more code
                  return new Object { Id = Id, poznamla = Poznamka, Oz = OZ };
             }
        }
 }

Or should I rather create a Method that will create and return the object ?

+20  A: 

Yes, it is bad practice.

Ideally, a getter should not be changing or creating anything (aside from lazy loading, and even then I think it leads to less clear code...). That way you minimise the risk of unintentional side effects.

Mitch Wheat
So what they are good for ?
getting a value! ....
Mitch Wheat
can use a field
Properties allow you to return arbitary values, for instance, pass a call to Count to a variable in the class (return m_Collection.Count, for example)
thecoop
Validating the setter. For example a specific range. And Lazy evaluation.
Dykam
They can also be declared abstract, virtual, or be part of an interface. You can't do that with fields.
thecoop
@Mitch:What about encapsulation? Let's say you have a class Rectangle with method getPointTopLeft() which returns a object of class Point to represent the top left corner. But you don't want this to reference the internal state of the rectangle? Would it be bad to clone the Point, or instantiate a new Point with the internal data as constructor arguments?
fireeyedboy
Would the downvoter please leave a comment. Thanks.
Mitch Wheat
@mitch Sorry, I misread the question and downvoted. While I was writing the comment I realized I was an idiot and went to remove my downvote. But guess what? The window for changing a vote is now like 30 seconds or something. So I had to edit your answer to be able to undo my mistake. My bad, sorry about that, I'm a dolt.
Will
I thought he was just providing a bad example for a getter that news up an object rather than a property that acts like a factory. I don't think new-ing up objects in getters is bad unless the newing-up process is expected to take a decent amount of cycles (e.g., the object creates a Thread in its constructor) or may result in an exception being thrown.
Will
Lazy loading can be a minefield when you're stepping through the `get` implementation in the debugger. Hint: turn off debugger evaluation of properties!
Daniel Earwicker
Would the downvoter please leave a comment. Thanks
Mitch Wheat
+8  A: 

If you want your getter to create a new object every time it is accessed, that's the way to do it. This pattern is normally refered to as a Factory Method.

However, this is not normally needed on properties (ie. getters and setters), and as such is considered bad practice.

Oded
That would mean you get a different object each time you accessed the getter; surely a bad idea!
Mitch Wheat
Hm, when I read this, it looks like you are saying that using the Factory Method pattern is bad practice. To my opinion, that's not really true. However, using the factory method pattern for getters and setters is a bad practice.
Fortega
@Mitch - I agree. Unless that's exactly what you need...
Oded
IMO a factory method should be named "createFoobar" to make very clear that a new Foobar object is created. On the other hand, a lazy loading getter that creates the object on the first call is, IMO, perfectly legal. I expect getters to return the same object on subsequent calls; a lazy loading getter does that.
ammoQ
A: 

It's maybe at most acceptable for structs. For reference types, I would only create a new object in a getter when it's only done once using some lazy-load pattern.

herzmeister der welten
A: 

It depends on the use of the getter. It's a great place to include this kind of code for lazy loading.

Joel Etherton
+2  A: 

A property should, to all intents and purposes, act like a field. That means no exceptions should be thrown, and no new objects should be created (so you don't create lots of unneccessary objects if the property is used in a loop)

Use a wrapper class or similar instead.

thecoop
A: 

It is a bad practice. In your example, you should be able to expect the same Object every time you access the object property.

Austin Salonen
A: 

As you have it it is bad but not dis similar to an acceptable practice called lazy loading which can be read about here.

http://www.aspcode.net/Lazy-loading-of-structures-in-C-howto-part-8.aspx

runrunraygun
A: 

It is a bad practice. But if you are thinking of objects as a bunch of getters & setters you should check the classical discussions about the topic.

As some folks mentioned, lazy loading could be a reason to do so. Depends on the actual business logic you are modeling here. You should create a separate method if it is better for legibility purposes, but if the code to create the object is simple you could avoid the indirection.

JuanZe
+3  A: 

yes, it is ... from the outside, it should be transparent, whether you access a property or a field ...

when reading twice from field, or a property, you expect two things:

  • there is no impact on the object's (external) behaviour
  • you get identical results

I have no real knowledge of C#, but I hope, the following makes my point clear. let's start like this:

Object o1 = myInst.object;
Object o2 = myInst.object;
o1.poznamka = "some note";

in the case of a field, conditions like the following will be true:

o1 == o2;
o2.poznamka == "some note";

if you use a property with a getter, that returns a new object every time called, both conditions will be false ...

your getter seems to be meant to produce a temporary snapshot of your instance ... if that is what you want to do, than make it a plain method ... it avoids any ambiguities ...

greetz

back2dos

back2dos
+2  A: 

According to me if something is 'property' the getter should return you a property (basically a data that is already existing) relevant to the object.

In your case, you are returning something that is not a property of that object at that moment. You are not returning a property of your object but a product of some action.

I would go with a method something like GetMyObject() instead. Especially if there is an 'action' will take place, I think it is most of the time best to have a method than a property name.

And try to imagine what would other developers who are not familiar with your code expect after seeing your property.

burak ozdogan
+1  A: 

A property is just a convenient way to express a calculated field.

It should still represent something about an object, regardless of how the value itself is arrived at. For example, if the object in question is an invoice, you might have to add up the cost of each line item, and return the total.

What's written in the question breaks that rule, because returning a copy of the object isn't something that describes the object. If the return value changes between calls to the property without an explicit change of object state, then the object model is broken.

Speaking in generalities, returning a new object like this will just about always break the rule (I can't think of a counter-example right now), so I would say that it's bad practice.

There's also the gotcha of properties where you can so easily and innocently call on a property multiple times and end up running the same code (which hopefully isn't slow!).

Jon Seigel
+8  A: 

Properties look like fields but they are methods. This has been known to cause a phenomenal amount of confusion. When a programmer sees code that appears to be accessing a field, there are many assumptions that the programmer makes that may not be true for a property.So there are some common properties design guidelines.

  1. Avoid returing different values from the property getter. If called multiple times in a row, a property method may return a different value each time; a field returns the same value each time.

  2. A property method may require additional memory or return a reference to something that is not actually part of the object's state, so modifying the returned object has no effect on the original object; querying a field always returns a reference to an object that is guaranteed to be part of the original object's state. Working with a property that returns a copy can be very confusing to developers, and this characteristic is frequently not documented.

  3. Consider that a property cannot be passed as an out or ref parameter to a method; a field can.

  4. Avoid long running property getters. A property method can take a long time to execute; field access always completes immediately.

  5. Avoid throwing exceptions from getters

  6. Do preserve previous values if a property setter throws an exception

  7. Avoid observable side effects.

  8. Allow properties to be set in any order even if this results in a temporary invalid state of objects.

Sourses

"CLR via C#", Jeffrey Richter. Chapter 9. Defining Properties Intelligently

"Framework Design Guidelines" 2nd edition, Brad Abrams, Krzysztof Cwalina, Chapter 5.2 Property Design

Sergey Teplyakov
+1  A: 

For writing code that is easily tested, you have to maintain separation of Object initialization.

i.e while in test cases you do not have hold on test some specific items.

like in House object you dont want to test anything related to kitchen object. and you wana test only the garden. so while you initiate a house class and initiate object in some constructors or in getters you wont be coding good that will support testing.

Usama Khalil
+1  A: 

As an aside to the comments already made, you can run into some real debugging headaches when lazy loading fields via a property.

I had a class with

private Collection<int> moo;

public Collection<int> Moo
{
  get 
  {
    if (this.moo == null) this.moo = new Collection<int>();
    return this.moo;
  }
}

Then somewhere else in the class there was a public method that referenced

this.moo.Add(baa);

without checking it was instantiated.

It threw a null reference exception, as expected. But the exception was on a UI thread so not immediately obvious where it was coming from. I started tracing through, and everytime I traced through, the error dissapeared.

For a while I have to admit I thought I was going crazy. Debugger - no error. Runtime, error. Much scratching of head later I spotted the error, and realised that the Visual Studio debugger was instantiating the Collection as it displayed the public properties of the class.

PaulG