Dependency Juggling Code Smell

The best way to pass dependencies is via constructor injection. Only in services, we need the dependency in. I've noticed that sometimes the dependency is passed way too early, just to be passed to another service as a method argument.

Why is it a code smell, and how can we avoid it?

Because constructor injection has good karma, we automatically assume that everything in __construct(...) is correct. That's why we can easily miss it.

Imagine this code in your project:

final class TypeResolver
{
    public function __construct(
        // using PHP 8 property promotion here
        private StringTypeResolver $stringTypeResolver,
        private ConstantStringTypeResolver $constantStringTypeResolver,
    ) {
    }

    public function resolve(Expr $expr): Type
    {
        return $this->stringTypeResolver->resolveFromExpr(
            $expr,
            $this->constantStringTypeResolver,
        );
    }
}

Can you see the smell in here? We have 2 services passed as a dependency in the constructor:

Constructor states what dependencies this class needs to work.


How many of them the class directly uses? Just the StringTypeResolver one.

The other one ConstantStringTypeResolver is juggled to StringTypeResolver as side dependency.

2 Pair of Keys

It's like your own keys from your home with one extra pair. You take them both with you everywhere. You use the first pair to unlock doors from your home. The extra pair you use exclusively when you open your door for your wife.

Why would you have these extra keys for your own? Instead, give them to your wife and let her handle it.

Each service should take care only of its own dependencies.
not take responsibility for others.

If we apply this approach to our code, we can move the ConstantStringTypeResolver where it belongs:

 final class TypeResolver
 {
     public function __construct(
        private StringTypeResolver $stringTypeResolver,
-       private ConstantStringTypeResolver $constantStringTypeResolver,
     ) {
     }

     public function resolve(Expr $expr): Type
     {
         return $this->stringTypeResolver->resolveFromExpr(
             $expr,
-            $this->constantStringTypeResolver,
         );
     }
 }

The StringTypeResolver now handles its dependencies on its own:

 final class StringTypeResolver
 {
+    public function __construct()
+    {
+        private ConstantStringTypeResolver $constantStringTypeResolver,
+    }

     public function resolveFromExpr(
        Expr $expr,
-       ConstantStringTypeResolver $constantStringTypeResolver,
     ) {
-        $constantStringTypeResolver->resolve(...);
+        $this->constantStringTypeResolver->resolve(...);
         // ...
     }
 }

👍


PHPStan Got Your Back

The tricky part is to watch for these cases not to get into your code. It's easy to take 2 key sets instead of 1 when you're in a rush to catch a tram or taxi.

Symplify PHPStan Rules got you covered. Just install it:

composer require symplify/phpstan-rules --dev

Then add the rule to your phpstan.neon config:

rules:
     - Symplify\PHPStanRules\Rules\NoDependencyJugglingRule

The rule looks for constructor dependencies that are juggled to method calls. Don't worry about it, and let your CI handle it. That's all for today.


Happy coding!



Do you learn from my contents or use open-souce packages like Rector every day?
Consider supporting it on GitHub Sponsors. I'd really appreciate it!