0

A teammate of mine wrote some code in the following way:

Class Foo implements Job {
// Framework uses type-hinting for dependency injection, only works on the handle() method
// not on other methods
public function handle(Dependency1 $dep1, Dependency2 $dep2) {

    $this->setAlpha($dep1, $dep2);
    $this->setBravo($dep1, $this->someOtherObject);
}

// This function is only called by the handle method
private function setAlpha($dep1, $dep2) {
    // Do something
}

// This function is only called by the handle method
private function setBravo($dep1, Object $otherObject) {
    // Do something
    $this->something = $dep1->something();
}
}

I am trying to explain to him why it is better to assign the dependencies to private or protected class properties and use them in your functions so you do not pollute the method parameters with dependencies.

His argument is that it is much more readable this way as you will know what the dependencies of the methods are.

My argument is that if you write your methods this way they can rather be made static which is not something that is good. And that if you ever get extra methods that need these dependencies, you need a different way of retrieving them

Does someone know if the code above is a good way to do it or should it be rewritten to class properties and why?

I am failing to find good reasons and am coming to this StackExchange to hear your opinions

2
  • No it's not. The methods are private, they can't use DI. Only handle() can use DI
    – Mazzy
    Jun 7, 2018 at 11:30
  • The dependencies must be injected via the handle() method, it's a framework thing(Laravel PHP). the question is, do you assign them to class properties or pass them along to the methods via params
    – Mazzy
    Jun 7, 2018 at 11:35

1 Answer 1

2

In my view, you are both right at times. It all depends on how often dep1 and dep2 change.

You friend is right: it is good to narrow down the scope of dependencies as much as possible. Passing them in as parameters fully narrows that scope. And if the class is large, it does indeed improve readability.

You are right is that putting those dependencies into private fields can simplify the code as you reduce the number of parameters each function has. And as classes should not be large, this approach can actually improve readability.

What I utter disagree with you on is the notion that "they can rather be made static which is not something that is good". Absolutely these methods can be made static. They have their dependencies injected via parameters, rather than accessed globally, so they are prime candidates for being static.

So which approach to choose? If those dependencies are fairly stable throughout the lifetime of the application, then inject them via a constructor and store them as fields. That way you need only pass one object around your system, Foo. If however they are different almost every time handle is called, you'd have to create a new Foo every time with that approach. So instead make them static and pass the dependencies in via parameters.

1
  • I agree with you that they are candidates for static methods. If the method setBravo() edits a class property, it cannot be made static. Should the method be rewritten so it returns the values that can then be set from the handle() method to the right class properties? Due to framework limitations, the constructor method is not DI, only the handle method. The class represents a single job so it will stay small
    – Mazzy
    Jun 7, 2018 at 11:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.