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

Template Method #18

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

Template Method #18

wants to merge 4 commits into from

Conversation

xsdc
Copy link
Owner

@xsdc xsdc commented Nov 14, 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.

Left some comments as if we come back to the pattern these comments will be useful. But as discussed, let's park this pattern for now.

Comment on lines +13 to +17
- The Template Method pattern starts with defining a protocol listing steps in a sequence of operations we would like to perform.

- We then provide a default implementation for each step in the sequence, as well as define the method that orchestrates the sequence.

- Subclasses are then created to override the default implementation as needed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clear, but do you understand the difference between subclassing and conforming to a protocol? Above you say "Subclasses are then created", but you don't subclass anything.

Comment on lines 23 to 31
- Let's assume the configuration is stored in memory in a way that is convenient for the frontend to display, but not in the expected format needed to send to the backend.

- Each watch series also has a different data structure we need to convert from.

- We'll use the Template Method pattern to define the steps needed to convert the configuration to the expected format.

- The pattern will allow us to cater to different watch series by providing a custom implementation for each series.

## Domain application
- Each series would have its own implementation of the steps needed to convert the configuration to the expected format.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent

Comment on lines 101 to 102
func process() {
mapDataForWatchCaseSize()
mapDataForWatchCaseMaterial()
mapDataForWatchBand()
mapDataForWatchBandSize()
mapDataForWatchEngraving()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that this process class calls all the same methods as the default implementation provided in your extension, why call it here again? This class can simply conform to the protocol and have an empty implementation and it will work just fine.

selectWatchBand()
selectWatchBandSize()
selectWatchEngraving()
class Series10AppleWatchConfiguration: AppleWatchConfigurationTemplate {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be a class or can it be a struct?

Comment on lines 111 to 106
func process() {
mapDataForWatchCaseSize()
mapDataForWatchCaseMaterial()
mapDataForWatchBand()
mapDataForWatchBandSize()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does your process method only call 4 of the 5 methods defined below?

selectWatchCaseMaterial()
selectWatchBand()
selectWatchBandSize()
class HermèsSeries10AppleWatchConfiguration: AppleWatchConfigurationTemplate {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a class?

func selectWatchCaseMaterial() {
print("Configure default Apple Watch case material.")
func mapDataForWatchCaseSize() {
print("Implementation for converting the front data structure for the Apple Watch Series 10 case size.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that this is the default implementation, suggest editing all these methods in this extension to read: "Default implementation for..."

Also, what is "front data"? "frontend data" perhaps?

Also, do you mean: "for concerting INTO the frontend data structure..."?

- The template method calls primitive operations as well as operations defined in AbstractClass or those of other objects.
#### Abstract class:

- Defines the steps in the sequence needed to map the configuration to the expected format.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't define any sequence; you just define a set of methods to call. Remove the word sequence.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Scrap that. We discussed this. Your code is missing the critical part which preserves the sequence.

Comment on lines 111 to 106
func process() {
mapDataForWatchCaseSize()
mapDataForWatchCaseMaterial()
mapDataForWatchBand()
mapDataForWatchBandSize()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

By implementing your own process method you are loosing the templates ability to define the sequence. But we discussed this earlier.

@xsdc xsdc force-pushed the template_method branch from 4f6750d to 0c1ceee Compare January 6, 2025 05:58
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