Skip to content

Conversation

@MikePresman
Copy link

@MikePresman MikePresman commented Jul 17, 2021

Files Changes

  • main.py
  • httputils.py

What has Changed?
HttpUtils.py now follows an aggregation pattern where instead of having CLIENT_SESSION act as a global variable it is now part of a boundary where it is only accessible through the root, where the root is an instance of the class 'Session'. This paradigm also applies to LOGGER where LOGGER is also part of the boundary.

Because I have applied aggregation here, both LOGGER and CLIENT_SESSION are now encapsulated since their instance does not need to be seen individually but only through the lens of an instance of a Session.

The logic and approach of setup_client_session() and teardown_client_session() remain. What only changes is the encapsulation, and the aggregation of the objects that manage those methods.

Why
This change seems appropriate as this change encourages keeping one client_session alive per user and managing that session through an instance of a class rather than passing that session where a lack of encapsulation could cause confusion. As such adding a boundary on the CLIENT_SESSION and LOGGER makes perfect sense here through the work of aggregation.

@Kilo59
Copy link
Collaborator

Kilo59 commented Jul 18, 2021

I appreciate the effort.

Their certainly is lot about this application that can and should be improved but I don't see how this particular refactor makes the the project better.

# ############
LOGGER = logging.getLogger("api")

session = Session()
Copy link
Collaborator

@Kilo59 Kilo59 Jul 18, 2021

Choose a reason for hiding this comment

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

What's the point of creating an instance of Session?

If there's only meant to be one Client Session for the lifetime of the application why are we tempting developers to create multiple Session instances by defining a class in the first place?

Seems everything could be a class method or attribute.
But at that point why have a class at all?

Maybe just to act as a container for http functions and attributes so we only have to import one object from the module. That's certainly reasonable.

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