SOLID Principle In Laravel with Repository Pattern

Question

I have some confusion about use of Controller with Repository Pattern while maintaining SOLID Principle. Consider, I have two types of Quotations

1. Commercial Quotation
2. Private Quotation

And there is a high chance of new types of quotations in future. Each Quotations has different fields, business logics yet they share many common functions. So I created a QuotationInterface

Quotation Inteface

interface QuotationInterface
{   
    public function save(array $data);

}

Quotation class that implement the interface

class CommercialQuotation implements QuotationInterface
{   
    public function(array $data)
    {
        // save commercial quotation
    }
}

class PrivateQuotation implements QuotationInterface
{   
    public function(array $data)
    {
    // save Private quotation
    }
}

Quotation Repository

class QuotationRepository 
{
    public function save(array $data, QuotationInterface $quotation)
    {
        $quotation->save($data);
    }
}

QotationController

public function store(Resource $resource)
{

    $inputs = $resource->all();

    /**
    *  Clearly here Open/Close Principle is broken
    */

    if ($inputs['type'] == 'private'){
        $quotation = new PrivateQuotation;;
    }
    else if($inputs['type'] == 'commercial'){
        $quotation = new CommercialQuotation;
    }

    $this->repo->save($inputs, $quotation);
}

Here in my QuotationController, it is clearly violating Open/Close Principle..

Is it a good idea to Create a Controller for each type of quotation (might be 10+ some day, who know?) to avoid the OCP violation or my design is just wrong? Any suggestion, design change tips, resource are welcome.

NOTE: My Quotation Controller will have many other functions except the save only.

Answer 1

If you're going ahead the way you've shown, i suggest you to use a single controller for your quotations and use a Factory design pattern to create your $quotation objects

For example, with a simple factory like this:

//FACTORY CLASS
abstract class QuotationFactory
{
    /** return QuotationInterface */
    public static function createQuotation($type)
    {
        switch($type)
        {
            CASE "private":
                return new PrivateQuotation();
            break;

            CASE "commercial":
                return new CommercialQuotation();
            break;    
        }
    }
}

you could use the factory from you controllers' methods:

//YOUR CONTROLLER'S METHOD
public function store(Resource $resource)
{   
    $inputs = $resource->all();

    //delegate objects creation to factory class
    $quotation = QuotationFactory::createQuotation( $inputs['type'] );

    $this->repo->save($inputs, $quotation);
}

This way you are not going to violate the open/closed principle in your controllers, because as you add quotations, you'll only need to modify the method of the factory (adding cases to the switch statement ), and it will return a QuotationFactory object wherever needed.

This will also keep your code DRY and SOLID because you don't have to repeat the if/else statements to create your objects in your controller's methods as you delegate the responsability of the objects' creation to a specific factory class

As correctly pointed ount in the comments below, the Simple factory is going to help you avoiding the Open/Closed Principle in your controllers but be ware that, from a more general point of view, the Simple Factory itself is inherently violating the OCP as it uses a switch case.

Anyway, from what i see of your application, the Simple factory could be a good solution, as your primary concern is to build in many places instances from a variable type. Thus, using the Simple factory you can 'hide' this process of creating objects into the factory and get the instances you need in your controllers. So you are violating the OCP only inside the factory in the switch case, but i think this could be a beareble trade-off

Answer 2

I think this mostly depends on the scope of your application. In the PHP world these days, people are so angry with if/else statements :). However, if that works for your application and seems reasonable in the scope of YOUR context, I think that's fine.

Businesses change and Business changes aren't always easy to plan for. You can only try to make those changes easier when they arise.

That being said, classes are cheap and I think that having separate classes at this point (and in the future) is very reasonable. If the requirements for each type of quotation expand, you'll be in good footing and I don't think you're abstracting so far that you're code will be difficult to understand.