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

Proxy #25

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Proxy #25

wants to merge 8 commits into from

Conversation

xsdc
Copy link
Owner

@xsdc xsdc commented Nov 19, 2024

No description provided.

Copy link
Collaborator

@ryanbrear ryanbrear left a comment

Choose a reason for hiding this comment

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

As it stands I just don't understand this design pattern. Also your example of auth being coupled with a cart being emptied is super odd. Who would ever do this?

Suggest reworking this doc so first time readers can "get it". I am a first time reader, and I don't.

Comment on lines 80 to 103
private func checkIfAuthenticated() -> Bool {
return false
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't make sense. Why return false in this method. In example code its perfectly legitimate to use comments to explain what a method might do and not add all the code. So // fetch authentication state here.

Copy link
Collaborator

@ryanbrear ryanbrear left a comment

Choose a reason for hiding this comment

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

Okay, I GET Proxy now. Much better.


## Domain application
- The proxy pattern allows us to separate the analytics logic from the `Bag` class, conforming to the single responsibility principle.
Copy link
Collaborator

Choose a reason for hiding this comment

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

adhering to, not conforming to. Use conforming for objects which conform to protocols.

logAnalyticsEvent(with: "allProductsRemovedFromBag")
}

private func logAnalyticsEvent(with id: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

One of the sections in API Guidelines is title: "Compensate for weak type information to clarify a parameter’s role"

Read it again.

The call site of this method reads: logAnalyticsEvent(with: {someStringGoesHere}. How is the consumer supposed to know what that String needs to be. Should it be a description, an ID, or something else.

In cases where argument labels are prepositions (with, for, etc) and the TYPE is a basic like String, Int, AnyObject, it helps the reader to append more info to the preposition to make the call site clearer.

So in this case it should read withID id: String

Note: another approach is to create an explicit type like UserID, or just create a typealias: typealias ID = String. This way the type communicates to consumers what needs to be passed in, and your method can take the form: user(with id: ID).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Tempted to go with the typealias, but withID id: String is better for the reader

- Common uses include controlling access to a class, adding logging, or to add caching.
- It is often used in situations where you want to add behavior to a class without changing the class itself.

- It is used in situations where you want to add behavior to a class without changing the class itself.
Copy link
Collaborator

Choose a reason for hiding this comment

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

behaviour


- The object that we want to add behavior to.

- In our scenario, this is an existing `Bag` class that we would like add analytics to.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Grammar

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