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

Coupling concerns #23

Merged
merged 3 commits into from
Oct 27, 2014
Merged

Coupling concerns #23

merged 3 commits into from
Oct 27, 2014

Conversation

Trismegiste
Copy link
Contributor

Hello, this lib is great !

But since relying on concrete class when type-hinting parameters is a bad practice (LSP), I've added an interface to this service for loose-coupling.

In the process I've added some missing visibility keywords to follow PSR-2

Hope this helps 🍻

@jd327
Copy link
Collaborator

jd327 commented Oct 21, 2014

Hi @Trismegiste, haven't forgotten about your PR, just been traveling. Need some time to review.

@Trismegiste
Copy link
Contributor Author

no problem, thanx.
Perhaps you might want to change some very generic phpdoc I've added.

@jd327
Copy link
Collaborator

jd327 commented Oct 22, 2014

Is there a certain reason the interface is called "HashGenerator"? If not, I think "iHashids" or "HashidsInterface" might be more appropriate / simpler? (looking at http://php.net/manual/en/language.oop5.interfaces.php)

@Trismegiste
Copy link
Contributor Author

iHashids, definitely not, according PSR-0/2, classname must be in CamelCase.

HashidsInterface is a good option but I'm not fond of redundancy. The responsibility of the contract is for generating hashes so HashGenerator, I'm not good at finding names 😺

Say, for example, someone else wants to substitute your service by his, he can implement HashGenerator, the interface name speaks for itself.

jd327 pushed a commit that referenced this pull request Oct 27, 2014
@jd327 jd327 merged commit da15469 into vinkla:master Oct 27, 2014
@jd327
Copy link
Collaborator

jd327 commented Oct 27, 2014

Great, thanks for the pull request + explanation. Version bumped up + pushed out.

@jd327
Copy link
Collaborator

jd327 commented Nov 14, 2014

@Trismegiste, I still have to adjust the examples in the repo, because in their current state there's a require missing for the generator. Is there a good way to require the generator file to make the test files work (without doing the actual "require_once")?

@Trismegiste
Copy link
Contributor Author

Hello,

The best (and only) way to load required files is the autoloader generated from Composer.

I make a PR for this, my bad

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