3
\$\begingroup\$

I am interested in how to handle errors/exceptions/returned data correctly.

For example, I have a script which creates, updates and deletes products data.

Here is method that receives data from an API, processes and decides what to create/update/delete.

private function importProducts($brand)
{
    $products = $this->dsApi->getProductsByBrand($brand);

    //returns ['create' => [], 'update' => [], 'delete' => [] ]
    $preparedProducts = $this->prepareProducts($products, $brand);

    foreach ($preparedProducts['delete'] as $key => $product) {
        $this->deleteProduct($product);
        $this->log->info(
            'Brand ' . $brand->name . '(' . $brand->id_brand . ')'
            . ' id: ' . $product . ' was deleted'
        );
    }

    foreach ($preparedProducts['create'] as $key => $product) {

        $this->importProduct($product, $brand);
        $this->log->info(
            'Brand ' . $brand->name . '(' . $brand->id_brand . ') sku: ' . $product['sku'] . ' was imported'
        );
    }

    foreach ($preparedProducts['update'] as $key => $product) {
        $this->importProduct($product, $brand, $product['idProductWc']);
        $this->log->info(
            'Brand ' . $brand->name . '(' . $brand->id_brand . ') sku: ' . $product['sku']
            . ' id: ' . $product['idProductWc'] . ' was updated'
        );
    }
}

And for example deleteProduct() method:

private function deleteProduct($product)
{
    wp_delete_post($product, true);

    $args = [
        'order' => 'ASC',
        'post_mime_type' => 'image',
        'post_parent' => (int) $product['idProductWc'],
        'post_status' => 'inherit',
        'post_type' => 'attachment',
    ];
    $child = get_children($args);
    foreach ($child as $id => $item) {
        wp_delete_attachment($id, true);
    }
}

Is the deletion method written correctly? Is it necessary to handle an error in wp_delete_post() method? How to handle it? What to write in logs in this case? Should the method return a response?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Ludisposed Dec 14 '17 at 13:15
  • 1
    \$\begingroup\$ @Ludisposed I'm sorry, got it. \$\endgroup\$ – Zhi V Dec 14 '17 at 13:17
2
\$\begingroup\$

Looking at your importProducts() method, firstly ask if this needs to be public, private or protected see PHP visibility

Now the importProducts() method in question, I would separate and have three methods as follows:

/**
 * @param $brand string
 * @param $productsToDelete array
 */
protected function _deletePreparedProducts($brand = null, $productsToDelete = array()) {
    if(empty($brand)) {
        /* Do we need a brand? If not throw an Exception or log to your debug logger */
    }
    /* deletion logic here */
}

/**
 * @param $brand string
 * @param $productsToCreate array
 */
protected function _createPreparedProducts($brand = null, $productsToCreate = array()) { /* create logic here */ }

/**
 * @param $brand string
 * @param $productsToUpdate array
 */
protected function _updatePreparedProducts($brand = null, $productsToUpdate = array()) { /* prepare logic here */ }

I would therefore change the importProducts() method as follows (assuming that you still want this as a private method):

<?php
private function importProducts($brand)
{
    $products = $this->dsApi->getProductsByBrand($brand);

    $preparedProducts = $this->prepareProducts($products, $brand);

    if(!empty($preparedProducts['delete'])) {
        $this->_deletePreparedProducts($brand, $preparedProducts['delete']);
    }

    if(!empty($preparedProducts['create'])) {
        $this->_createPreparedProducts($brand, $preparedProducts['create']);
    }

    if(!empty($preparedProducts['update'])) {
        $this->_updatePreparedProducts($brand, $preparedProducts['update']);
    }
}

Using this approach, if you need to change the logic to update a product, you will only have to worry about your update method, rather than having lots of logic in one method. And in your main method you will be checking if there are any products to delete, update or create first rather than assuming that there is, as is the case in your original script.

If there are no products to import then why was this method called? This is where you could throw an exception, or log to a debug file, like this:

    if(empty($preparedProducts['delete'])
        && empty($preparedProducts['create'])
        && empty($preparedProducts['update'])) {
        throw new \Exception("Import products called without any products to import");
    }

I might also consider preparing the data first before calling the importProducts() method, rather than preparing that data within the method - this will depend on your dataflow.

I would finally suggest that as you are updating and deleting products, the method name may be incorrect. manageProducts() or something similar might be a better name for this method.

\$\endgroup\$
  • \$\begingroup\$ Thank you. I call importProducts() from another public function.. And what about deleteProduct()? Should I check wrong incoming data? \$\endgroup\$ – Zhi V Dec 8 '17 at 18:18
  • \$\begingroup\$ Yes the basic idea is to always sanitize date if you are able to first, I've thought of a way of improving my answer so I will update it presently. \$\endgroup\$ – Shaun Bebbers Dec 11 '17 at 9:37
  • \$\begingroup\$ Thank you. I also added another question from this project [codereview.stackexchange.com/questions/182586/… \$\endgroup\$ – Zhi V Dec 12 '17 at 12:01
  • \$\begingroup\$ Updated post. I added return values to the function deleteProducts() and checking it. Is it ok? \$\endgroup\$ – Zhi V Dec 14 '17 at 13:13
  • \$\begingroup\$ paste.laravel.io/9717a794-3596-441f-935f-1f9450035bd4 \$\endgroup\$ – Zhi V Dec 14 '17 at 13:24

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.