6

I'm developing a quite complex logistics management system which will keep growing into several other ERP related modules. Therefore, I am trying to have as much of the SRP and Open/Close Principles in place for ease of extension and domain based management.

Therefore, I decided to use Laravel and the following pattern (not sure if this has a name or not):

I will use the PRODUCT object for my example. An object/entity/domain has a Class class ProductService {}

This class has a Service Provider which is included in the providers array and is also autoloaded: ProductServiceServiceProvider

The service provider instantiate (makes) the ProductRepository which is an interface. The interface currently has a MySQL (and some Eloquent) called EloquentProductRepository implementation(s) and a ProductRepositoryServiceProvider binds the implementation which is also loaded and in the providers array.

Now a product has many different attributes and relationships with other domains and because the other domains (or entities) need to be fully detached and again abiding with the above principle (SRP etc..) I decided to also have the same structure for them as i do for the product...I know some might think that this is too much but we need to have the system very extendable and to be honest I like to be organised and have a uniform pattern (it doesn't take that much more time and saves me a lot later).

My question is this. The ProductService which handles all the business logic of the Product and makes the "Product" what it is will have several dependencies injected on creation of it's instance through the constructor.

This is what it has at the moment:

namespace Ecommerce\Services\Product;

use Ecommerce\Repositories\Product\ProductRepository;
use Ecommerce\Services\ShopEntity\ShopEntityDescriptionService;
use Content\Services\Entity\EntitySeoService;
use Content\Services\Entity\EntitySlugService;
use Ecommerce\Services\Tax\TaxService;
use Ecommerce\Services\Product\ProductAttributeService;
use Ecommerce\Services\Product\ProductCustomAttributeService;
use Ecommerce\Services\Product\ProductVolumeDiscountService;
use Ecommerce\Services\Product\ProductWeightAttributeService;
use Ecommerce\Services\Product\ProductDimensionAttributeService;

/**
 * Class ProductService
 * @package Ecommerce\Services\Product
 */
class ProductService {

    /**
     * @var ProductRepository
     */
    protected $productRepo;

    /**
     * @var ShopEntityDescriptionService
     */
    protected $entityDescription;

    /**
     * @var EntitySeoService
     */
    protected $entitySeo;

    /**
     * @var EntitySlugService
     */
    protected $entitySlug;

    /**
     * @var TaxService
     */
    protected $tax;

    /**
     * @var ProductAttributeService
     */
    protected $attribute;

    /**
     * @var ProductCustomAttributeService
     */
    protected $customAttribute;

    /**
     * @var ProductVolumeDiscountService
     */
    protected $volumeDiscount;

    /**
     * @var ProductDimensionAttributeService
     */
    protected $dimension;

    /**
     * @var ProductWeightAttributeService
     */
    protected $weight;

    /**
     * @var int
     */
    protected $entityType = 3;


    public function __construct(ProductRepository $productRepo, ShopEntityDescriptionService $entityDescription, EntitySeoService $entitySeo, EntitySlugService $entitySlug, TaxService $tax, ProductAttributeService $attribute, ProductCustomAttributeService $customAttribute, ProductVolumeDiscountService $volumeDiscount, ProductDimensionAttributeService $dimension, ProductWeightAttributeService $weight)
    {
        $this->productRepo = $productRepo;
        $this->entityDescription = $entityDescription;
        $this->entitySeo = $entitySeo;
        $this->entitySlug = $entitySlug;
        $this->tax = $tax;
        $this->attribute = $attribute;
        $this->customAttribute = $customAttribute;
        $this->volumeDiscount = $volumeDiscount;
        $this->dimension = $dimension;
        $this->weight = $weight;
    }
`

Is it bad practice to have as much arguments passed to the constructor in PHP (please ignore the long names of the services as these might change when the ERP namespaces have been decided upon)?

As answered by Ben below, in this case it is not. My question was not related to OOP but more to performance etc.. The reason being is that this particular class ProductService is what web deves would do with a controller, i.e. they would probably (and against principles) add all DB relationships in one ProductController which handles repository services (db etc..) and attaches relationships and then it suddenly becomes your business logic.

In my application (and I see most applications this way), the web layer is just another layer. MVC takes care of the web layer and sometimes other Apis too but I will not have any logic except related to views and JS frameworks in my MVC. All of this is in my software.

In conclusion: I know that this is a very SOLID design, the dependencies are injected and they really are dependencies (i.e. a product must have tax and a product does have weight etc..) and they can easily be swapped with other classes thanks to the interfaces and ServiceProviders. Now thanks to the answers, I also know that it is Okay to inject so many dependencies in constructor.

I will eventually write an article about the design patterns which I use and why I use them in different scenarios so follow me if you're interested in such.

Thanks everyone

Keith Mifsud
  • 1,599
  • 1
  • 16
  • 26
  • 1
    If you find you're injecting too many arguments into a constructor, then it's probably time to start looking at Dependency Injection Containers... take a look at Laravel's [IOC Container](http://laravel.com/docs/ioc) – Mark Baker Sep 07 '14 at 22:17
  • 6
    @Mark Baker: ... or it's time to start thinking that the class depends on too much – zerkms Sep 07 '14 at 22:19
  • 1
    I don't think that's true, if a class needs to depend on a specific concrete class, it should be dependency injected. All well written PHP should follow SOLID principles IMO. Doing this using Laravel's IoC makes it even easier. – David Barker Sep 07 '14 at 22:31

1 Answers1

4

Generally, no, It's not a bad practice, in most cases. But in your case, as said in the comments by @zerkms, it looks like your class is depending on a lot of dependencies, and you should look into it, and think on how to minimize the dependencies, but if you're actually using them and they should be there, I don't see a problem at all.

However, you should be using a Dependency Injection Container (DIC).

An dependency injection container, is basically a tool which creates the class by the namespace you provide, and it creates the instance including all the dependencies. You can also share objects, so it won't create a new instance of it while creating the dependencies.

I suggest you to ue Auryn DIC

Usage:

 $provider = new Provider();
 $class = $provider->make("My\\App\MyClass");

What happens here is this:

namespace My\App;

use Dependencies\DependencyOne,
    Dependencies\DependencyTwo,
    Dependencies\DependencyThree;

class MyClass {

    public function __construct(DependencyOne $one, Dependency $two, DependencyThree $three) {
         // .....
    }
}

Basically, the Provider#make(namespace) creates an instance of the given namespace, and creates the needed instances of it's consturctor's parameters and all parameter's constructors parameters and so on.

Artemkller545
  • 979
  • 3
  • 21
  • 55
  • 4
    Laravel ships with its own IoC container. Why are you advocating Auryn? – Joseph Silber Sep 08 '14 at 00:14
  • Thanks Ben. I wanted to point out that this class in particular does have these dependencies and together they make the Product. I am using Laravel IoC and I have Service Providers' Classes which inject the instance. I don't like to create a new instance within a class as I don't believe that is good practice at all. The instance might be existing and thus a new one will have to rebuild its properties etc.. I will add some more info on the question so that other's can understand this pattern a bit better. Meanwhile thank you for answering my question. – Keith Mifsud Sep 08 '14 at 06:39
  • 1
    @JosephSilber I am not really a fan of Lavarvel, because from what I seen, it creates and binds through static methods. I was always teached to basically forget about Statics in php, and prevent it where possible. – Artemkller545 Sep 08 '14 at 11:00
  • 4
    @BenBeri - You're wrong. Laravel does not bind anything though static methods. It just ships with proxy classes (what they call Facades) that provide static access to the underlying classes. The IoC container itself *does not* bind anything through static methods. – Joseph Silber Sep 08 '14 at 14:00
  • Static global access. It's a load of rubbish. Auryn is a great framework-agnostic tool. – Jimbo Jul 27 '15 at 20:01