Why single function accessor appears to be considered as bad habit?

Question

I see quite often (ie within the Slim framework) that single function accessor style (like in [1] below) is deprecated in favor to classical Java-ish 2 functions accessors (get/set) (like in [2] below). Personally I rather prefer less lines of code (in [1]) and less typing (get/set) than being able to chain setter calls like in [3] (which I consider awful).

Am I missing something?

 class Test {
    protected $body;

    // [1] single function accessor
    public function body($body = null)
    {
        if (!is_null($body)) 
        $this->body=$body;
        return $this->body;
    }

    // [2] dual function accessors
    public function getBody()
    {
        return $this->body;
    }

    // [2] dual function accessors
    public function setBody($body)
    {
        $this->body=$body;

        //[3] ok, you could return $this for chaining
    }
}

Answer 1

Are Single Function Accessors A Bad Habit?

They are not a good idea. The reasons are simple:

• They have multiple responsibilities (setting and getting data). Good functions have a single responsibility and do it well.

• They mask intent. You can't look at the method call and understand what's going to happen.

  What do you think the body() method does? Well, body is a noun. And having a method (which should be a verb) being a noun is confusing.

  But what if the method was age(). Age is both a verb and a noun. So when you see $obj->age(), are you telling the object to give you its age? Or are you telling the object to age itself?

  Whereas $obj->getAge() is crystal clear what you're trying to do. And $obj->getBody() is equally clear.

• It adds complexity to methods.

  Your overall complexity will stay the same (because the same logic exists), but it shifts to fewer methods. Which means that instead of having 2 dead simple methods, you have one more complicated method.

So yeah, I'd avoid it.

But let's step back a minute. Rather than asking the question if "Single Function Accessors" are a bad habit, let's ask the same question about accessors in general...

Are Property Accessors A Bad Habit?

My answer is: yes.

Depends on Role of Object:

What it depends on is the role of the object, and the role of the specific property. It's a case-by-case basis.

There are tons of different types of objects (domain objects, services, proxies, collections, etc). Some are stateful, and some are not.

If the object isn't stateful, it has no properties and hence we can ignore it.

For those objects that have state, why do they have that state?

If it's because they represent the state, then the state should be public (not saying the property should be public, but the state should be exposed to the outside world). So domain models which represent business entities should have public state.

class User {
    public $name;
}

But if the role of the object isn't to represent the state, but to do something with it, then it shouldn't be exposed.

class UserMapper {
    protected $pdo;
    public function __construct(Pdo $connection) {
        $this->pdo = $connection;
    }
    public function findUserById($id) {
        ...
}

In the mapper case, $pdo is incidental state. The mapper's job isn't to represent the data of the database connection, but it needs it to work.

The Bottom Line

The bottom line is never expose state that the object isn't directly representing.

Just because you have a property, doesn't mean you should expose it. In fact, often times you shouldn't expose it. This is known as Information Hiding.

Depends On Type Of State:

But what about types of objects that are stateful? Well, as it turns out there are two fundamental types of state:

1. Application State

   Think Configuration and things like that. Basically state that's not defined at build time, but is known at runtime.

   It's important to note that this state shouldn't change through the course of a request. And it also should be reasonably the same request-to-request (aside from deploys, etc).

2. User State

   Think state and data that's derived or dependent upon user input. An obvious example would be data that was submitted from a form.

   But a less obvious example would be if you use different renderers for different types of representations. So if a user requested a JSON response, the JSON rendering code being set in a property would be considered user state.

My assertion:

Property accessors for application state are bad. There's no reason that application state should change mid-run, therefore there's no reason that you should have to propagate the state.

Property accessors for user state may be OK. But there's more to it than that.

Depends On What The Accessor Does

In the case of your example:

public function setBody($body)
{
    $this->body=$body;
}

You're essentially making $body a public property.

And there's nothing wrong with that. But why do you need a method? What's wrong with: public $body; in the first place?

Some people will say "properties are evil, they must never be public!". And that's hogwash, since that's exactly what you did with that accessor.

Now, if the accessor did some typing information (through type hints), or other validation logic (minimum length, etc), then that's a different story...

Or is it?

Let me give an example.

class Person {
    public $name;
    public $age;
}

vs

class StrictPerson {
    protected $name;
    protected $age;

    public function setName($name) {
        if (!is_string($name)) throw new BlahException();
        if (strlen($name) < 10) throw new InvalidException();
        $this->name = $name;
    }
    public function getName() {
        return $this->name;
    }
    public function setAge($age) {
        if (!is_int($age)) throw new ....
        if ($age < 0 || $age > 150) throw new ...
        $this->age = $age;
    }
    public function getAge() {
        return $this->age;
    }
}

Now, it should be clearly obvious that those properties are always valid. Right? Right? Right?

Well, no. What would happen if I created a child:

class LoosePerson extends StrictPerson {
    public function setName($name) {
        $this->name = $name;
    }
    public function setAge($age) {
        $this->age = $age;
    }
}

All of a sudden, all of our validation disappears. Now you could argue that was intended and it's the programmer's problem. Or you could simply change the properties to be private instead, to hold them always being valid. Right? Right? Right?

Well, no. What would happen if I did this:

$a = new StrictPerson;
$r = new ReflectionProperty($a, 'name');
$r->setAccessible(true);
$r->setValue($a, 'Bob');

I've just set an invalid value onto an object that's supposed to always validate.

The Bottom Line

Using accessors as validators only works if you always use them. And every tool that you use always uses them. Things like mysqli and PDO and Doctrine and PHPUnit which set the property directly rather than calling setters can cause massive problems.

Instead, you can use an external validator:

class PersonValidator {

    public function validate(Person $person) {
        if (!is_string($person->name)) {
            throw new Blah...
        }
        if (strlen($person->name) < 10) {
            throw new Blah...
        }
        if (!is_int($age)) throw new ....
        if ($age < 0 || $age > 150) throw new ...
        return true;
    }
}

So, Are Property Accessors A Bad Habit?

I argue that yes, often times they are a bad habit.

Now, there are some cases where you should use them:

• When you must represent that state in an interface

  Properties are not specifiable in an interface. So if you must represent that the object exposes state in an interface, then you should use a getter and a setter.

  I'd strongly urge you to consider why you are creating the interface. For simple data objects, it's often better to rely on the core object type (since polymorphism can't benefit due to there being no logic on the data object).

• When you need to hide the internal representation for some reason.

  An example would be a class that represents a unicode string. You may have an accessor method to get the raw value. But you'd want that to be a method so that you could convert the internal representation to a string of the proper character set.

That brings up an interesting point.

When you create an accessor, you should never create a property accessor. You should instead create a state accessor. The difference is that property accessors must always return a property or set it).

A state accessor on the other hand can "fake it" if it needs to. So our example above about the unicode string class, could internally represent the string as an array of code points. Then when you access the "string" state, it will convert the array into an actual string.

Objects Should Abstract

If you're going to use an accessor, it should be to abstract state. Not to represent it.

The Bottom Line / TLDR

Property accessors are a bad habit. If you're going to do that, make the properties public and use a validator object.

State accessors are not a bad habit. They are quite useful for maintaining useful abstractions.

Answer 2

Pass all your objects dependencies in the constructor and just have getters to access them, no need for setters most of the time.