Gfrktyl

I have a function addSupplier() that adds a supplier. Each supplier has multiple addresses and each address has multiple contacts.

public function addSupplier ($request) {

        // ADDING THE SUPPLIER BASIC INFO

        $supplier_fillable_values = array_merge(

            $request -> except('address_contact_inputs'),

            ['created_by' => $this->getAuthUserId()]

        );

        $new_supplier = Supplier::create($supplier_fillable_values);



        // ADDING THE SUPPLIER ADDRESSES AND THE ADDRESS CONTACTS

        foreach ($request -> address_contact_inputs as $address) {

            // ADD ADDRESS

            $new_supplier_address = $new_supplier -> addresses() -> create([

                'address'       => $address['address'],

                'created_by'    => $this -> getAuthUserId()

            ]);



            // ADD ADDRESS CONTACTS

            foreach ($address['contacts'] as $contact)

                $new_supplier_address -> contacts() -> create([

                    'name'          => $contact['name'],

                    'phone_number'  => $contact['phone_number'],

                    'created_by'    => $this -> getAuthUserId()

                ]);

        }





        return $request -> address_contact_inputs;

    }

Everything works fine but question is: Is this the best practice or I should use events and create an event for:

1. Storing the supplier basic info


2. Storing the supplier addresses


3. Storing contact for each supplier's address

You could use events for triggering these kinds of things, but there is a danger that your code starts to become harder to follow. Martin Fowler gives a good top-level talk on it here.

There are two issues jumping out at me:

Firstly, your method accepts a request object so there is no indication as to what the requirements of that method are. You could change the method to accept specific arguments such as $created_by, $address_details, etc. This would make it easier to re-use the code elsewhere if needed.

I would make the controller then responsible for preparing that set of data (e.g. handling the preparation of the address details array, getting the 'created_by' etc). And make your addSupplier method work with a set of arguments that could be run from anywhere in your app.

public function addSupplier ($primary_contact_details, $contact_addresses, $created_by = NULL) {

    // ADDING THE SUPPLIER BASIC INFO

    $supplier_fillable_values = array_merge(

        $primary_contact_details,

        ['created_by' => $created_by]

    );

    $new_supplier = Supplier::create($supplier_fillable_values);



    // ADDING THE SUPPLIER ADDRESSES AND THE ADDRESS CONTACTS

    foreach ($contact_addresses as $address) {

        // ADD ADDRESS

        $new_supplier_address = $new_supplier -> addresses() -> create([

            'address'       => $address['address'],

            'created_by'    => $this -> getAuthUserId()

        ]);



        // ADD ADDRESS CONTACTS

        foreach ($address['contacts'] as $contact)

            $new_supplier_address -> contacts() -> create([

                'name'          => $contact['name'],

                'phone_number'  => $contact['phone_number'],

                'created_by'    => $this -> getAuthUserId()

            ]);

    }



    return $request -> address_contact_inputs;

}

Secondly, it's good practise to avoid too many levels of indention, so you could refactor to use one level of indentation and splitting out the foreach loops to their own public/private methods on the same class.