1

I and one guy have been discussing potential solution of a problem on an unrelated board, regarding the typical getter/setter hate, ie. using getters/setters leads to procedural programming.

I presented the attached solution as a representation of encapsulation and the TellDontAsk principles, which he said was not OO enough, because it persumably:

  • has methods returning a value of an attribute
  • has methods mutating attributes

This is the code:

interface AnimalWithCollar
{
    /**
     * Adds new collar to the animal.
     *
     * @param Collar $collar
     *
     * @throws InvalidArgumentException When the collar you are trying to add already has an owner differentiating from
     *                                  from the animal you are trying to add the collar to.
     */
    public function addCollar(Collar $collar);

    /**
     * Checks, whether the animal has a collar.
     *
     * @return bool
     */
    public function hasCollar();
}

interface ItemForFetching
{
    /**
     * Checks, whether the animal passed as the first parameter can carry the item.
     *
     * @param AnimalThatCanFetch $animal
     *
     * @return bool
     */
    public function animalCanCarryIt(AnimalThatCanFetch $animal);
}

class Stick implements ItemForFetching
{
    /** @var int */
    protected $_weight;

    /**
     * @param int $weightOfTheItem
     */
    public function __construct($weightOfTheItem)
    {
        $this->_weight = $weightOfTheItem;
    }

    /**
     * @inheritdoc
     * @see ItemForFetching::animalCanCarryIt
     */
    public function animalCanCarryIt(AnimalThatCanFetch $animal)
    {
        return $animal->canCarryAnItemWeighting($this->_weight);
    }
}

interface AnimalThatCanFetch extends AnimalWithCollar
{
    /**
     * Fetches the item passed as the first parameter.
     *
     * @param ItemForFetching $item
     *
     * @throws InvalidArgumentException When the animal is unable to fetch the item, because it is too heavy.
     */
    public function fetch(ItemForFetching $item);

    /**
     * Checks, whether the animal can carry an item of said weight.
     *
     * @param int $weight
     *
     * @return bool
     */
    public function canCarryAnItemWeighting($weight);
}

class Dog implements AnimalThatCanFetch
{
    /** @var Collar[] */
    protected $_collars;

    /**
     * @inheritdoc
     * @see AnimalWithCollar::addCollar
     */
    public function addCollar(Collar $collar)
    {
        $collar->assignOwner($this);
        $this->_collars[] = $collar;
    }

    /**
     * @inheritdoc
     * @see AnimalWithCollar::hasCollar
     */
    public function hasCollar()
    {
        return (count($this->_collars) > 0);
    }

    /**
     * @inheritdoc
     * @see AnimalThatCanFetch::fetch
     */
    public function fetch(ItemForFetching $item)
    {
        if ($item->animalCanCarryIt($this) == false)
        {
            throw new InvalidArgumentException('The item is too heavy. The animal cannot fetch it.');
        }

        // process the fetching logic
    }

    /**
     * @inheritdoc
     * @see AnimalThatCanFetch::canCarryAnItemWeighting
     */
    public function canCarryAnItemWeighting($weight)
    {
        return $weight <= 5; // indicating it may carry an item up to 5 kg
    }
}

class Collar
{
    /** @var int */
    protected $_diameter;

    /** @var string */
    protected $_color;

    /** @var AnimalWithCollar */
    protected $_owner;

    /**
     * @param int $diameter
     * @param string $color
     */
    public function __construct($diameter, $color)
    {
        $this->_diameter = $diameter;
        $this->_color = $color;
        $this->_owner = null;
    }

    /**
     * @param AnimalWithCollar $newOwner
     *
     * @throws InvalidArgumentException When the collar is already owned by a different animal than the one passed.
     */
    public function assignOwner(AnimalWithCollar $newOwner)
    {
        if ($this->hasOwner() && $this->belongsTo($newOwner) == false)
        {
            throw new InvalidArgumentException('The collar may have only one owner.');
        }

        $this->_owner = $newOwner;
        $newOwner->addCollar($this);
    }

    /**
     * @param AnimalWithCollar $animal
     * @return bool
     */
    public function belongsTo(AnimalWithCollar $animal)
    {
        return $this->_owner == $animal;
    }

    /**
     * @return bool
     */
    public function hasOwner()
    {
        return $this->_owner != null;
    }
}

Amount of interfaces, naming conventions aside, is there a way this design could be even more object oriented than it already is?

migrated from codereview.stackexchange.com Feb 3 '16 at 15:34

This question came from our site for peer programmer code reviews.

  • @Phrancis Modified the question a little. Thanks for the input. – Ondřej Šimon Feb 2 '16 at 13:30
  • As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. – Jamal Feb 3 '16 at 4:51
2
  • You can replace some of your if statements that use $var == false with !$var

For example:

if ($item->animalCanCarryIt($this) == false)

could have been,

if (!$item->animalCanCarryIt($this))
  • You don't need to declare a variable in constructor as null, it will be null by default anyway.

Remove this line:

$this->_owner = null;
  • You don't need parentheses surrounding (someExpression) sometimes.

This line in your code is one of them.

return (count($this->_collars) > 0);

You can remove the parentheses:

return count($this->_collars) > 0;
  • Not going to downvote nor upvote your answer, but it is unrelated to the question. I am asking for an opinion on the encapsulation and OO approach, not things whether there should be == false or the condition should be preceded by the exlamation mark. – Ondřej Šimon Feb 2 '16 at 13:29
  • 2
    I know. Still you are in code-review. I shared you my opinions. ;) – Ozan Kurt Feb 2 '16 at 13:49
  • Yes, I know. But it was not quite the topic I wanted an answer to, hence the detached attitude. Thank you for the answer nonetheless. – Ondřej Šimon Feb 2 '16 at 14:26
  • @OndřejŠimon to be clear, on this site any and all aspects of a given piece of code may be and are encouraged to be reviewed - no one is required to review or even think about the topics the OP finds most important. – Dannnno Feb 3 '16 at 4:47
2

The code is not strictly TellDontAsk. There are a number of methods where an object is giving some information about itself. Particularly:

AnimalWithCollar::hasCollar()
ItemForFetching::animalCanCarryIt()
AnimalThatCanFetch::canCarryAnItemWeighting()
Collar::belongsTo()
Collar::hasOwner()

You can see that these methods are questions, thus you're asking not telling.

For TellDontAsk, each method should be an action. The only return is something that is created as result of the action. So not a boolean, data, a property, etc.

Additionally:

Dog::addCollar() and Collar::assignOwner() create a dead loop.

You should follow PSR-1 and PSR-2. Which among other this say that a property should not start with an underscore.

AnimalWithCollar has nothing to do with AnimalThatCanFetch, so don't extend the interface. Also anything might be able to fetch (like a drone). No need to make that animal specific.

With Collar you're binding on an implementation rather than an interface. Define a Wearable interface instead.

AnimalWithCollar says it's an animal with a collar, but that's not the case. You want to say it can wear a collar. Or perhaps better, it's an animal with a neck AnimalWithNeck.

InvalidArgumentException is a LogicException, meaning something is wrong with the code. You should not use it for a runtime exception. You should create your own RuntimeExpection for this.

interface Animal
{
    /**
     * Make the animal ware an item
     *
     * @param Wearable $item
     *
     * @throws UnwearableException    If the item can't be worn
     * @throws WearableWornException  When item is already worn
     */
    public function wear(Wearable $item);
}

interface AnimalWithNeck extends Animal
{
    /**
     * Put an item on the neck of the animal
     *
     * @param Wearable $item
     * @param int      $diameter
     *
     * @throws WearableSizeException  If the item is to big or small
     */
    public function putOnNeck(Wearable $item, $diameter);
}

interface Wearable
{
    /**
     * The wearable gets put on the animal
     *
     * @param Animal $animal
     *
     * @throws UnwearableException    If the item can't be worn by the animal
     * @throws WearableSizeException  If the item is to big or small
     * @throws WearableWornException  When item is already worn
     */
    public function getPutOn(Animal $animal);
}

interface Fetcher
{
    /**
     * Fetches the item passed as the first parameter.
     *
     * @param Fetchable $item
     *
     * @throws FetchableToHeavyException  When the actor is unable to fetch the item, because it is too heavy.
     */
    public function fetch(Fetchable $item);

    /**
     * Carry an item with a specific weight
     *
     * @param Fetchable $item
     * @param int $weight
     *
     * @throws FetchableToHeavyException  When the actor is unable to fetch the item, because it is too heavy.
     */
    public function carry(Fetchable $item, $weight);
}

interface Fetchable
{
    /**
     * The item is picked up by an actor
     *
     * @param Fetcher $actor
     *
     * @throws FetchableToHeavyException  When the actor is unable to fetch the item, because it is too heavy.
     */
    public function getPickedUpBy(Fetcher $actor);
}

class UnwearableException extends LogicException
{
}

class WearableSizeException extends RuntimeException
{
}

class WearableWornException extends RuntimeException
{
}

class FetchableWeightException extends RuntimeException
{
}

class Stick implements Fetchable
{
    /** @var int */
    protected $weight;

    /**
     * @param int $weight
     */
    public function __construct($weight)
    {
        $this->weight = $weight;
    }

    /**
     * @inheritdoc
     * @see Fetchable::getPickedUpBy
     */
    public function getPickedUpBy(Fetcher $actor)
    {
        return $actor->carry($this, $this->weight);
    }
}

class Collar implements Wearable
{
    /** @var int */
    protected $diameter;

    /** @var string */
    protected $color;

    /** @var Animal */
    protected $owner;

    /**
     * @param int $diameter
     * @param string $color
     */
    public function __construct($diameter, $color)
    {
        $this->diameter = $diameter;
        $this->color = $color;
    }

    /**
     * @inheritdoc
     * @see Wearable::getPutOn
     */
    public function getPutOn(Animal $newOwner)
    {
        if ($newOwner === $this->owner) {
            return;
        }

        if ($this->owner) {
            throw new WearableWornException('The collar is already worn by another owner.');
        }

        if (!$newOwner instanceof AnimalWithNeck) {
            throw new UnwearableException('A collar can only be worn by an animal with a neck');
        }

        $newOwner->putOnNeck($this, $this->diameter);
        $this->owner = $newOwner;
    }
}

class Dog implements AnimalWithNeck, Fetcher
{
    /** @var Wearable[] */
    protected $wearables;

    /**
     * @inheritdoc
     * @see Animal::wear
     */
    public function wear(Wearable $item)
    {
        $collar->getPutOn($this);
    }

    /**
     * @inheritdoc
     * @see AnimalWithNeck::putOnNeck
     */
    public function putOnNeck(Wearable $item, $diameter)
    {
        if ($diameter < 3 || $diameter > 10) {
            throw new WearableSizeException("The item has a diameter of $diameter, but I can only wear items with a diameter between 3 and 10 around my neck");
        }

        $this->wearables[] = $item;
    }

    /**
     * @inheritdoc
     * @see Fetcher::fetch
     */
    public function fetch(Fetchable $item)
    {
        $item->getPickedUpBy($this);

        // process the fetching logic
    }

    /**
     * @inheritdoc
     * @see Fetcher::carry
     */
    public function carry(Fetchable $item, $weight)
    {
        if ($weight > 5) {
            throw new FetchableWeightException("The item weights $weight, but I can only carry 5");
        }
    }
}

This is not my preference of structuring the code. The answer is specific to the question of the OP. Martin Fowler has this to say about it:

But personally, I don't use tell-dont-ask. I do look to co-locate data and behavior, which often leads to similar results. One thing I find troubling about tell-dont-ask is that I've seen it encourage people to become GetterEradicators, seeking to get rid of all query methods. But there are times when objects collaborate effectively by providing information. A good example are objects that take input information and transform it to simplify their clients, such as using EmbeddedDocument. I've seen code get into convolutions of only telling where suitably responsible query methods would simplify matters [1]. For me, tell-don't-ask is a stepping stone towards co-locating behavior and data, but I don't find it a point worth highlighting.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy