I'm not sure if this is the best place to post, but it's the most like an "Architecture" forum, so I'll try here first.
In short, I'm looking for opinions about whether or not a certain result of calling a method is 'ok' or a 'bad thing'.
And I fully accept that the only reason i have such a problem with this is that I got so thoroughly burnt by it.
From a purely OO perspective, should a call to a getter method ever change the state of the object it is getting?
Example: if I have two methods - void setList(EList list); EList getList(String listname);
Further: the EList object has an internal pointer (sorry for the C/C++ term), that points to the 'current' position within the list.
Is it acceptable for getList() to move the pointer to the top of the list?I realize this is a rather strained example, but it's basically about right. I think you get the idea...
1. A purely architectural/analytical approach might tell me that a getter method should never change the state of the object or 'thing' it is getting.
2. A practical, perhaps "accepted" or "best practice" way of doing things. ie: What makes a programmer's job easier. Kind of like "well sure, a db should be in 4NF, but in usage, sometimes it's as low as 2NF."
How about it? [ February 13, 2002: Message edited by: Mike Curwen ]
Ah yes, in the old days of Smalltalk this question was called "should getters have side effects?" Here's a much less contrived example: It's somewhat common to use a technique called lazy initialization to avoid default values for variables until they are absolutely necessary. For instance, consider the following. Let's say I've got an Order that has a Vector called LineItems. Let's further consider that it has variables for Customer, etc. There might be many cases in which I want to create or retrieve and Order but never look at its line items. However, if my Order class set the lineItems variable to be an empty Vector in either the constructor or in the class definition, then there would always be a Vector created whether I needed it or not. Instead, you can do the following in your getter:
Now, some folks would say "but it's not worth the runtime check for the if statement every time you call the getter just to avoid creating the Vector!" Well, sometimes it is and sometimes it isn't. It's a classic speed/space tradeoff. So, what's my opinion on the subject? Personally, I follow the principle of least astonishment. I only allow side effects that could be resonably expected -- like initializing null variables to a reasonable default. On the other hand, I would consider resetting a counter to NOT be reasonably expected. Kyle
Just juding from the behavior of something similar in the JDK, collections. If you get an Iterator or Enumerator, it's always set to the top of the collection. In your case, I think I'd might refactor your list class to follow this behavior and use an Iterator on your list vise an internal pointer. This will aviod ugly concurrency issues if two threads get a hold of the same list.
Hum... IMHO, A basic run of the mill getter, for the most part, no it should modify the data. If you have a business need to modify the data and return the result, the method used should have a different signiture. getRestartedEList() That helps avoid confusion and unwanted/unexpected side effects later.
Thanks for the replies... I'll add a bit more now.
Let's assume that MOST uses of the getList() call would be to initially retrieve the list. We need to retrieve the list before we cycle over it, right? And further assume that the list was created much earlier and placed into a 'holding space'. Much like an attribute space on an HttpRequest (as an example).
What this implies, is that it has not 'been used' beforehand, and so one would naturally expect the list to be 'at the beginning' anyways. So now we have this situation. The extra 'true' parameter is for the parameter named 'reset'. This particular call means "please give me the list, and reset it to the top". But what a pain in the bum, right? Of *course* I want the top of the list.. so shouldn't 'true' be the default value for reset? Seems resonable.
And this is how I got burnt... I didn't realize the API had the two methods. I thought there was only getList(String name), and didn't realize that calling that one, was actually like calling getList(String name, boolean reset) with reset being defaulted to 'true'.
This is what I was doing, and I admit it looks odd, but believe me when I say, I'm under the hood of something, and for the sake of making *small* changes, I have to do it this way.So you see the infinite loop, right? My call to getList() on the "INNER" part resets it back to the top, because the default for the reset parameter is true. Remember it's pulling the same list from that nebulous 'attribute space'. If we were to watch the internal list pointer, it would be like a tennis match.
Kyle expressed my thoughts pretty well. "HOLY SIDE-EFFECT BATMAN!"
But then there's this: Kyle also said:
I only allow side effects that could be resonably expected -- like initializing null variables to a reasonable default
Or... like I stated at the top of this post.. it's "reasonable" to assume a list is at the top of the list in 95% of the cases you'd call getList(). And so defaulting this method to reset the list is not a bad thing. Because who wants to type all of those trues, when MY particular example can be avoided by simply sending in 'false'. But on the other hand.. if 95% of cases are for retrieving a list "for the first time"... wouldn't it be at the top anyways? And what about the programming chestnut of NEVER assuming something that is critical? If you want to ENSURE the pointer is at the top, you should code it.
And Carl, I agree about the iterator! But there isn't one for this EList class I'm forced to use.
Thanks everyone for your replies. [ February 13, 2002: Message edited by: Mike Curwen ]
posted 17 years ago
Any kind of list like that should use an iterator to go through it. It is about 100 times easier to understand and you will never run into confusion about side effects.
If the side effects are unavoidable, document it as part of the contract. Otherwise, do defensive copying or depend on immutability. In your case, make a copy of the list, then return it! [ February 14, 2002: Message edited by: Pho Tek ]
For me, this issue really boils down to one of naming. The reason you got "burned" was because you (not unreasonably) assumed that the the method followed the normal semantics of a generic getXXX() method, and thus had no side effects and so on. I still favor the idea of having two methods: getResetElist(name) and getContinuedElist(name) (or whatever). This would have neatly avoided the problem of assuming the semantics of a generically-named method. Even if you had only ever seen the most common method. The concept that I apply is that expressing intent in method and member names is [b]far[/i] more efficient both at run-time and at compile-time than any sort of defensive programming, parameter-copying, assertions, test cases etc.. The cost of a few extra characters of typing here and there is massively swamped by even a short spell of puzzlement or bug-tracking. Don't use generic names when you can use specific names, and especially don't ever flout the assumed semantics of well-known names like getXXX(), setXXX(), iterator() and so on.