Acyclic Visitor pattern #734 #753
Conversation
…n-patterns into acyclic-visitor
|
Is it #643 or should we do something? |
|
@Argyro-Sioziou Thanks for the PR 👍 |
There was a problem hiding this comment.
@Argyro-Sioziou I am done with initial review. Really neat implementation 👍 . There are a few changes though. Let me know when you are done with the changes or have any questions.
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(ConfigureForDosVisitor.class); | ||
|
|
||
| public Hayes() { |
There was a problem hiding this comment.
No need to explicitly defining default constructor
| } | ||
|
|
||
| /** | ||
| * Accept visitor |
There was a problem hiding this comment.
Suggestion: Accept all visitors, but honor only HayesVisitor.
| * [Visitor Pattern](../visitor/README.md) | ||
|
|
||
| ## Credits | ||
| * [Acyclic Visitor](http://condor.depaul.edu/dmumaugh/OOT/Design-Principles/acv.pdf) |
There was a problem hiding this comment.
Suggestion: How about a consequences section explaining the good and bad points.
The good
- No dependency cycles between class hierarchies
- No need to recompile all the visitors if a new one is added
- Does not cause compilation failure in existing visitors if class hierarchy has a new member
The bad
- Violates the principle of least surprise or Liskov's Substitution principle by showing that it can accept all visitors but actually only being interested in particular visitors
- Parallel hierarchy of visitors has to be created for all members in visitable class hierarchy
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * CongigureForDosVisitor class implements both zoom's and hayes' visit method |
There was a problem hiding this comment.
Typo: CongigureForDosVisitor
Suggestion: we can have an interface called AllModemVisitor which extends all visitor interfaces. In our case we have two visitors but in real applications we can have many visitors. So it becomes cumbersome to extend all every time we need to visit all modems.
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * CongigureForDosVisitor class implements both zoom's visit method for Unix |
There was a problem hiding this comment.
Typo: CongigureForDosVisitor
| ConfigureForDosVisitor conDos = new ConfigureForDosVisitor(); | ||
| Zoom zoom = mock(Zoom.class); | ||
|
|
||
| ((ZoomVisitor)conDos).visit(zoom); |
There was a problem hiding this comment.
No need to cast explicitly here, as ConfigureForDosVisitor already implements ZoomVisitor
| @Test | ||
| public void testVisitForHayes() { | ||
| ConfigureForDosVisitor conDos = new ConfigureForDosVisitor(); | ||
| Hayes hayes = mock(Hayes.class); |
| Hayes hayes = mock(Hayes.class); | ||
|
|
||
| Assertions.assertThrows(ClassCastException.class, () -> { | ||
| ((HayesVisitor)conUnix).visit(hayes); |
There was a problem hiding this comment.
This test case doesn't test anything, the class cast exception occurs because ConfigureForUnixVisitor does not implement HayesVisitor and still we are explicitly casting it.
We can remove this test case
| /** | ||
| * HayesVisitor test class | ||
| */ | ||
| public interface HayesVisitorTest { |
There was a problem hiding this comment.
Tip: No need to create test classes for interfaces. Adds no value. So we should remove all test cases for interfaces
| /** | ||
| * Modem test class | ||
| */ | ||
| public abstract class ModemTest { |
There was a problem hiding this comment.
Remove test case for interface.
|
@npathai Thank you a lot for the review. I have made the requested changes. I hope everything is fine now, otherwise let me know if there is anything else I shall change :) |
|
@Argyro-Sioziou I apologize for the delay, was on a vacation. I will review it by tomorrow and let you know if any more changes are required else I will merge it. |
|
Well done @Argyro-Sioziou 👍 |
|
@all-contributors please add @Argyro-Sioziou for code |
|
I've put up a pull request to add @Argyro-Sioziou! 🎉 |
Acyclic Visitor pattern implementation