Possible issue with SRP maybe?

I have two classes. One is the parent of the other. The child should form a wrapper for syntactic sugar to give the client coder a fluent API for a database abstraction ala $results = $query->foo()->bar()->baz(); The responsibility between the two classes is to build a proper command bag in the end. Here is some example code.

class Parent
{
    protected $commandBag;

    public function __construct(Bag $bag = null)
    {
         $this->bag = $bag ?: new Bag();
    }
    private function internalFoo($parameters)
    {
           /* do some validation of the parameters from foo() in the child and fill the command bag */

          $this->commandBag->foo = array_merge($this->commandBag->foo, $parameters);
          return $this;
    }
}

class Child extends Parent
{
    public function __construct(Bag $bag = null)
    {
         parent::__construct($bag);   
    }

    public function foo($parameter1, $parameter2)
    {
        /* do some logic to manipulate (and basically also validate) parameters for the method internalFoo() */

        $this->internalFoo([$parameter1, $this->someMethod($parameter2)])

        return $this;

    }   
}

And to use this, the client dev can do something like this.

$query = new Baz(); // <--grandparent of child
$result = $query->foo('parameter', 'parameter')->bar()->baz(); // the fluent API

My question is, does the building and validating form an SRP violation? Theoretically, it is two reasons for changing either classes. Isn’t it?

Scott

You have an immediate problem here, regardless of SRP: $this->internalFoo is going to error because internalFoo is private.

Without a clearer example, it’s difficult to tell, but this doesn’t look like an SRP problem to me, however I have a few suggestions:

  1. Favour composition over inheritance:
class Child
{
    public function __construct(Parent $parent)
    {
         $this->parent = $parent;
    }

    public function foo($parameter1, $parameter2)
    {
        /* do some logic to manipulate (and basically also validate) parameters for the method internalFoo() */

        $this->parent->internalFoo([$parameter1, $this->parent->someMethod($parameter2)])

        return $this;

    }   
}
  1. Here /* do some logic to manipulate (and basically also validate) parameters for the method internalFoo() */ Why can’t internalFoo do its own validation? If this method needs to accept only valid inputs, the checks should he inside the method, imho.

Hey Tom.

Sorry, it should have been protected, not private.

The validation in internalFoo()` is to make sure the parameters passed as an array to it are formed properly within the array. Not sure why such validation is even necessary (I didn’t create the classes myself actually), and since the child already does building of the array and not the client, it should be fine. Validation isn’t really necessary, I think.

Originally, internalFoo() was just foo() in the parent and thus why inheritance was necessary. foo() was also necessary for the fluent API. Then foo() was moved down to the child, because the other sugar methods were doing a similar kind of validation.

I’ll have a look at the composition suggestion later. Thanks!

As for validation and building the command bag being one responsibility, I still have some doubts. Although, if the reason for change is only for the command bag, then yeah, I guess it is only one reason. Hmmm…

Scott

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.