1
\$\begingroup\$

Consider Crate::addLength() in the following code:

class Crate
{
    protected $length, $width, $height;

    function __construct(float $length, float $width, float $height)
    {
        $this->length = $length;
        $this->width = $width;
        $this->height = $height;
    }

    function addLength(float $length): Crate
    {
        return new self($this->length + $length, $this->width, $this->height);
    }
}

I am concerned that Crate object creating a Crate object is a violation of Single Responsibility Principle (SRP). Crate object duties is to keep dimensions of the crate. Creating another crate object seems like introducing extra responsibilities to a class that already has responsibilities.

With that in mind, how to I make the code better?

The directions I think I can go are

1) Is it better to just instead increase the length of the Crate?

function addLength(float $length): void
{
    $this->length += $length;
}

When doing so however, my crate becomes 'stretchy', which is typically not done in real life. Instead, a new crate is constructed with given measurements.

Thus,

2) Is it better to instead delegate object creation to a separate class?

class CrateService
{

    function addLength(Crate $crate, float $length): Crate
    {
        return new Crate($crate->GetLength() + $length, $crate->getWidth(), $crate->getHeight());
    }
}

That seems to work, even if a little cumbersome - we basically call every crate's dimension individually and add the adder length, and create a new crate.

\$\endgroup\$
2
  • \$\begingroup\$ The problem with this question is that we don't know if you're going to do other things with the Crate class. Set the color of the crate? Fill it with stuff? Who knows? You must have other plans for this class than just setting dimensions. Objects in a program don't always follow the same rules as objects in real-life. A 'stretchy' crate is not a problem, unless there's something else that prohibits it. \$\endgroup\$ Commented Jul 29, 2019 at 15:14
  • \$\begingroup\$ My plan is to 1) set up initial crate dimensions based on a pump product that will be in the crate, 2) if a motor is added to the pump (decided by customer), add a length adder to the crate's original length dimension. I may later add a price computation based on crate dimensions to the Crate object. \$\endgroup\$ Commented Jul 29, 2019 at 15:55

1 Answer 1

2
\$\begingroup\$

I see no problem, given your comment, to have a 'stretchy' crate, but in my example code below you can control whether the crate can be stretched or not.

class Crate
{
    private $length, $width, $height;
    private $resizable = true;

    function __construct(float $length, float $width, float $height)
    {
        list($this->length, $this->width, $this->height) = [$length, $width, $height];
    }

    function getDimensions(): array
    {
        return [$this->length, $this->width, $this->height];
    }

    function isResizable()
    {
        return $this->resizable;
    }

    function fixateSize()
    {
        $this->resizable = false;
    }

    function addLength(float $extraLength): float 
    {
        return $this->length += $this->isResizable() ? $extraLength : 0;
    }
}

I've added three methods; getDimensions(), isResizable() and fixateSize(). Note that there's no function to undo the fixateSize(). That is intentional: What would be the point of fixating the size if you can undo it?

I've made the dimension and resize fields private. Only the Crate class should be responsible for manipulating these. Child classes should use the methods of Crate to change them, hence the getDimensions() method.

Note how addLength() now returns the new length, and how it doesn't add the $extraLength when the crate size is fixated. Instead of returning the new length you could return a boolean, indicating whether the extra length is accepted. Another reason for not accepting the extra length could be that the crate would become too long.

I wouldn't use the CrateService class, that just overly complicated, and the addLength() method that returns a new object is just weird. The name of the method doesn't suggest this at all. A better name would have been cloneAndStretch().

\$\endgroup\$
1
  • \$\begingroup\$ Thank you. I have been reading about immutable objects in PHP (blog.joefallon.net/2015/08/immutable-objects-in-php), and that is where I also get the concept and reasoning for making any changes in constructor only. I am struggling with the immutability concept, however, debating on whether I actually need it or not. The crate can be stretchy or not stretchy at code writing time (i.e. it will not change at runtime) \$\endgroup\$ Commented Jul 29, 2019 at 18:09

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.