Skip to content

Conversation

@MikePresman
Copy link

@MikePresman MikePresman commented Jul 15, 2021

Modified Files:

  • 'app/data/__init.py'
  • 'main.py'
  • 'app/routers/v2.py'

Modified the file 'app/data/init.py' to implement the AGGREGATION pattern.

Replaces the original way of accessing the DATA_SOURCES dictionary. Rather than having the dictionary be a globally accessible object the dictionary is now only accessible through an instance of the class 'DataSource' which acts as the root in the boundary.

The boundary of 'DataSource' would thus include the __DATA_SOURCE dictionary which is now encapsulated, which holds references to the services (NYT, CSBS, Jhu).

As such the dictionary is now accessible through a getter def get_data_source(self, source: str) -> LocationService:
And the dictionary can also be appended through a setter def add_data_source(self, source: str, reference_to_source: LocationService) -> None:

Also to ensure consistency in functionality with how sources are accessed in each request in 'v2.py' the class (DataSource) implements a def get_all_sources(self) -> dict: method which returns the dictionary. This prevents breaking any functionality in 'v2.py'.

For example instead of an endpoint accessing a source via
locations = await request.state.source.get_all()

It is now accessed with a minor change of
locations = await request.state.source.get_all_sources()[source].get_all()

Minor changes in 'main.py' are implemented in order to set the default source.
Before: source = data_source(request.query_params.get("source", default="jhu"))
After: source = DataSource()
source.get_data_source(request.query_params.get("source", default = "jhu"))

Again, this implementation encourages aggregation which in turn enforces the objects inside the boundary to be only accessible through the root. This ensures invariants such as the fact that only one source should be returned through the getter.

…or a deeper level of abstraction while encapsulating data sources in such a way that the data sources and the boundary can only be accessed through the root
def __init__(self):
self.__DATA_SOURCES['jhu'] = JhuLocationService()
self.__DATA_SOURCES['csbs'] = CSBSLocationService()
self.__DATA_SOURCES['nyt'] = NYTLocationService()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I appreciate that you took the time to explain what you were doing and why.
But I still don't really see the value of this refactor.

For much the same reason as I said in your other PR.
#349 (comment)

DataSource doesn't need to be a class. What is the point of creating a DataSource instance considering all the instances have the exact same sources?

There are ways to make the __init__ method return the same instance singleton, but this won't do that.
Although I'm not actually recommending that (just an FYI).

@ExpDev07 ExpDev07 closed this Aug 24, 2022
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