0

I have check Coding standard of Module and i have get

function nesting level

this is my code

public function execute()
{
    $mediaDirectory = $this->filesystem->getDirectoryRead($this->directoryList::MEDIA);
    $destinationPath = $mediaDirectory->getAbsolutePath('cs/svg/');

    $storeManager = $this->storeManagerInterface;
    $currentStore = $storeManager->getStore();
    $mediaUrl = $currentStore->getBaseUrl($this->urlInterface::URL_TYPE_MEDIA);

    $data = $this->getRequest()->getPost();

    if (isset($data['data'])) {
        $svgname = rand(10000, 99999);
        $path = $destinationPath.$svgname.".svg";
        $img = $data['data'];
        $this->fileForceContents($path, $img);
        return $mediaUrl.'cs/svg/'.$svgname.'.svg';
    }
    if (isset($data['itemId'])) {
        try {
            $cart = $this->cart;
            $items = $cart->getQuote()->getAllItems();
            foreach ($items as $item) {
                if ($item->getId() == $data['itemId']) {
                    $options = $item->getProduct()->getTypeInstance(true)->getOrderOptions($item->getProduct());
                    if (isset($options['info_buyRequest']['svg_data'])) {
                        $part = explode("/", $options['info_buyRequest']['svg_data']);
                        foreach ($part as $key => $value) {
                            if (strpos($value, '.svg') === true) {
                                $myFile = $destinationPath.$value;
                                if ($this->file->fileExists($myFile)) {
                                    $this->file->rm($myFile);
                                }
                            }
                        }
                    }
                }
            }
        } catch (\Exception $e) {
            $this->logger->critical($e->getMessage());
        }
    }
}
3

you have 8 levels of foreach-if-try structures in your code.
Starting from if (isset($data['itemId'])) { you have if-try-foreach-if-if-foreach-if-if.
You should keep this number below 5.

In your case you can simply do that by creating a new method in the class. Let's call it processItem with this code.

private function processItem($item) 
{
    $options = $item->getProduct()->getTypeInstance(true)->getOrderOptions($item->getProduct());
    if (isset($options['info_buyRequest']['svg_data'])) {
        $part = explode("/", $options['info_buyRequest']['svg_data']);
        foreach ($part as $key => $value) {
            if (strpos($value, '.svg') === true) {
                $myFile = $destinationPath.$value;
                if ($this->file->fileExists($myFile)) {
                    $this->file->rm($myFile);
                }
            }
         }
    }
}

Now your code can become

if (isset($data['itemId'])) {
    try {
        $cart = $this->cart;
        $items = $cart->getQuote()->getAllItems();
        foreach ($items as $item) {
            if ($item->getId() == $data['itemId']) {
                $this->processItem($item);
            }
        }
    } catch (\Exception $e) {
        $this->logger->critical($e->getMessage());
    }
}

you can also include if ($item->getId() == $data['itemId']) { in the new method but then you will have to pass $data as a parameter for this method.

  • 1
    one more level could be removed by inverting the first if if (!isset($data['itemId'])) return false; try { ... } – Miroslav Petroff Apr 24 at 13:19
  • Instead $cart->getQuote()->getAllItems() this should work too $item = $cart->getQuote()->getItemById($data['itemId']) and then check if it is not null. – sv3n Apr 24 at 14:16
  • I agree...the code can be improved not just from complexity potin of view – Marius Apr 24 at 14:35

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.