Abstract classes - please, no private methods

[rela589n]

Abstract - protected, Final - private

The oro commerce has a great deal of source code, it has it's own legacy.

I'd like to point out to \Oro\Bundle\VisibilityBundle\Api\Processor\AbstractSetVisibilityScope. This class is actually abstract, however there are some private methods (like getWebsite, isVisibilityExists).

I would like to ask not to make any private methods in the abstract class. If you would really like to disallow some method extension, please, make it final protected, but not private.

Thank you!

[mbessolov]

@rela589n please elaborate more on why this change is needed in your opinion. It would be helpful if you describe the custom visibility scope processor in your project for which such change would be beneficial.

[rela589n]

I would like to change the logic of process method, however I can't just take and override it. It uses private methods. Hence, just to override it, it is necessary to copy-paste all the private methods it uses. This is not something specific to current case, but rather a general thing. Clients of the code should be able to extend it without copy-pasting most of the logic.

Thanks

[mbessolov]

There are only 9 lines of code in those private methods, so not so much to copy if it comes to that. The existing implementations of this abstract class in our code do not justify the need of polluting the interface provided by this abstraction with additional methods. Without any specific details to showcase concrete benefits this refactoring is unlikely to happen.

[rela589n]

Then question arises why won't you make this class final, if it's not supposed to be extended?

[mbessolov]

It is supposed to be extended, and it provides a concise and comprehensive interface for extension by inheritance - 2 abstract methods to implement. If you need to make a lot of changes, or if you need to override the process method, then this abstraction may not be the right one to use for your use case at all. By looking at the implementations (each of which is 3-4 lines of code) in our products I see no obvious issues that would require any more methods to be available for extension: \Oro\Bundle\VisibilityBundle\Api\Processor\SetCustomerGroupProductVisibilityScope \Oro\Bundle\VisibilityBundle\Api\Processor\SetCustomerProductVisibilityScope \Oro\Bundle\VisibilityBundle\Api\Processor\SetProductVisibilityScope

[mbessolov]

Declaring the process method as final would probably have made the original intent of this abstraction more clear.

[rela589n]

Yes, sorry. I meant why does classes, which extend AbstractSetVisibilityScope (actually I thought about SetCustomerGroupProductVisibilityScope) are not final? Also, as you have remarked, it would make sense to make process method final.