Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More code cleaned up, test coverage increased #38

Merged
merged 2 commits into from
Aug 25, 2016

Conversation

curry684
Copy link
Contributor

More generic finetuning and best practices, added some more phpDoc and some tests to increase line coverage to 85%. Made some private functionality that could limit extension protected.

Added some more phpDoc and some tests to increase line coverage to 85%.
* @return \Symfony\Component\HttpFoundation\Request
*/
private function getCurrentRequest()
protected function getCurrentRequest()
Copy link
Member

Choose a reason for hiding this comment

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

I actually disagree with this: we should only add extension points when people ask and give us a good reason - that'll help maintenance in the future. If we keep this private, the can still fetch the Request if they want to, but injecting the request_stack service into directly into some base class.

@curry684
Copy link
Contributor Author

I don't agree with your comments on protected variables and functions from a software architect's point of view. In a properly set up OOP library actually nearly everything should be protected instead of private, because you the developer can never foresee what other people ever want to do with your library.

Take the $container change as a practical use case: exactly why would you want to keep it private? Does the class' other functionality inherently break if a derived class accesses the container? Or even if he overwrites it? In my opinion it doesn't, see there is no reason to forbid reading and writing it. Private visibility should be reserved for properties that will fatally break if 'abused', like the primary key of a Doctrine entity. In nearly all other cases you're only making decisions for others that they can make fine themselves.

Just my 5 cents, we can agree to disagree and I'll revert those changes.

@weaverryan
Copy link
Member

@curry684 In Symfony's core, we take the exact opposite stance :). But, it's definitely subjective.

Here's an old blog post about that decision: http://fabien.potencier.org/pragmatism-over-theory-protected-vs-private.html. In short, private makes it much easier for us to make changes, without breaking end-user code. But if there's a use-case for protected, and we decide it's the right extension point to add, we should do that.

Cheers!

@curry684
Copy link
Contributor Author

Yeah I know the post, I just tend to be on the pragmatist end of things 😉

I'll revert the protected changes tonight, code is on my laptop at home.

@curry684
Copy link
Contributor Author

Done 👍

@weaverryan weaverryan merged commit 28d5a86 into knpuniversity:master Aug 25, 2016
@weaverryan
Copy link
Member

Awesome, thank you!

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.

2 participants