small optimization in if-condition

[likemusic]

Description

There is no needs to cast $value to bool in runtime because it have value === '' by if-condition above

Fixed Issues (if relevant)

Not relevant.

Manual testing scenarios

Not relevant.

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[VladimirZaets]

Hi @likemusic , thanks for contribution. In the current case, you can optimize if condition better, and to avoid the second if condition inside. That's will looks something like

if ($value === '' && $considerUseDefaultsAttribute && !(bool)$product->getData($attribute)) {
...
}

[likemusic]

@VladimirZaets updated according you review.
Casting to bool removed in if scope, because its not necessary with not (!) operator.

I made more DDD version at https://github.com/likemusic/magento2/blob/fix-prepare-product-attributes-ddd/app/code/Magento/Catalog/Controller/Adminhtml/Product/Initialization/Helper/AttributeFilter.php

Can I marge it to pull-request's branch?

There is if statement moved to separate function. Maybe you can suggest a more appropriate name for the function?

'Protected' access modification used to have possibility override this function if necessary. Is that okay?
I use blank line before return statement. Is that okay?

[likemusic]

By branches have many small commits. Maybe it's would be better to rebase they into one commit before merging to this pull-request's branch?

[likemusic]

Seems that according to Technical guidelines selected function should be private and code inside it should be moved to separate class like AttributeShouldNotBeUpdatedResolver. Am I right?

[DanielRuf]

Was the annotation for $product used?

[VladimirZaets]

Hi @likemusic
That you create the new method is ok, but according to Magento Technical guidelines, this method should be private.

Also, the better name for this method, I think, something like isAttributeShouldNotBeUpdated or attributeShouldBeUpdated and in this case method should return inverted result and condition will look like !$this->isAttributeShouldBeUpdated(...).

Uses blank line before return statement is ok.

Maybe it's would be better to rebase they into one commit before merging to this pull-request's branch?

• That's will be good, but it's not a requirement

In the current case, this is the Helper class. Creating helper classes it's a bad practice in general because it violates the Single responsibility principle. Instead of helpers, we should use services.
So, in the current case not need to move method to separate class.

[likemusic]

@DaanKouters thanx for notice. It fixed.

[likemusic]

@VladimirZaets, I renamed method to isAttributeShouldNotBeUpdated () because it more accurately reflects what exactly the method does.

In the current case, this is the Helper class. Creating helper classes it's a bad practice in general because it violates the Single responsibility principle.

It's not clear to me. Why/how Helper class violates the Single responsibility principle?

Instead of helpers, we should use services.

Did you mean we should introduce new Service Contract interface and create class that implements it?

So, in the current case not need to move method to separate class.

Why? I still have some misunderstanding about using private/protected access modifiers. I asked related question here.

[VladimirZaets]

Why/how Helper class violates the Single responsibility principle?

• Because Helper class have a lot of reason to update. To Helper class we can add a lot of methods that will resolve differently tasks, so Helper class has a lot of responsibility. More detail you can read here.

Did you mean we should introduce new Service Contract interface and create the class that implements it?

• In general, you are right, but if you want to create new service you should think through a design of service, find the cases where this method will be used and think about cases of using this service at all. So, as for me, this is a large work that shouldn't be doing in the scope of this fix, current fix is ok.

[magento-engcom-team]

Hi @likemusic. Thank you for your contribution.
Changes from your Pull Request will be available with the upcoming 2.2.5 release.

[likemusic]

@VladimirZaets

In general, you are right ... So, as for me, this is a large work that shouldn't be doing in the scope of this fix

I still can't understand If we not follow "composition over inheritance" why we keep method private but not protected?

I think extensible code is more important and valuable feature then suppressing warnings from meqp-validator. Isn't it?