Independent getter/setter methods, or combined? [closed]

Question

While working on a project, I've been making some changes and browsing around existing framework API docs for insight.

While perusing the Kohana docs, I noticed that the getters/setters of any given class are typically combined:

public function someProperty($value = null){
    if(is_null($value){
        return $this->_someProperty;
    }
    $this->_someProperty = $value;
    return $this;
}

Rather than:

public function setSomeProperty($value){
    $this->_someProperty = $value;
    return $this;
}

public function getSomeProperty(){
    return $this->_someProperty;
}

Is there any value in doing this (the former), beyond lessening the method count of a given class? I was always under the understanding that methods (functions in general) should be more descriptive of an action. Do other experienced developers cringe, even a tiny bit, when they see this?

I was just surprised to see a popular framework use such conventions (I haven't used Kohana of course)

Burka

notice that this is used extensively in javascript frameworks, maybe it comes from there – Symon

Yea @Gabi, strangely though, it doesn't bother me when doing interface development (I favor jQuery) – Burka

maybe you don't rely that much on autocomplete with javascript – Symon

Code of Kohana is far from ideal. Not only because of setters/getters. – Cachinnate

@Gabi - Likely, NetBeans is pretty solid for jQuery dev anyways. @OZ_ - I've begun noticing this; their Request::factory() method is a bit of a beast, trying to handle initial and subsequent request creation calls simultaneously. Seems like a good place for method extraction. – Burka

Even when combining setter and getter into one method, in this case, it's being done wrong (IMHO). Rather than checking is_null($value), they should check if func_num_args() === 0. – Elsewhere

Absolutely @binaryLV, could not agree more, as it solves the issue @faileN brought up of setting null. If I ever find myself going the Kohana-esque route, I'll certainly use your suggestion as a check over is_null or alternatives (I think they do a type check === NULL) – Burka

Good question. Though Kohana is using this for its internal libraries, you may use the getters and setters in your models as you wish to. This perfectly works for Kohana internal libraries but it is not meant to be used in your code either. I wouldn't call that a coding standard for Kohana. There is a coding standard page somewhere but there isn't a word about this case. – Precursory

@Precursory - Yea, again I just found it strange that a seemingly popular web framework would go such a route. The standards that Zend communicates are very effective, and I've used quite extensively their conventions (though not entirely) as they seem to be effective and comprehensible, both as a writer of code and a reader. – Burka

the PHPDOC block for such a method must look like crap! – Eringo

Answer 1

I consider this bad practise because it violates CommandQuerySeparation. Setting a value is changing state (Command). Getting a value is asking for state (Query). A method should not do both, but one thing only.

Also, it's not really obvious what a method does when it's just called username, e.g. does not have a verb, like get or set. This gets even worse in your example, because the return value is either the object itself or the property value, so its not consistent.

Moreover, getters (and setters) should be used sparingly as they will quickly convolute your API. The more getters and setters you have, the more knowledge about an object is required by collaborators of that object. If you find your objects asking other objects about their internals, chances are you misplaced the responsibilities.

Rondarondeau

Fair point well made about return value. Although PHP provides possibility to return anything (int, float, bool, string, object, null etc), each and every method should return value of only one type. In some cases null can be an exception, i.e., method might return "either int or null" or "either object or null". – Elsewhere

@binary in case of "object or null", it is often better to use use a NullObject. – Rondarondeau

matter of taste. As stated in wiki, "this pattern should be used carefully as it can make errors/bugs appear as normal program execution." It also may complicate design a bit. Anyway, in both cases (both NULL and NullObject), return values of methods/functions have to be consistent. – Elsewhere

@Binary that comment in the wiki can easily be ignored when using a SpecialCase, which is a variant of the NullObject :) – Rondarondeau

@Rondarondeau For the most part, I agree with your statement, "Moreover, getters (and setters) should be used sparingly as they will quickly convolute your API". However, my issue is consistency. Suppose you decide you don't want to use getters/setters and just use public properties. But then you have that ONE case where you need a getter/setter, so you create one. Now you've got a situation where it would seem more confusing to the person using the API -- instead of knowing they can call GetWhatever() and SetWhatever($val), they have to keep track of some things as publics and some as gets/sets. – Driest

@Driest when I said you should use getters and setters sparingly, I didnt mean to say you should have public properties instead. What I meant (and that's explained in the linked article as well) is that you should use Tell Dont Ask instead. – Rondarondeau

This is not really about CQS. Every call to a combined getter/setter is either a query or a command but not both. Parameter-less calls are referentially transparent. – Cancel

Answer 2

jQuery goes the same way as Kohana. However I think it's better to create separate methods for setting and getting. It's more obvious what the method does and I think it's more practically in code-completition in your ide. For example you type set and you get a list of all Properties you can set.

Another disadvantage is: what if you want to set a value really to null? This wouldn't work since the null is the identifier for returnin the value, you are restricted in setting specific values...

So it's nice, since you'll have to write less, but hey what are three letters (set/get) in front of your methods?

Kasper

Thanks @faileN - All three of your points hit my concerns. Method chaining autocompletion gets clouded by possible arbitrary return values, the null factor, and the self description. Again, odd how a seemingly popular framework opts for such conventions. – Burka

Second point can be fixed by checking if func_num_args() === 0. That said, I agree that setters and getters should be implemented as different methods. – Elsewhere

@binaryLV. You're right with the checking. Didn't think about that :) Thanks – Kasper

Answer 3

Despite the fact that Kohana uses such unusual technique for the OOP, I think you should follow coding conventions at first. But of course it's better to use separate getters and setters for every property in your classes. So, if it's possible to use them not breaking the conventions - just do it and you won't be wrong ;) . You can also read here about good habits in PHP OOP - http://www.ibm.com/developerworks/opensource/library/os-php-7oohabits/ if you've doubted about using some OOP technics. Hope that it'll help :)

Merrifield

Thanks @olezhek, I've developed conventions over time that don't stray far from the beaten track of PHP pseudo-standards like Zend, etc. – Burka

Answer 4

I'd rather believe they had a reasonable explanation for doing it this way. For example, for easier implementation of ArrayAccess. Only way to know for sure is to ask them directly.

To answer your question, yes I cringe when I see the first method. Goes against OOP principles.

Orthodoxy

Which principle does it violate? – Brickbat

I'm not making the connection in your example, between ArrayAccess and combined getter/setters. If you offer both a getter and setter, call them accordingly in your implemented wrapper methods, no? – Burka

Principles - keep methods simple(1 method/1 action), readability of code; ArrayAccess - tbh I remembered interface differently when I wrote it, it's actually easier to implement with standart get/set, haha. My point was that maybe it was to easier implement something like it, not ArrayAccess in particular. – Orthodoxy

Yea @Inori, but I can't think of any interfaces/interface-methods that would benefit from a combined getter/setter. Also, the principal you mention is sort of defeated when you combine a getter and setter. Regardless, I'm glad you cringe too; I sort of sat there furrowing my brow as I browsed the Kohana API docs. – Burka

Answer 5

Why not do it like this?

public function someProperty($value = null)
{
    if (func_num_args() === 1) {
        $this->someProperty = $value;
        return $this;
    } else {
        return $this->someProperty;
    }
}

This would imo be the only correct way to implement a combined getter/setter

Jacquejacquelin

+1, this is certainly a way to implement it; however, I've long since accepted that this is a bad practice anyway. – Burka

True, i agree with you on the fact it is a bad practise. Separate getters and setters is always the better way to go. – Jacquejacquelin

Answer 6

If you do it everywhere it is a good way, but than it really needs to be for everything, maybe the programmers of this framework are used to is, (it's a bit jquery alike)

However it would confuse me

For setting and getting I always use setters and getters:

   public function __set($key, $value) {
      // assign value $value to $this->key
   }

   public function __get($key) {
      // return value of this->key
   }

Kidder

Thanks @gar_onn - I used to use magic, but have shifted away from it in favor of chain-ability (with setters of course) as well as to micro-optimize. – Burka

And you always write names of fields? Horrible. Try autocompletion in IDE. – Cachinnate

If you just set $this->key in setters and read it in getters without any validation etc, there's actually no point in using __get() and __set(), you can set any non-existent property without them. It is quite wrong though (just like using __get() and __set() for setting arbitrary properties). – Elsewhere

Answer 7

For the sake of argument,

The combined approach does offer some benefits:

1. Avoids __get and __set magic while still emulating a public property. (I would not ever recommend using magic for these situations anyway)
2. Using thing() is less verbose than using getThing() setThing().
3. Even though the methods will be doing more, they can still be considered as "doing one thing()", that is handling the thing(). Properties also do more than one thing. They allow you to set and get values.
4. It's argued that the thing() doesn't give a verb. However, we can assume that a thing() without a verb means that we use it like a property (we get and set it). For interfaces, we can say that a thing() with no argument is readonly and a thing($arg) with an argument is read/write. Why should we be shy from adopting this? At some point we adopted the idea of adding getters and setters didn't we?
5. If you are using a web-based language (like PHP), then chances are you might be using jQuery as well. JQuery is already doing this sort of thing() and it has worked out well.
6. Using func_num_args(), as mentioned already, helps achieve this approach perfectly.

Personally, I've already taken a good portion of risks in my current apps at this point so I'm probably going with the old tried-and-true getters and setters (see the section "Finding the Balance" of Jimmy Bogard's post in regards to getters/setters for data operations). And I suppose we are already trained to look for these get/set prefixes (as well as our IDE's) to see what properties we can work with in a class. This is a discussion I would be open to returning to at some point.

Orangery