Refactoring code in Java, alternatives to large if statement

Question

I'm refactoring some code in a project I'm working on and I ran into a large if/else if statement that follows the format:

if (changer instanceof AppleChanger)
{
   panel = new ApplePanel();
}
else if (changer instanceof OrangeChanger)
{
   panel = new OrangePanel();
}

Now my first impulse was to refactor it using polymorphism to have it appear like

panel = changer.getChangerPanel();

However unfortunately the class package doesn't have access to the panel package.

My next impulse was to create a PanelChooser class with an overloaded method:

PanelChooser.getPanel(changer);

//Overloaded Method
public Panel getPanel(OrangeChanger changer)
{
   Panel orangePanel = new OrangePanel();
   return orangePanel;
}
public Panel getPanel(AppleChanger changer)
{
   Panel applePanel = new ApplePanel();
   return applePanel;
}

Is this a good solution or is there a better way to solve this?

i suspect the second one won't work because you will have to explicitly cast the changer into a subclass for java to identify the method to invoke. you could use a registry class/ map (so map changers to panels and look up with the passed instance) perhaps. — aishwarya, Jan 10 '12 at 08:41

score 13 · Accepted Answer · answered Jan 10 '12 at 08:57

13

The fundamental 'problem' here is that you have parallel class hierarchies. You're not going to be able to replace that if statement without some fairly heavy refactoring. Some suggestions are on c2 wiki.

The best you can do, and possibly a perfectly fine solution, is to move the if statement into a 'factory' class and make sure it's not duplicated anywhere else.

answered Jan 10 '12 at 08:57

artbristol

32,010
5
70
103

1

+1 for identification of parallel class hierarchies. Definitely a case for Factories. – earcam Jan 10 '12 at 09:09
1

+1 A Factory pattern is the best solution, it provides you with some extra flexibility (for example allowing many-to-one mappings from changers to panels), separates the logic from the actual class and puts the "configuration" all in one place. – Viruzzo Jan 10 '12 at 09:10
This seems like the best idea for now, I think that refactoring to the degree necessary is beyond my expertise level and that using a factory class with a note about the future possibilities will satisfy my current level. – FooBar Jan 10 '12 at 09:16
Instead of factories, I would highly recommend using google guice http://code.google.com/p/google-guice/ – avanderw Jan 10 '12 at 12:57

score 4 · Answer 2 · answered Jan 10 '12 at 08:55

4

I think its good that your first impulse didn't work :) Otherwise you would couple you changer code (which should be something about logic) to UI code (panel) and its wrong.

Now I can offer you the following solution:

create an interface PanelCreator with method Panel createPanel like this:

interface PanelCreator {
   Panel createPanel();
}

Now, provide 2 implementations:

public class OrangePanelCreator implements PanelCreator{
   Panel createPanel() {
        return new OrangePanel();
   }
}

public class ApplePanelCreator implements PanelCreator {

  Panel createPanel() {
        return new ApplePanel();
  }
}

And now come the interesting part:

Create a Map, PanelCreator> this would act like a registry for your panels:

Map<Class<Changer>, PanelCreator> registry = new HashMap<>;
registry.put(OrangeChanger.class, new OrangePanelCreator());
registry.put(AppleChanger.class, new ApplePanelCreator());

And in your code now you can do the following thing:

panel = registry.get(changer.getClass()).createPanel();

I think it will be more elegant since you can easily change implementations of creators given the changer.

Hope this helps

answered Jan 10 '12 at 08:55

Mark Bramnik

39,963
4
57
97

A good quick solution, but it won't work if someone adds a subclass of e.g. OrangeChanger. – artbristol Jan 10 '12 at 08:58
you that in this case the registry should be updates with additional "put" call? – Mark Bramnik Jan 10 '12 at 09:00
You can do that, but you're breaking the Liskov Substitution Principle. There's not really any advantage over an if statement, in terms of lines of code expended to achieve the same result. – artbristol Jan 10 '12 at 09:04
In theory you're right, but the question here is whether its reasonable to create such a subtype. You're right also that there is no advantage in the current implementation, but the main idea was to decouple the logic of conditional creation of Panels and the changer. In real cases the best design won't be a simple map but probably some more advanced configuration (probably xml based with spring or whatever). In this case the advantage is clear, you don't need to recompile your code once the new changes is created. – Mark Bramnik Jan 10 '12 at 09:19
As for the lines of code measurement, sorry, I just don't buy it. If you need less lines of code, you can probably take a look on groovy or scala where the closures can make the same job dynamically, without a need to create a whole hierarchy of classes like I was forced to do in Java... – Mark Bramnik Jan 10 '12 at 09:20
if you do add another implementation, you would have to modify the code/ configuration for this look up to work anyways, irrespective of if-else or registry or the polymorphic way. a subclass of an existing implementation does not hurt either. There is definitely an advantage with the registry approach with the number of lines going down significantly, in addition to a massive 'apparent' reduction in cyclomatic complexity. I say apparent because the n-paths don't actually go down but they are hidden via configuration. anyways, it makes code much simpler to read and relate to, IMHO. – aishwarya Jan 10 '12 at 09:21

score 2 · Answer 3 · answered Jan 10 '12 at 09:12

If there is more than one of this if/else constructs in the code dependending on the instance type of a Changer, you can use the visitor pattern like this:

public interface ChangerVisitor {
  void visit(OrangeChanger changer);
  void visit(AppleChanger changer);
  ...
}

public class ChangerVisitorEnabler<V extends ChangerVisitor> {
  public static <V extends ChangerVisitor> ChangerVisitorEnabler<V> enable(V) {
    return new ChangerVisitorEnabler<V>(visitor);
  }

  private final V visitor;

  private ChangerVisitorEnabler(V visitor) {
    this.visitor = visitor;
  }

  public V visit(Charger changer) {
    if (changer instanceof OrangeChanger) {
      visitor.visit((OrangeChanger)changer);
    } else if (changer instanceof AppleChanger) {
      visitor.visit((AppleChanger)changer);
    } else {
      throw new IllegalArgumentException("Unsupported charger type: " + changer);
    }
    return visitor;
  }
}

Now you have a single type check code block and a type safe interface:

public PanelChooser implements ChangerVisitor {

  public static Panel choosePanel(Changer changer) {
    return ChangerVisitorEnabler.enable(new PanelChooser()).visit(changer).panel;
  }

  private Panel panel;

  private PanelChooser() {
  }

  void visit(OrangeChanger changer) {
    panel = orangePanel();
  }

  void visit(AppleChanger changer) {
    panel = applePanel();
  }

}

The usage is very simple:

panel = PanelChooser.choosePanel(chooser);

score 1 · Answer 4 · answered Jan 10 '12 at 08:46

1

Perhaps you can do:

public Panel getPanel(Changer changer)
{
    String changerClassName = changer.class.getName();
    String panelClassName = changerClassName.replaceFirst("Changer", "Panel");
    Panel panel = (Panel) Class.forName(panelClassName).newInstance();
    return panel;
}

I don't program in Java, but that is what I would try if this were in C#. I also don't know if this would work with your packages.

Good luck!

answered Jan 10 '12 at 08:46

Daniel Lidström

9,930
1
27
35

1

Ok, but this requires naming conventions for your classes; there must be an `xyzPanel` for each `xyzChanger` class. And it isn't really type-safe, because there is a cast to `Panel` there. – Jesper Jan 10 '12 at 08:54
2

IMHO, it's better to avoid such manipulations with class names, when it's not the only option. It's harder to maintain, e.g. class name changes won't cause any compilation error. – Wizart Jan 10 '12 at 08:55
1

Yes, this would be a naming convention. Conventions are supposed to speed up development. However, `getPanel` should be possible to override, in case you'd like to change the convention for some `Changer` type. This would be the abstract factory design pattern, in case you're wondering. – Daniel Lidström Jan 10 '12 at 08:58
This is interesting, I'll search for if I can implement it in Java, thanks. – FooBar Jan 10 '12 at 09:01
That would already be Java, unless I made a mistake somewhere :-) – Daniel Lidström Jan 10 '12 at 09:07

score 0 · Answer 5 · answered Jan 10 '12 at 08:53

0

I don't see enough existing code and design at whole. So probably, first of all I would try to move the code with panel instantiation to the same place where Changer instance is created. Because choosing a Panel is the same decision as choosing a Changer.

If a selected Changer is dynamically selected, you may just create these panels and then show/hide them accordingly.

answered Jan 10 '12 at 08:53

Wizart

940
5
8

The Changer is assigned using a call to the server, the server has no access to panel code so unfortunately that's not an option. – FooBar Jan 10 '12 at 08:58

Mark Rotteveel · Answer 6 · 2012-01-10T09:10:32.087

Your solution will not work, because Java selects the method based on the compiletime type (which here is probably Changer). You could use a Map<Class<? extends Changer>, Panel> (or Map<Class<? extends Changer>, Class<? extens Panel>> if you need to create new instances every time). This solution does require extra work if you need this to work for - yet unknown - subclasses of for example OrangeChanger.

eg for a single instance per Changer subclass

changerToPanel.get(changer.getClass());

or if you need new instances:

changerToPanelClass.get(changer.getClass()).newInstance();

The other option would be to go for your initial hunch, and make Changer know about Panel.

Akos Cz · Answer 7 · 2012-01-10T09:11:34.800

Take a look at the Factory and Abstract Factory Patterns.

The Factory Pattern is a creational pattern as it is used to control class instantiation. The factory pattern is used to replace class constructors, abstracting the process of object generation so that the type of the object instantiated can be determined at run-time.

Abstract Factory Pattern is a creational pattern, as it is used to control class instantiation. The abstract factory pattern is used to provide a client with a set of related or dependent objects. The family of objects created by the factory is determined at run-time according to the selection of concrete factory class.

score 0 · Answer 8 · answered Jan 10 '12 at 09:05

I'd do the following:

Have an interface PanelChooser with a single method returning a Panel for a Changer.

Have an implementation ClassBasedPanelChooser returning a panel when the Change implements a certain class and null otherwise. The class and the panel to be returned get passed in in the constructor.

Have another implementation CascadingPanelChooser which takes a list of PanelChoosers in the constructor arguments and on call of its method asks each PanelChooser to provide a panel until it receives a not null panel, then it returns that panel.

score 0 · Answer 9 · edited May 23 '17 at 11:48

Do not use instanceof.Why polymorphism fails The only place to use instanceof is inside equals method.

To answer your question. Follow this link. Credits to Cowan and jordao .

Using Reflection.

public final class Handler {
  public static void handle(Object o) {
    for (Method handler : Handler.class.getMethods()) {
      if (handler.getName().equals("getPanel") && 
          handler.getParameterTypes()[0] == o.getClass()) {
        try {
          handler.invoke(null, o);
          return;
        } catch (Exception e) {
          throw new RuntimeException(e);
        }
      }
    }
    throw new RuntimeException("Can't handle");
  }

  public static void handle(Apple num) { /* ... */ }
  public static void handle(Orange num) { /* ... */ }

Chain of Responsibility

 public abstract class Changer{
   private Changer next;

   public final boolean handle(Object o) {
      boolean handled = doHandle(o);
      if (handled) { return true; }
      else if (next == null) { return false; }
      else { return next.handle(o); }
   }

   public void setNext(Changer next) { this.next = next; }

   protected abstract boolean doHandle(Object o);
}

public class AppleHandler extends Changer{
   @Override
   protected boolean doHandle(Object o) {
      if (!o instanceof Apple ) {
         return false;
      }
      OrangeHandler.handle((Orange) o);
      return true;
   }
}

Refactoring code in Java, alternatives to large if statement

9 Answers9

Related