24

I have a class called Heading that does a few things, but it should also be a able to return the opposite of the current heading value, which finally has to be used via creating a new instance of the Heading class itself.

I can have a simple property called reciprocal to return the opposite heading of the current value and then manually create a new instance of the Heading class, or I can create a method like createReciprocalHeading() to automatically create a new instance of the Heading class and return it to the user.

However, one of my colleagues recommended me to just create a class property called reciprocal that returns a new instance of the class itself via its getter method.

My question is: Isn't it an anti-pattern for a property of a class to behave like that?

I particularly find this less intuitive because:

  1. In my mind, a property of a class should not return a new instance of a class, and
  2. The name of the property, which is reciprocal, doesn't help the developer to fully understand its behaviour, without getting help from the IDE or checking the getter signature.

Am I being too strict about what a class property should do or is it a valid concern? I have always tried to manage the state of the class via its fields and properties and its behaviour via its methods, and I fail to see how this fits to the definition of the class property.

  • 6
    As @Ewan says in the last paragraph of his answer, far from being an anti-pattern, having Heading as an immutable type and reciprocal returning a new Heading is "pit of success" good practice. (with the caveat at the two calls to reciprocal should return "the same thing", ie they should pass the equality test.) – David Arno Nov 8 '16 at 9:08
  • 20
    "my class is not immutable". There's your real anti-pattern, right there. ;) – David Arno Nov 8 '16 at 9:16
  • 3
    @DavidArno Well, sometimes there is a huge performance penalty for making certain things immutable, especially if the language doesn't support it natively, but otherwise I agree that immutability is a good practice in a lot of cases. – 53777A Nov 8 '16 at 9:20
  • 2
    @dcorking the class is implemented both in C# and TypeScript but I rather keep this language-agnostic. – 53777A Nov 8 '16 at 13:34
  • 2
    @Kaiserludi sounds like an answer (a great answer) - comments are for requesting clarity – dcorking Nov 8 '16 at 15:05

10 Answers 10

22

Its not unknown to have things like Copy() or Clone() but yes I think you are right to be worried about this one.

take for example:

h2 = h1.reciprocal
h2.Name = "hello world"
h1.reciprocal.Name = ?

It would be nice to have some warning that the property was a new object each time.

you might also assume that:

h2.reciprocal == h1

However. If your heading class was an immutable value type then you would be able to implement these relationships and reciprocal might be a good name for the operation

  • 1
    Just note that Copy() and Clone() are methods, not properties. I believe the OP is referring to property getters returning new instances. – Lynn Nov 8 '16 at 11:48
  • 6
    i know. but properties are (sort of) also methods in c# – Ewan Nov 8 '16 at 11:51
  • 4
    In that case, C# has much more to offer: String.Substring(), Array.Reverse(), Int32.GetHashCode(), etc. – Lynn Nov 8 '16 at 14:21
  • If your heading class was an immutable value type - I think you should add that property setters on immutable objects should return the new instances always (or at least if the new value is not the same as the old value), because that's the definition of immutable objects. :) – Ivan Kolmychek Nov 8 '16 at 14:33
17

An interface of a class leads the user of the class to make assumptions about how it works.

If many of these assumptions are right and few are wrong, the interface is good.

If many of these assumptions are wrong and few are right, the interface is rubbish.

A common assumption about Properties is that calling the get function is cheap. Another common assumption about properties is that calling the get function twice in a row will return the same thing.


You can work around this by using consistency, in order to change expectations. For example, for a small 3D library where you need Vector, Ray Matrix, etc you can make it such that getters such as Vector.normal and Matrix.inverse are pretty much always expensive. Unfortunately even if your interfaces consistently use expensive properties, the interfaces will at best be as intuitive as one that uses Vector.create_normal() and Matrix.create_inverse() - yet I know of no strong argument that can be made that using properties creates a more intuitive interface, even after changing expectations.

10

Properties should return very quickly, return the same value with repeated calls, and getting their value should have no side effects. What you describe here should not be implemented as a property.

Your intuition is broadly correct. A factory method does not return the same value with repeated calls. It returns a new instance each time. This pretty much disqualifies it as a property. It's also not fast if later development adds weight to the instance creation, e.g. network dependencies.

Properties should generally be very simple operations that get/set a part of the current state of the class. Factory-like operations don't meet this criteria.

  • 1
    The DateTime.Now property is commonly used and refers to a new object. I think the name of a property can help remove the ambiguity about whether or not the same object is returned each time. It's all in the name and how it's used. – Greg Burghardt Nov 8 '16 at 18:06
  • 3
    DateTime.Now is regarded as a mistake. See stackoverflow.com/questions/5437972/… – Brad Thomas Nov 8 '16 at 19:19
  • @GregBurghardt The side effect of a new object might not be an issue in and of itself, because DateTime is a value type, and has no concept of object identity. If I understand correctly, the issue is that it's non-deterministic and returning a new value each time. – Zev Spitz Nov 9 '16 at 6:43
  • 1
    @GregBurghardt DateTime.New is static, and therefore you can't expect it to represent a state of an object. – Tsahi Asher Nov 9 '16 at 8:50
5

I don't think there's a language-agnostic answer to this since what constitutes a “property” is a language-specific question, and what a caller of a “property” expects is a language-specific question as well. I do think the most fruitful way to think about this is to think about what it looks like from the point of view of the caller.

In C#, properties are distinctive in that they are (conventionally) capitalized (like methods) but lack parentheses (like public instance variables). If you see the following code, absent documentation, what do you expect?

var reciprocalHeading = myHeading.Reciprocal;

As a relative C# novice, but one who’s read Microsoft’s Property Usage Guidelines, I would expect Reciprocal to, among other things:

  1. be a logical data member of the Heading class
  2. be inexpensive to call, such that there’s no need for me to cache the value
  3. lack observable side effects
  4. produce the same result if called twice in succession
  5. (maybe) offer a ReciprocalChanged event

Of these assumptions, (3) and (4) are probably correct (assuming Heading is an immutable value type, as in Ewan's answer), (1) is debatable, (2) is unknown but also debatable, and (5) is unlikely to make semantic sense (though whatever has a heading should perhaps have a HeadingChanged event). This suggests to me that in a C# API, “get or calculate the reciprocal” should not be implemented as a property, but especially if the calculation’s cheap and Heading is immutable, it’s a borderline case.

(Note, though, that none of these concerns have anything to do with whether calling the property creates a new instance, not even (2). Creating objects in the CLR, in and of itself, is pretty cheap.)

In Java, properties are a method naming convention. If I see

Heading reciprocalHeading = myHeading.getReciprocal();

my expectations are similar to those above (if less explicitly set out): I expect the call to be cheap, idempotent, and lacking in side effects. However, outside the JavaBeans framework the concept of a “property” is not all that meaningful in Java, and particularly when considering an immutable property with no corresponding setReciprocal(), the getXXX() convention is now somewhat old-fashioned. From Effective Java, second edition (already more than eight years old now):

Methods that return a non-boolean function or attribute of the object on which they’re invoked are usually named with a noun, noun phrase, or a verb phrase beginning with the verb get …. There is a vocal contingent that claims that only the third form (beginning with get) is acceptable, but there is little basis for this claim. The first two forms usually lead to more readable code… (p. 239)

In a contemporary, more fluent API, then, I would expect to see

Heading reciprocalHeading = myHeading.reciprocal();

-- which would again suggest that the call is cheap, idempotent, and lacks side effects, but would say nothing about whether a new calculation is performed or a new object is created. This is fine; in a good API, I shouldn’t care.

In Ruby, there's no such thing as a property. There are “attributes”, but if I see

reciprocalHeading = my_heading.reciprocal

I have no immediate way of knowing whether I’m accessing an instance variable @reciprocal via an attr_reader or a simple accessor method, or whether I’m calling a method that performs an expensive calculation. The fact that the method name is a simple noun, though, rather than say calcReciprocal, suggests, again, that the call is at least cheap and probably doesn’t have side effects.

In Scala, the naming convention is that methods with side effects take parentheses and methods without them don’t, but

val reciprocal = heading.reciprocal

could be any of:

// immutable public value initialized at creation time
val reciprocal: Heading = … 

// immutable public value initialized on first use
lazy val reciprocal: Heading = … 

// public method, probably recalculating on each invocation
def reciprocal: Heading = …

// as above, with parentheses that, by convention, the caller
// should only omit if they know the method has no side effects
def reciprocal(): Heading = …

(Note that Scala allows various things that are nonetheless discouraged by the style guide. This is one of my major annoyances with Scala.)

The lack of parentheses tells me the call doesn’t have side effects; the name, again, suggests that the call should be relatively cheap. Beyond that, I don’t care how it gets me the value.

In short: Know the language you’re using, and know what expectations other programmers will bring to your API. Everything else is an implementation detail.

  • 1
    +1 one of my favourite answers, thanks for taking your time to writing this up. – 53777A Nov 9 '16 at 8:31
2

As others have said, it is a rather common pattern to return instances of the same class.

The naming should go hand in hand with the language's naming conventions.

For example, in Java I'd probably expect it to be called getReciprocal();

That being said, I'd consider mutable vs immutable objects.

With immutable ones, things are pretty easy, and it won't hurt if you return the same object or not.

With mutable ones, this can get very scary.

b = a.reciprocal
a += 1
b = what?

Now what is b referring to? The reciprocal of the original value of a? Or the reciprocal of the changed one? This might be a too short of an example, but you get the point.

In those cases look for a better naming which communicates what happens, for example createReciprocal() might be a better choice.

But it really depends on the context as well.

  • Did you mean mutable? – Carsten S Nov 8 '16 at 18:00
  • @CarstenS Yes, of course, thanks. No idea where my head was. :) – Eiko Nov 8 '16 at 18:03
  • Disagree somewhat on getReciprocal(), since that "sounds" like a "normal" JavaBeans style getter. Prefer "createXXX()" or possibly "calculateXXX()" which at indicate or hint to other programmers that something different is happening – user949300 Nov 9 '16 at 23:05
  • @user949300 I agree somewhat with your concerns - and mentioned the create... name further down. There are downsides, too, though. create... implies new instances which might not always be the case (think of singular dedicated objects for some special cases), calculate... sort of leaks implementation details - which might encourage wrong assumptions on the price tag. – Eiko Nov 10 '16 at 15:20
1

In the interests of single-responsiblity and clarity I would have a ReverseHeadingFactory that took the object and returned its reverse. This would make it clearer that a new object was being returned and would mean the code to produce the reverse was encapsualated away from other code.

  • 1
    +1 for the name clarity, but what is wrong with SRP in other solutions? – 53777A Nov 8 '16 at 10:25
  • 3
    Why do you recommend a factory class over a factory method? (In my opinion, a useful responsibility of an object is often to emit objects that are like itself, such as iterator.next. Does this mild violation of SRP cause other software engineering problems?) – dcorking Nov 8 '16 at 12:53
  • 2
    @dcorking A factory class versus method (in my opinion) is usually the same type of question as "Is the object comparable, or should I make a comparator?" It's always okay to make a comparator, but only okay to make them comparable if it's a pretty universal way of comparing them. (E.g., Bigger numbers are greater than smaller ones, this is good for comparable, but you could say all evens are greater than all odds, I would make that into a comparator) -- So for this, I would think there is only one way to reverse a header, so I would lean towards making a method to avoid an extra class. – Captain Man Nov 8 '16 at 15:18
0

It depends. I usually expect properties to return things that are part of an instance, so returning a different instance of the same class would be a bit unusual.

But for example a string class could have properties "firstWord", "lastWord", or if it handles Unicode "firstLetter", "lastLetter" which would be full string objects - just usually smaller ones.

0

Since the other answers cover the Property part, I'll just address your #2 about reciprocal:

Do not use anything other than reciprocal. It is the only correct term for what you're describing (and it is the formal term). Do not write incorrect software to save your developers from learning the domain they're working in. In navigation, terms have very specific meanings and using something as seemingly innocuous as reverse or opposite could lead to confusion in certain contexts.

0

Logically, no, it is not a property of a heading. If it were, you could also say that negative of a number is property of that number, and frankly that would make "property" lose all meaning, almost everything would be a property.

Reciprocal is a pure function of a heading, same as negative of a number is a pure function of that number.

As a rule of thumb, if setting it does not make sense , it probably shouldn't be a property. Setting it can still be disallowed of course, but adding a setter should be theoretically possible and meaningful.

Now, in some languages, it might make sense to have it as a property as specified in the context of that language anyway, if it brings some advantages due to mechanics of that language. For example, if you want to store it to a database, it's probably sensible to make it as property if it allows automatiic handling by ORM framework. Or maybe it is a language where there is no distinction between a property and a parameterless member function (I don't know such a language, but I'm sure there are some). Then it is up to the API designer and documenter to distinguish between functions and properties.


Edit: For C# specifically, I'd look at existing classes, especially those of Standard Library.

  • There are BigInteger and Complex structures. But they are structures, and have only static functions, and all properties are of different type. So not much design help there for you Heading class.
  • Math.NET Numerics has Vector class, which has very few properties, and seems to be pretty close to your Heading. If you go by this, then you'd have a function for reciprocal, not a property.

You might want to look at libraries actually used by your code, or existing similar classes. Try to stay consistent, that's usually the best, if one way isn't clearly right and another way wrong.

-1

Very interesting question indeed. I see no SOLID+DRY+KISS violation in what you're proposing, but, still, it smells bad.

A method that returns an instance of the class is called a constructor, right? so you're creating a new instance from a non-constructor method. It's not the end of the world, but as a client, I wouldn't expect that. This is usually done in specific circumstances: a factory or, sometimes, a static method of the same class (useful for singletons and for... not-so-object-oriented programmers?)

Plus: what happens if you invoke getReciprocal() on the returned object? you obtain another instance of the class, that could very much be an exact copy of the first one! And if you invoke getReciprocal() on THAT one? Objects sprawling anyone?

Again: what if the invoker needs the reciprocal value (as scalar, I mean)? getReciprocal()->getValue()? we are violating Law of Demeter for little gains.

  • I'd gladly accept counter-arguments form the downvoter, thanks! – Dr. Gianluigi Zane Zanettini Nov 8 '16 at 16:59
  • 1
    I didn't downvote but I suspect the reason might be the fact that it doesn't really add much to the rest of the answers, but I might be wrong. – 53777A Nov 8 '16 at 17:23
  • 2
    SOLID and KISS are often opposites of each other. – whatsisname Nov 8 '16 at 18:04

protected by gnat Nov 9 '16 at 19:00

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.