Visitor pattern code example violate SOLID

[yonatankarp]

In the current example of the Visitor patter the UnitVisitor described as follow:

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:

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:

public interface Visitor {
  void visit(Unit unit);
}

Or alternatively if you want to make sure that you're type safe:

public interface Visitor<T extends Unit> {
  void visit(T t);
}

[yonatankarp]

By the way, happy to pick this fix up if you all agree that this is indeed an issue 😄

[iluwatar]

Thanks for raising the issue @yonatankarp. I'd like to see the improvement implemented.

[yonatankarp]

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:

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

[xyllq999]

I want to fix this issue. And I have pull a request, Thank you. @iluwatar

[xyllq999]

I have fixed this issue, pls review and merge this. Thanks! @iluwatar