Repository: iluwatar/java-design-patterns
Issue: Visitor pattern code example violate SOLID 
Labels: epic: pattern
type: enhancement

In the current example of the Visitor patter the `UnitVisitor` described as follow:

```java
public interface UnitVisitor {

  void visitSoldier(Soldier soldier);

  void visitSergeant(Sergeant sergeant);

  void visitCommander(Commander commander);
}
```

This interface violate the interface segregation principle as shown later on when implementing `CommanderVisitor`, `SergeantVisitor` and `SoldierVisitor` as each one of them have methods that are not relevant for the instance.

The problem becomes even worst if someone will call be mistake the wrong method in the Unit concrete instances. For example:

```java
public class Soldier extends Unit {

  public Soldier(Unit... children) {
    super(children);
  }

  @Override
  public void accept(UnitVisitor visitor) {
    visitor.visitCommander(this); // <-- Wrongly calling the commander method
    super.accept(visitor);
  }

  @Override
  public String toString() {
    return "soldier";
  }
}
```

This should be fixed by having a single method in the `UnitVisitor` interface as follow:
```java
public interface Visitor {
  void visit(Unit unit);
}
```

Or alternatively if you want to make sure that you're type safe:

```java
public interface Visitor<T extends Unit> {
  void visit(T t);
}
```

Issue comments:
By the way, happy to pick this fix up if you all agree that this is indeed an issue 😄 
Thanks for raising the issue @yonatankarp. I'd like to see the improvement implemented.
After reading a bit deeper about this design pattern in multiple sources (see below), it seems like the design itself is (almost) correctly implemented, but the example is confusing.

In the design, each visitor should have a `visit` method for each of the elements (e.g. `DisplayUnitVisitor`), however, in the current example we have 3 different visitors that are doing the same thing (logging that we visit the specific element or nothing if this is not the correct element).

I would suggest making the following changes to the code:

- Amend `UnitVisitor` methods to overload the `visit` method with the different elements in our system (`Commander`, `Sergeant` and `Solider`) as follow:
```java
public interface UnitVisitor {

  void visit(Soldier soldier);

  void visit(Sergeant sergeant);

  void visit(Commander commander);

}
```

- Create a single concrete implementation of the `UnitVisitor` interface (e.g. `LogUnitVisitor`). There's an option to have multiple visitors - however IMHO it will complicate the example for no good reason.

- Add `Consequences` with the disclaimer that this pattern requires changes to each visitor when adding a new element to the system.

**sources:**

- https://www.baeldung.com/java-visitor-pattern
- https://www.tutorialspoint.com/design_pattern/visitor_pattern.htm
- https://en.wikipedia.org/wiki/Visitor_pattern

I want to fix this issue. And I have pull a request, Thank you. @iluwatar 
I have fixed this issue, pls review and merge this. Thanks! @iluwatar 

Pull request review comments:

Patch that resolved the issue:
diff --git a/visitor/README.md b/visitor/README.md
index 38df87d284cc..dae82966bf0f 100644
--- a/visitor/README.md
+++ b/visitor/README.md
@@ -72,7 +72,7 @@ public class Commander extends Unit {
 
   @Override
   public void accept(UnitVisitor visitor) {
-    visitor.visitCommander(this);
+    visitor.visit(this);
     super.accept(visitor);
   }
 
@@ -90,7 +90,7 @@ public class Sergeant extends Unit {
 
   @Override
   public void accept(UnitVisitor visitor) {
-    visitor.visitSergeant(this);
+    visitor.visit(this);
     super.accept(visitor);
   }
 
@@ -108,7 +108,7 @@ public class Soldier extends Unit {
 
   @Override
   public void accept(UnitVisitor visitor) {
-    visitor.visitSoldier(this);
+    visitor.visit(this);
     super.accept(visitor);
   }
 
@@ -126,17 +126,17 @@ Here are then some concrete visitors.
 public class CommanderVisitor implements UnitVisitor {
 
   @Override
-  public void visitSoldier(Soldier soldier) {
+  public void visit(Soldier soldier) {
     // Do nothing
   }
 
   @Override
-  public void visitSergeant(Sergeant sergeant) {
+  public void visit(Sergeant sergeant) {
     // Do nothing
   }
 
   @Override
-  public void visitCommander(Commander commander) {
+  public void visit(Commander commander) {
     LOGGER.info("Good to see you {}", commander);
   }
 }
@@ -145,17 +145,17 @@ public class CommanderVisitor implements UnitVisitor {
 public class SergeantVisitor implements UnitVisitor {
 
   @Override
-  public void visitSoldier(Soldier soldier) {
+  public void visit(Soldier soldier) {
     // Do nothing
   }
 
   @Override
-  public void visitSergeant(Sergeant sergeant) {
+  public void visit(Sergeant sergeant) {
     LOGGER.info("Hello {}", sergeant);
   }
 
   @Override
-  public void visitCommander(Commander commander) {
+  public void visit(Commander commander) {
     // Do nothing
   }
 }
@@ -164,17 +164,17 @@ public class SergeantVisitor implements UnitVisitor {
 public class SoldierVisitor implements UnitVisitor {
 
   @Override
-  public void visitSoldier(Soldier soldier) {
+  public void visit(Soldier soldier) {
     LOGGER.info("Greetings {}", soldier);
   }
 
   @Override
-  public void visitSergeant(Sergeant sergeant) {
+  public void visit(Sergeant sergeant) {
     // Do nothing
   }
 
   @Override
-  public void visitCommander(Commander commander) {
+  public void visit(Commander commander) {
     // Do nothing
   }
 }
diff --git a/visitor/src/main/java/com/iluwatar/visitor/Commander.java b/visitor/src/main/java/com/iluwatar/visitor/Commander.java
index a7c0a0252ad2..9fd10dcd4b77 100644
--- a/visitor/src/main/java/com/iluwatar/visitor/Commander.java
+++ b/visitor/src/main/java/com/iluwatar/visitor/Commander.java
@@ -32,9 +32,13 @@ public Commander(Unit... children) {
     super(children);
   }
 
+  /**
+   * Accept a Visitor.
+   * @param visitor UnitVisitor to be accepted
+   */
   @Override
   public void accept(UnitVisitor visitor) {
-    visitor.visitCommander(this);
+    visitor.visit(this);
     super.accept(visitor);
   }
 
diff --git a/visitor/src/main/java/com/iluwatar/visitor/CommanderVisitor.java b/visitor/src/main/java/com/iluwatar/visitor/CommanderVisitor.java
index cf91623bf2b1..f26a4154aada 100644
--- a/visitor/src/main/java/com/iluwatar/visitor/CommanderVisitor.java
+++ b/visitor/src/main/java/com/iluwatar/visitor/CommanderVisitor.java
@@ -31,18 +31,30 @@
 @Slf4j
 public class CommanderVisitor implements UnitVisitor {
 
+  /**
+   * Soldier Visitor method.
+   * @param soldier Soldier to be visited
+   */
   @Override
-  public void visitSoldier(Soldier soldier) {
+  public void visit(Soldier soldier) {
     // Do nothing
   }
 
+  /**
+   * Sergeant Visitor method.
+   * @param sergeant Sergeant to be visited
+   */
   @Override
-  public void visitSergeant(Sergeant sergeant) {
+  public void visit(Sergeant sergeant) {
     // Do nothing
   }
 
+  /**
+   * Commander Visitor method.
+   * @param commander Commander to be visited
+   */
   @Override
-  public void visitCommander(Commander commander) {
+  public void visit(Commander commander) {
     LOGGER.info("Good to see you {}", commander);
   }
 }
diff --git a/visitor/src/main/java/com/iluwatar/visitor/Sergeant.java b/visitor/src/main/java/com/iluwatar/visitor/Sergeant.java
index d92cfce3fcc5..96d1531be506 100644
--- a/visitor/src/main/java/com/iluwatar/visitor/Sergeant.java
+++ b/visitor/src/main/java/com/iluwatar/visitor/Sergeant.java
@@ -32,9 +32,13 @@ public Sergeant(Unit... children) {
     super(children);
   }
 
+  /**
+   * Accept a Visitor.
+   * @param visitor UnitVisitor to be accepted
+   */
   @Override
   public void accept(UnitVisitor visitor) {
-    visitor.visitSergeant(this);
+    visitor.visit(this);
     super.accept(visitor);
   }
 
diff --git a/visitor/src/main/java/com/iluwatar/visitor/SergeantVisitor.java b/visitor/src/main/java/com/iluwatar/visitor/SergeantVisitor.java
index f4bf23d5abc2..66fe05f129d6 100644
--- a/visitor/src/main/java/com/iluwatar/visitor/SergeantVisitor.java
+++ b/visitor/src/main/java/com/iluwatar/visitor/SergeantVisitor.java
@@ -31,18 +31,30 @@
 @Slf4j
 public class SergeantVisitor implements UnitVisitor {
 
+  /**
+   * Soldier Visitor method.
+   * @param soldier Soldier to be visited
+   */
   @Override
-  public void visitSoldier(Soldier soldier) {
+  public void visit(Soldier soldier) {
     // Do nothing
   }
 
+  /**
+   * Sergeant Visitor method.
+   * @param sergeant Sergeant to be visited
+   */
   @Override
-  public void visitSergeant(Sergeant sergeant) {
+  public void visit(Sergeant sergeant) {
     LOGGER.info("Hello {}", sergeant);
   }
 
+  /**
+   * Commander Visitor method.
+   * @param commander Commander to be visited
+   */
   @Override
-  public void visitCommander(Commander commander) {
+  public void visit(Commander commander) {
     // Do nothing
   }
 }
diff --git a/visitor/src/main/java/com/iluwatar/visitor/Soldier.java b/visitor/src/main/java/com/iluwatar/visitor/Soldier.java
index ae27bd9c5555..ef7645891519 100644
--- a/visitor/src/main/java/com/iluwatar/visitor/Soldier.java
+++ b/visitor/src/main/java/com/iluwatar/visitor/Soldier.java
@@ -32,9 +32,13 @@ public Soldier(Unit... children) {
     super(children);
   }
 
+  /**
+   * Accept a Visitor.
+   * @param visitor UnitVisitor to be accepted
+   */
   @Override
   public void accept(UnitVisitor visitor) {
-    visitor.visitSoldier(this);
+    visitor.visit(this);
     super.accept(visitor);
   }
 
diff --git a/visitor/src/main/java/com/iluwatar/visitor/SoldierVisitor.java b/visitor/src/main/java/com/iluwatar/visitor/SoldierVisitor.java
index eb931e23858d..2ed8b66b980d 100644
--- a/visitor/src/main/java/com/iluwatar/visitor/SoldierVisitor.java
+++ b/visitor/src/main/java/com/iluwatar/visitor/SoldierVisitor.java
@@ -31,18 +31,30 @@
 @Slf4j
 public class SoldierVisitor implements UnitVisitor {
 
+  /**
+   * Soldier Visitor method.
+   * @param soldier Soldier to be visited
+   */
   @Override
-  public void visitSoldier(Soldier soldier) {
+  public void visit(Soldier soldier) {
     LOGGER.info("Greetings {}", soldier);
   }
 
+  /**
+   * Sergeant Visitor method.
+   * @param sergeant Sergeant to be visited
+   */
   @Override
-  public void visitSergeant(Sergeant sergeant) {
+  public void visit(Sergeant sergeant) {
     // Do nothing
   }
 
+  /**
+   * Commander Visitor method.
+   * @param commander Commander to be visited
+   */
   @Override
-  public void visitCommander(Commander commander) {
+  public void visit(Commander commander) {
     // Do nothing
   }
 }
diff --git a/visitor/src/main/java/com/iluwatar/visitor/UnitVisitor.java b/visitor/src/main/java/com/iluwatar/visitor/UnitVisitor.java
index 29aefc1adbbf..08cce7babca8 100644
--- a/visitor/src/main/java/com/iluwatar/visitor/UnitVisitor.java
+++ b/visitor/src/main/java/com/iluwatar/visitor/UnitVisitor.java
@@ -28,10 +28,10 @@
  */
 public interface UnitVisitor {
 
-  void visitSoldier(Soldier soldier);
+  void visit(Soldier soldier);
 
-  void visitSergeant(Sergeant sergeant);
+  void visit(Sergeant sergeant);
 
-  void visitCommander(Commander commander);
+  void visit(Commander commander);
 
 }


Test patch:
diff --git a/visitor/src/test/java/com/iluwatar/visitor/CommanderTest.java b/visitor/src/test/java/com/iluwatar/visitor/CommanderTest.java
index 1e68955509f5..a6054f7244f3 100644
--- a/visitor/src/test/java/com/iluwatar/visitor/CommanderTest.java
+++ b/visitor/src/test/java/com/iluwatar/visitor/CommanderTest.java
@@ -42,7 +42,7 @@ public CommanderTest() {
 
   @Override
   void verifyVisit(Commander unit, UnitVisitor mockedVisitor) {
-    verify(mockedVisitor).visitCommander(eq(unit));
+    verify(mockedVisitor).visit(eq(unit));
   }
 
 }
\ No newline at end of file
diff --git a/visitor/src/test/java/com/iluwatar/visitor/SergeantTest.java b/visitor/src/test/java/com/iluwatar/visitor/SergeantTest.java
index 77fcbafe6a52..4d430f5beedb 100644
--- a/visitor/src/test/java/com/iluwatar/visitor/SergeantTest.java
+++ b/visitor/src/test/java/com/iluwatar/visitor/SergeantTest.java
@@ -42,7 +42,7 @@ public SergeantTest() {
 
   @Override
   void verifyVisit(Sergeant unit, UnitVisitor mockedVisitor) {
-    verify(mockedVisitor).visitSergeant(eq(unit));
+    verify(mockedVisitor).visit(eq(unit));
   }
 
 }
\ No newline at end of file
diff --git a/visitor/src/test/java/com/iluwatar/visitor/SoldierTest.java b/visitor/src/test/java/com/iluwatar/visitor/SoldierTest.java
index a59eca6b450d..f5f31767ee9a 100644
--- a/visitor/src/test/java/com/iluwatar/visitor/SoldierTest.java
+++ b/visitor/src/test/java/com/iluwatar/visitor/SoldierTest.java
@@ -42,7 +42,7 @@ public SoldierTest() {
 
   @Override
   void verifyVisit(Soldier unit, UnitVisitor mockedVisitor) {
-    verify(mockedVisitor).visitSoldier(eq(unit));
+    verify(mockedVisitor).visit(eq(unit));
   }
 
 }
\ No newline at end of file
diff --git a/visitor/src/test/java/com/iluwatar/visitor/VisitorTest.java b/visitor/src/test/java/com/iluwatar/visitor/VisitorTest.java
index 694be7ab23f8..dbc342806685 100644
--- a/visitor/src/test/java/com/iluwatar/visitor/VisitorTest.java
+++ b/visitor/src/test/java/com/iluwatar/visitor/VisitorTest.java
@@ -97,7 +97,7 @@ public VisitorTest(
 
   @Test
   void testVisitCommander() {
-    this.visitor.visitCommander(new Commander());
+    this.visitor.visit(new Commander());
     if (this.commanderResponse.isPresent()) {
       assertEquals(this.commanderResponse.get(), appender.getLastMessage());
       assertEquals(1, appender.getLogSize());
@@ -106,7 +106,7 @@ void testVisitCommander() {
 
   @Test
   void testVisitSergeant() {
-    this.visitor.visitSergeant(new Sergeant());
+    this.visitor.visit(new Sergeant());
     if (this.sergeantResponse.isPresent()) {
       assertEquals(this.sergeantResponse.get(), appender.getLastMessage());
       assertEquals(1, appender.getLogSize());
@@ -115,7 +115,7 @@ void testVisitSergeant() {
 
   @Test
   void testVisitSoldier() {
-    this.visitor.visitSoldier(new Soldier());
+    this.visitor.visit(new Soldier());
     if (this.soldierResponse.isPresent()) {
       assertEquals(this.soldierResponse.get(), appender.getLastMessage());
       assertEquals(1, appender.getLogSize());