Avoid checking source,target type multiple times

[sivajik34]

We can check only one time type of source or target.
Present code we are checking source ,target type for each field unnecessarily.

[vrann]

This method becomes much more complex than it should be. There is duplication of logic and the complexity of the method increases.

You can do following:

1. wrap the information about isDataObject, isExtensible, etc in some simple class, i.e. and pass this object as input to the methods, where currently object is passed.

class DataObjectTypeWrapper {
    public function __construct($object) {}

    public function getObject() {}

    public function isDataObject() {}

    ...
}

2. Move every "if" body to a separate method (and potentially separate class) which accepts the DataOjectTypeWrapper. There are a lot of duplication of logic right now which an be avoided by re-suing those methods.

[sivajik34]

@vrann
Regarding 2nd point:I'm not getting to avoid duplication.
we need create separate class or method for inner if loop,or outer if loop?
maybe I'm not getting your implementation thoughts on this.

<?php

/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
/**
 * Utility class for checking type of  object
 */

namespace Magento\Framework\DataObject;

class DataObjectTypeWrapper {

    private $dataObject;

    public function __construct($object) {
        $this->dataObject = $object;
    }

    public function getObject() {
        return $this->dataObject;
    }

    public function isArray() {
        return is_array($this->dataObject);
    }

    public function isDataObject() {
        return ($this->dataObject instanceof \Magento\Framework\DataObject);
    }

    public function isExtensible() {
        return ($this->dataObject instanceof \Magento\Framework\Api\ExtensibleDataInterface);
    }

    public function isAbstractSimple() {
        return ($this->dataObject instanceof \Magento\Framework\Api\AbstractSimpleObject);
    }

}

[vrann]

@sivajik34 Thank you for the contribution. This is a good intent. I have few comments to implementation. Also, would you mind squashing commits and force-pushing the branch? There are a lot of commits under the PR.

[sivajik34]

@vrann Thank you for your response.I don't mind squashing commits and force-pushing the branch.I will try my best to refactor using your suggestions.

[orlangur]

What is the performance benefit of such change? Are there any other benefits besides performance?

/home/travis/build/magento/magento2/lib/internal/Magento/Framework/DataObject/Copy.php:62	
The method copyFieldsetToTarget() has a Cyclomatic Complexity of 86. The configured cyclomatic complexity threshold is 10.
/home/travis/build/magento/magento2/lib/internal/Magento/Framework/DataObject/Copy.php:62	
The method copyFieldsetToTarget() has an NPath complexity of 2106. The configured NPath complexity threshold is 200.

This does not look like an acceptable refactoring generally, it is better to check builds on Travis CI prior to PR creation and eliminate all PHPMD violations.

[sivajik34]

@orlangur I agreed.I know its not acceptable.based on @vrann i will try to implement without any PHPMD violations.I don't know PHPMD violations and all.but i try to learn & want contribute something to Magento.
I'm a l just beginner in this area.I always welcome your feedback also.

[orlangur]

@sivajik34 no problem, we all are learning here :) Good luck with making PHPMD happy about your changes.

[vrann]

@sivajik34 Hi, do you expect to do more changes soon to this PR?

[sivajik34]

@vrann Yes,I want but no idea how to move further.

[okorshenko]

Closing this PR due to inactivity. @sivajik34 Please create new PR when ready.