No getter/setter on model with many instance vars

Question

While reading Object calisthenics and Getters/Setters. Evil. Period. I got confused. Let me explain it by example.

I have this organization class, representing the information about organization, the classic entity:

class Organization
{
    private $name;

    private $city;

    private $street;

    private $phone;

    private $email;

    public function __construct($name, $city, $street, $phone, $email)
    {
        $this->name = $name;
        $this->city = $city;
        $this->street = $street;
        $this->phone = $phone;
        $this->email = $email;
    }

    public function getName()
    {
        ....
    }

    public function getCity()
    {
        ....
    }

    public function setCity($newCity)
    {
        ....
    }

    public function getStreet()
    {
        ....
    }

    public function setStreet($newStreet)
    {
        ....
    }

    public function getPhone()
    {
        ....
    }

    public function getEmail()
    {
        ....
    }

    public function setPhone($newPhoneNumber)
    {
        ....
    }
}

The organization name, city, street, phone and email may change, so the setters looks kind a logical. Getters are used to to display the properties of organization in table. How can I remove the boilerplate code and break trough from this anemic model? Maybe this is the case where the getter/setter is unavoidable?

Also object initialization using __construct. Is it a good practice to pass so many instance variables to constructor?

I gave it a try, according to what I read. Created some ValueObject to store information and do validation (don't know if the ValueObject should do a validation on its data):

class Phone
{
    private $phone;

    public function __construct($phone)
    {
        if (!$this->validate($phone)) {
            throw new InvalidPhoneNumberException();
        }

        $this->phone = $phone;
    }

    public function getPhone()
    {
        return $this->phone;
    }

    private function validate($phone)
    {
        // validate the phone number
    }
}

class Email
{
    private $email;

    public function __construct($email)
    {
        if (!$this->validate($email)) {
            throw new InvalidEmailAddressException();
        }

        $this->email = $email;
    }

    public function getEmail()
    {
        return $this->email;
    }

    private function validate($email)
    {
        // validate email
    }
}


class Address
{
    private $city;

    private $street;

    public function __construct($city, $street)
    {
        $this->city= $city;
        $this->street = $street;
    }

    public function getCity()
    {
        return $this->city;
    }

    public function getStreet()
    {
        return $this->street;
    }
}

Then passed these objects instead of string representations of data to my Organization and did some renaming on methods:

class Organization
{
    private $name;

    private $address;

    private $phone;

    private $email;

    public function __construct($name, Address $address, Phone $phone, Email $email)
    {
        $this->name = $name;
        $this->address = $address;
        $this->phone = $phone;
        $this->email = $email;
    }

    public function getName()
    {
        return $this->name;
    }

    public function getAddress()
    {
        return $this->address;
    }

    public function getPhone()
    {
        return $this->phone;
    }

    public function getEmail()
    {
        return $this->email;
    }

    public function changeAddress(Address $newAddress)
    {
        $this->address = $newAddress;
    }

    public function changePhoneNumber(Phone $newPhoneNumber)
    {
        $this->phone = $newPhoneNumber;
    }
}

Does this code smells less and looks more mature from the perspective of OOP?

Answer 1

Getters and setters are not evil, period.

Mindlessly setting and getting fields of an object is bad, though. Especially with this syntax (setX, getX).

Simple example showing the difference between mindless and thoughtful setter :

# Mindless
class CartesianPoint {
    private $x;
    private $y;

    public function setX($x) {
        $this->x = $x;
    }

    public function setY($y) {
        $this->y = $y;
    }
}

# Thoughtful
class CartesianPoint {
    private $x;
    private $y;

    public function plot($x, $y) {
        $this->x = $x;
        $this->y = $y;
    }
}

Can you spot the differences ? You don't want to be able to only set X or Y location of a cartesian point, because that is not how it works.

Note that object is written in bold in the second sentence. Your Organization class is merely a data structure here. There's no sense attached to it, it doesn't follow rules and doesn't use its own logic.

In conclusion, if something has some logic/sense about it that the external world knows (should know) nothing about, make an object, but so far, Organization should only have public fields, and absolutely no methods inside. That's not an object.

Answer 2

I like that you are considering your Address, Email, and Phone objects as being immutable. That is, if you needed to change one of these on the Organization object, you would simply discard the old reference for a new one.

As you rightfully stated, using these classes makes it much easier to simplify code where these dependencies are being used, as you can enforce the dependency via type-hinting, and skip all the guarding clauses.

With regards to the question around whether to write getters/setters, I think your approach is fine. There are a couple of other approaches you might consider to find what works best for your application's needs:

Use magic methods defined in a trait or base class to provide immutability to classes that use/extend on this functionality.

Trait example:

trait ReadOnlyMagicAccessors
{
    public function __get($prop) {
        if(!property_exists($this, $prop) {
            throw new Exception("I don't have that property!");
        }
        return $this->$prop;
    }

    public function __set($prop, $value) {
        throw new Exception('I am immutable!');
    }

    public function __isset($prop) {
        if(!property_exists($this, $prop) {
            throw new Exception("I don't have that property!");
        }
        return isset($this->prop);
    }         
}

class Phone
{
    use ReadOnlyMagicAccessors;

    private $phone;

    public function __construct($phone)
    {
        if (!$this->validate($phone)) {
            throw new InvalidPhoneNumberException();
        }

        $this->phone = $phone;
    }

    // no need for getter here anymore

    private function validate($phone)
    {
        // validate the phone number
    }
}

This would allow property-style access to callers (i.e. $phone->phone), which some developers prefer.

Another approach might also use a trait which presents a standard accessor like $class->get('key').

Trait GetAccessor {
    public function get($prop) {
        if(!property_exists($this, $prop) {
            throw new Exception("I don't have that property!");
        }
        return $this->$prop;
    }
}

The trait would be used in the class similarly to the first example.

Another thing I might suggest is to think about your Organization entity and how it interacts in your system.

• Should it really be immutable?
• Should it be capable of holding one-to-many relationships with addresses, emails, and phone numbers?
• Do you need to validate $name in your constructor?

Finally, I don't like change* naming convention as you are doing a simple set operation here. I think set* would be more standard nomenclature for what is happening here.