Visitor #16
Visitor #16
Conversation
| - For example, for a shooter game, you can add a new operation to the game objects to calculate the damage they receive when hit by a bullet. | ||
| - This can be done without modifying the game objects themselves. | ||
| - The Visitor pattern enables adding functionality to existing objects without modifying them. | ||
| - A visitor is created separately, where the logic added. |
There was a problem hiding this comment.
Separately from what? And I think the word "is" is missing.
| ## Illustrative example | ||
|
|
||
| - Consider an analytics service that tracks site traffic for each day of the year. | ||
| - We're presented with a log class that contains last years site traffic for each day in an array. | ||
| - We can implement a visitor to perform calculations using this array, separate from the log class. | ||
| - The visitor is given access to the log class, then we are able to calculate the average, total, or maximum traffic for the year. |
There was a problem hiding this comment.
Placing an illustration before the problem statement doesn't make logical sense. An illustration is difficult to follow if I haven't first been provided context derived from a problem statement.
| - We would like a way of calculating various product discount types without modifying `Product` objects. | ||
| - We'll use the Visitor pattern to perform the discount calculations separately from products. |
There was a problem hiding this comment.
This is not an adequate problem statement. A problem statement takes this form: "Scenario >> Objective >> Problem.
So something like: "Assume you need to calculate discounts on various products (scenario and objective). These discounts are constantly shifting, so adding methods to our products to return a discount value would require constant editing of these product classes, which violates the Open Closed Principle (problem). The Visitor pattern can help us solve this problem (leads into the actual solution).
| - The operation's name and signature identifies the class that sends the Visit request to the visitor. | ||
| - That lets the visitor determine the concrete class of the element being visited. | ||
| - Then the visitor can access the element directly through its particular interface. | ||
| - The class we would like to add external functionality to. |
There was a problem hiding this comment.
You say class here but go on to define a protocol.
| protocol Product { | ||
| func accept(visitor: DiscountVisitor) | ||
| } |
There was a problem hiding this comment.
There is a fundamental problem with this protocol name. Will message you about it.
| func accept(visitor: DiscountVisitor) { | ||
| visitor.visit(macBook: self) | ||
| } |
There was a problem hiding this comment.
So it looks like this is absolutely correct, as GPT provided very similar code when I asked for a tutorial on the Visitor pattern. But if you or any dev submitted this code in a PR I would reject it. Method names should align with what they do. This method both accepts and visits a visitor so should be named: acceptAndVisit(_ visitor: DiscountVisitor).
| func visit(macBook: MacBookProduct) | ||
| func visit(vision: VisionProduct) |
There was a problem hiding this comment.
Do these methods form grammatical phrases or prepositional phrases? And given the answer, what is their correct method signature based on Swift API Guidelines?
|
|
||
| func accept(visitor: ProductVisitor) { | ||
| visitor.visit(macProduct: self) | ||
| func getDiscount() -> Double { |
There was a problem hiding this comment.
What does the word "get" add here?
There was a problem hiding this comment.
And since the method takes no params it could be a computed property, no?
| ObjectStructure: | ||
| class EmployeeDiscountVisitor: DiscountVisitor { | ||
| private let discountPercentage = 0.5 | ||
| private var discount: Double = 0.0 |
There was a problem hiding this comment.
You have one property storing discount. But two methods that can set it. So depending on which visit method was called last, the value will change. That is a sure recipe for a bug. I'm not sure how to fix it, but it needs to be fixed.
There was a problem hiding this comment.
I think the solution is need a dedicated Discount Visitor per product. So EmployeeDiscountVisitorForMac, EmployeeDiscountVisitorForVision. You can't have a generic EmployeeDiscountVisitor because you end up with code that lends itself to bugs.
| - Then the visitor can access the element directly through its particular interface. | ||
| #### Element: | ||
|
|
||
| The protocol that accepts a visitor for the objects we would like to add functionality to. |
There was a problem hiding this comment.
Protocols don't DO anything other than define behaviour. So saying "thee protocol that accepts a visitor" doesn't make sense.
| protocol ProductVisitor { | ||
| func accept(visitor: ProductVisitor) | ||
| protocol DiscountVisitorAccepting { | ||
| func accept(_ visitor: DiscountVisitor) |
There was a problem hiding this comment.
Prove to me from the API guidelines that this is the correct method signature rather than `acceptVisitor(_ visitor: DiscountVisitor).
There was a problem hiding this comment.
Not saying it is wrong wrong. But I need a solid argument.
There was a problem hiding this comment.
acceptVisitor is better from the call site
| func acceptAndVisit(_ macBookPro: MacBookProProduct) | ||
| func acceptAndVisit(_ visionPro: VisionProProduct) |
There was a problem hiding this comment.
This makes no sense. Visitors visit; they don't accept.
There was a problem hiding this comment.
Previously I said the accept method on DiscountVisitorAccepting should be acceptAndVisit because that is what the method ends up doing. But in hindsight it would be better to stick to the received pattern.
So acceptVisitor on DiscountVisitorAccepting, and then visitMacBook(_:) or visit(macbook:) on DiscountVisitor.
| private(set) var macBookProDiscount: Double = 0.0 | ||
| private(set) var visionProDiscount: Double = 0.0 |
| private(set) var macBookProDiscount: Double = 0.0 | ||
| private(set) var visionProDiscount: Double = 0.0 |
There was a problem hiding this comment.
Why are you defining the type here Double if there are default values? Swift can resolve the type for you. You are just adding unnecessary code.
No description provided.