-1
\$\begingroup\$

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
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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