Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork333
update decoupled-example with abstraction layer#770
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This PR needs some addition work.
| from ..user.repositories import UserRepositoryMeta | ||
| class AggregationServiceMeta(metaclass=abc.ABCMeta): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| classAggregationServiceMeta(metaclass=abc.ABCMeta): | |
| classAggregationService(metaclass=abc.ABCMeta): |
Meta suffix might give it a wrong impression that this is a metaclass. Let's avoid ambiguous naming.
| self.user_repository = user_repository | ||
| self.photo_repository = photo_repository | ||
| class AggregationService(AggregationServiceMeta): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| classAggregationService(AggregationServiceMeta): | |
| classAggregationServiceImpl(AggregationService): |
or
| classAggregationService(AggregationServiceMeta): | |
| classMemoryAggregationService(AggregationService): |
See comment above. This way it would be much more clear about implementation specifics of theAggregationService interface.
| def __init__(self, user_repository: UserRepositoryMeta, photo_repository: PhotoRepositoryMeta): | ||
| self.user_repository: UserRepositoryMeta = user_repository | ||
| self.photo_repository: PhotoRepositoryMeta = photo_repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Interfaces generally has no__init__, it's an implementation detail.
| def__init__(self,user_repository:UserRepositoryMeta,photo_repository:PhotoRepositoryMeta): | |
| self.user_repository:UserRepositoryMeta=user_repository | |
| self.photo_repository:PhotoRepositoryMeta=photo_repository |
| │ ├── abstraction | ||
| │ │ ├── __init__.py | ||
| │ │ ├── analytics | ||
| │ │ │ ├── __init__.py | ||
| │ │ │ └── services.py | ||
| │ │ ├── photo | ||
| │ │ │ ├── __init__.py | ||
| │ │ │ └── repositories.py | ||
| │ │ └── user | ||
| │ │ ├── __init__.py | ||
| │ │ └── repositories.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For such simple app example, I'd move everything intointerfaces.py to keep app structure from bloating.
| @abc.abstractmethod | ||
| def get_photos(self, user_id): | ||
| """Must be implemented in order to instantiate.""" | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| pass |
Coverage report treatspass as a code. Which never executes forabstractmethods.
Hi
The decoupled-example code is good, but it lack one important component of DIP rule(https://www.cs.utexas.edu/users/downing/papers/DIP-1996.pdf)
I mean the "Abstraction" layer which can help make "user" and "photo" compliant to abstract classes of their own and "analytics" depends on these abstract classes, and even for "analytics" can be extracted one abstract class, then formain.py (the top-level service aggregation) can only depend on these three abstract class, finally "ApplicationContainer" is for connecting the embodied containers("user", "photo", "analytics") tomain.py.
The update code is better to demostrate DIP rule for developer.
I also update docs part.
Please check.
Thanks.