-
Notifications
You must be signed in to change notification settings - Fork 177
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
Contracts should be defined by interface rather than abstract classes #2
Comments
The ReadModel component is affected as well. |
i2398: Create Scenario stub
There are cases where we use abstract classes to define contracts on our internal projects. For this open source project we see that adding some flexibility by using interfaces is probably better. So +1 for refactoring the contracts towards interfaces. We'll try to do it ourselves in the upcoming time, but a pull request would be very welcome! |
👍 |
1 similar comment
👍 |
I'm going to take this unless someone else has started. |
We haven't started yet, so go for it :) |
On a sidenote (we still need to update the README's), on freenode we have #qandidate where we can discuss stuff when slow-chatting on github doesn't work :) |
Fixed by #26 |
A good example is the EventDispatcher component (I haven't reviewed other components yet). The contact is defined by the
AbstractEventDispatcher
, which contains only public abstract methods.Such abstract class does not have any benefit over an interface IMO (there is no implementation code in it, only a contract), but it imposes a strong constraint on the actual implementation (PHP does not support multiple inheritance).
why not using interfaces for the contracts ?
thus, using a typehint on
AbstractEventDispatcher
looks weird IMO, it is likely that some devs could end up typehinting the implementation instead (I would be in favor of usingEventDispatcher
as the name of the interface and finding a different name for the simple implementation provided, but this is a separate topic for which http://verraes.net/2013/09/sensible-interfaces/ explains it well)The text was updated successfully, but these errors were encountered: