Skip to content

Conversation

@imanghafoori1
Copy link

Entrust.php is somewhat refactored.
Resulting in a more concise and cleaner code by removing some duplication.
and your unit tests still pass...

I hope you like it...
I can continue to work on it and apply some design patterns if you agree upon this commit.

Entrust.php is somewhat refactored.
Resulting in a more concise and cleaner code by removing some duplication.
and you unit tests still pass...
@jobrios
Copy link
Contributor

jobrios commented Nov 26, 2015

The duplication removal and greater expressiveness you've got here are good ideas, but you should avoid creating additional state variables like $this->filterName.

@imanghafoori1
Copy link
Author

I know it is a new way of thinking and looks weird for you but the whole propose of OOP is to provide such a way of programming.
But this is not the end of story...
There is a rule in oop design that says:

  • In most of cases private properties (and methods) should end up being extracted into a new dedicated class as public properties and that new class should be injected into the current one.

So if I continue to work on this and apply Single Responsibility Principle on it...
Of course this class should be broken it to smaller ones each with just a single responsibility and clean intent.
The strategy pattern seems to be a good candidate to for the case of this package.

Any way, that private property causes no harm.

@imanghafoori1
Copy link
Author

I do like to spend my weekend refactoring this package if you agree...

@andrewelkins
Copy link
Collaborator

I like this. I'll think it over today and may merge it tomorrow.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The strategy of making the filer name is all gathered here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants