How to use Visitor pattern for substituting "instanceof"

Question

I'm a little bit unfriendly with Visitor pattern, but I have a task that needs to have Visitor implementation (if I want to avoid "instanceof" checks).

I have a class that is a wrapper for several gwt elements: Label, Panel, Widget (can be checkbox, listbox, textbox etc.). I use an array as a collection of alike parts of UI. E.g. Label + checkbox, Label + text box; Label + button etc.

Some of elements are constructed in a different way (part of another class derived from, for example, Panel). So as a result I have two constructors that are the same, but in one place overloaded method is used. I can merge these constructors and check the element using "instanceof" inside mentioned method. But I dislike this solution and want to substitute it using Visitor pattern. To tell the truth, I don't know how to do it and hope for you help.

Here is an example of what I have:

public class MyWidgets {
    private String stringLabel;
    private Widget widget;
    private Panel panel;

    public MyWidgets(String stringLabel, Widget widget) {
      this.stringLabel = stringLabel;
      this.widget = widget;

      initPanel(stringLabel, widget);
    }

    public MyWidgets(ConstructedClass cs, Widget widget) {
       this.widget = widget;

       initPanel(cs, widget);
    }

    private initPanel(String label, Widget widget) {
      panel = SomeStaticUtilityClass.initPanel(new Label(label), widget);
    }

    private initPanel(ConstructedClass cs, Widget widget) {
      panel = SomeStaticUtilityClass(cs, widget);
    }
}

Something like this (I tried to make it maximum abstract, in reality it is more difficult).

So I have a solution using "instanceof":

private initPanel(Object object, Widget widget) {
  if(object instanceof String) {
    panel = SomeStaticUtilityClass.initPanel(new Label(label), widget);
  }
  if(object instanceof ConstructedClass) {
    panel = SomeStaticUtilityClass.initPanelFromObject(cs, widget);
  }
}

I want to be saved from "instanceof" and leave just one constructor and even, if it is possible, one init method without its overloaded version. Thank you in advance for you suggestions, help.

P.S> I repeat, that the class above is fabricated, and looks like some misunderstanding especially with this String label :)

Answer 1

IMO, your existing solution, with two constructors, is fine.

You could use the strategy pattern and have your constructor take an instance of some PanelProvider interface instead of Object. This interface would have the following method:

Panel createPanel(Widget widget);

. Clients would pass an instance of StringPanelProvider or an instance of ConstructedClassPanelProvider to the constructor. Your constructor would thus look like:

public MyWidgets(PanelProvider panelProvider, Widget widget) {
   this.widget = widget;
   this.panel = panelProvider.createPanel(widget);
}

And the StringPanelProvider implementation would look like

public class StringPanelProvider implements PanelProvider {

    private String s;

    public StringPanelProvider(String s) {
        this.s = s;
    }

    @Override
    public Panel createPanel(Widget widget) {
        return SomeStaticUtilityClass.initPanel(new Label(s), widget);
    }
}

The ConstructedClassPanelProvider would look the same.

If you really want to use the Visitor pattern then you'll have to modify the above a little bit:

public interface Visitable {
    void accept(Visitor visitor);
}

public interface Visitor {
    void stringVisited(String s);
    void constructedClassVisited(ConstructedClass cs);
}

public class StringVisitable {
    private String s;

    public StringVisitable(String s) {
        this.s = s;
    }

    void accept(Visitor visitor) {
        visitor.stringVisited(s);
    }
}

// similar for ConstructedClassVisitable

public MyWidgets(Visitable visitable, final Widget widget) {
   this.widget = widget;
   visitable.accept(new Visitor() {
       public void stringVisited(String s) {
           panel = SomeStaticUtilityClass.initPanel(new Label(label), widget);
       }

       public void constructedClassVisited(ConstructedClass cs) {
           panel = SomeStaticUtilityClass.initPanelFromObject(cs, widget);
       }
   });
}

But this looks like overengineering to me.

Answer 2

One implementation using the visitor pattern is as follows:

public interface ConstructionArgVisitor {
    void visit(LabelText text);

    void visit(ConstructedClass clazz);
}

public interface ConstructionArg {
    void accept(ConstructionArgVisitor visitor);
}

public class LabelText implements ConstructionArg {
    private final String text;

    public LabelText(String str) {
        this.text = str;
    }

    @Override
    public void accept(ConstructionArgVisitor visitor) {
        visitor.visit(this);
    }

    public String getString() {
        return this.text;
    }
}

public class ConstructedClass implements ConstructionArg {
    @Override
    public void accept(ConstructionArgVisitor visitor) {
        visitor.visit(this);
    }
}

public class MyWidgets implements ConstructionArgVisitor {
    private String stringLabel;
    private Widget widget;
    private Panel panel;

    public MyWidgets(ConstructionArg constructionArg, Widget widget) {
        this.widget = widget;
        constructionArg.accept(this);
    }

    @Override
    public void visit(LabelText labelText) {
        this.stringLabel = labelText.getString();
        this.panel = SomeStaticUtilityClass.initPanel(new Label(labelText.getString()), this.widget);
    }

    @Override
    public void visit(ConstructedClass clazz) {
        this.panel = SomeStaticUtilityClass.initPanelFromObject(clazz, this.widget);
    }
}

This solution is very similar to JB Nizet's one. The difference between this implementation's ConstructorArgVisitor and JB Nizet's Visitor interface is the method names. The visit method is overloaded in ConstructorArgVisitor, whereas in JB Nizet's Visitor, the method names contain the type in them (e.g., stringVisited). Overloading the visit method more closely resembles the example of the visitor pattern on the Wikipedia page.

I agree with JB Nizet that using the visitor pattern may be a bit overengineered; however, if you use a PanelProvider as JB Nizet recommends, unless you know the argument is a String or ConstructedClass ahead of time, you may still need to do an instanceof check, which you are trying to avoid.

Now this is my personal preference, so you can disregard if you like: Try not to do work in the constructor as Misko Hevery recommends in "Flaw: Constructor does Real Work." For example, you can move the construction logic into a factory. The following uses a modified version of the visitor pattern above:

public interface ConstructionArgVisitor<T> {
    T visit(LabelText text);

    T visit(ConstructedClass clazz);
}

public interface ConstructionArg {
    <T> T accept(ConstructionArgVisitor<T> visitor);
}

public class LabelText implements ConstructionArg {
    private final String text;

    public LabelText(String str) {
        this.text = str;
    }

    @Override
    public <T> T accept(ConstructionArgVisitor<T> visitor) {
        return visitor.visit(this);
    }

    public String getString() {
        return this.text;
    }
}

public class ConstructedClass implements ConstructionArg {
    @Override
    public <T> T accept(ConstructionArgVisitor<T> visitor) {
        return visitor.visit(this);
    }
}

public class MyWidgetsFactory implements ConstructionArgVisitor<MyWidgets> {
    private final Widget widget;

    public MyWidgetsFactory(Widget widget) {
        this.widget = widget;
    }

    public MyWidgets createMyWidgets(ConstructionArg constructionArg) {
        return constructionArg.accept(this);
    }

    @Override
    public MyWidgets visit(LabelText text) {
        return new MyWidgets(text.getString(), this.widget, SomeStaticUtilityClass.initPanel(
                new Label(text.getString()), this.widget));
    }

    @Override
    public MyWidgets visit(ConstructedClass clazz) {
        return new MyWidgets(null, this.widget, SomeStaticUtilityClass.initPanelFromObject(clazz, this.widget));
    }
}

public class MyWidgets {
    private final String stringLabel;
    private final Widget widget;
    private final Panel panel;

    public MyWidgets(String stringLabel, Widget widget, Panel panel) {
        this.stringLabel = stringLabel;
        this.widget = widget;
        this.panel = panel;
    }
}

public static void main(String[] args) {
    final Widget widget = ...;
    final MyWidgetsFactory factory = new MyWidgetsFactory(widget);

    // create MyWidgets from label text
    final String str = ...;
    final MyWidgets labelWidget = factory.createMyWidgets(new LabelText(str));

    // create MyWidgets from constructed class
    final ConstructedClass clazz = ...;
    final MyWidgets constructedClassWidget = factory.createMyWidgets(clazz);
}

I also see that you're calling a static method during construction. Even though in a lot of codebases GUIs are notoriously hard to test, you may want to read "Flaw: Brittle Global State & Singletons" and "Guide: Writing Testable Code."