-2

I have code in the following form:

public void drawObject(MyObject myObject) {
  RootElement root = new RootElement();

  if (myObject.hasA()) {
     root.addElement(new XElement());
  } else {
     root.addElement(new YElement());
  }
  if (myObject.hasB()) {
     root.addElement(new ZElement("A"));
  }
  if (myObject.hasC()) {
     root.addElement(new ZElement("B));
  }

  //and so on for around 20 more conditions

  root.draw();
}

I think that's not optimal. However, I can't think of any better solution to this. Is there any design pattern that I could use?

SCM
  • 31

2 Answers2

2

There isn't always a design pattern that will elegantly solve complexity. Some complexity is inherent in the problem you're solving. At most, you can shuffle the complexity around.

So quite possibly, the If Statement Pattern is the best you can do here.

However, if your MyObject type is some interface and different concrete types have different but simpler logic for drawing themselves, you could replace conditionals with polymorphism. This splits up the complexity and moves it into individual classes. For example:

public void drawObject(MyObject myObject) {
  myObject.toElement().draw()
}

interface MyObject {
  ...
  public RootElement toElement();
}

class MyObjectA implements MyObject {
  ...
  @Override
  public RootElement toElement() {
    RootElement root = new RootElement();
    root.addElement(new XElement());
    return root;
  }
}

class MyObjectB implements MyObject { ... }
class MyObjectC implements MyObject { ... }

Sometimes, instead of testing whether some element should be added, it could make sense to use the Null Object Pattern. You would then always add an element, but the element may not draw anything. Whether that would simplify things depends on your specific code, and your snippet is to abstract to make a statement on this.

But again: maybe this complexity cannot be avoided. As a last resort, you can extract individual conditions into separate helper functions and give them a useful name. This can make a large complex method easier to read, but makes the overall code more complex. Something like this:

public void drawObject(MyObject myObject) {
  RootElement root = new RootElement();

  addElementForA(root, myObject);
  addElementForB(root, myObject);
  addElementForC(root, myObject);
  ...

  root.draw();
}

private static void addElementForA(RootElement root, MyObject myObject) {
  if (myObject.hasA()) {
    root.addElement(new XElement());
  } else {
    root.addElement(new YElement());
  }
}

private static void addElementForB(RootElement root, MyObject myObject) {
  if (myObject.hasB()) {
    root.addElement(new ZElement("A"));
  }
}

private static void addElementForC(RootElement root, MyObject myObject) {
  if (myObject.hasC()) {
    root.addElement(new ZElement("B"));
  }
}

Don't use placeholder names like this in the real code, but think about what the condition represents in your problem domain. E.g. if this would assemble a web page with an optional footer, you might call one of these helpers maybeAddFooter().

amon
  • 135,795
2

This code features many things I'd improve but here's the first refactoring:

public void drawObject(MyObject myObject) {
  RootElement root = new RootElement();
  Element element = myObject.elementFactory();
  root.addElement(element);

  root.draw();
}

Why? Feature Envy, Tell. Don't ask., and Functions Should be Small. As small your naming talent permits.

MyObject knows everything you're asking about to make the element. Why not just tell MyObject to make the element? Now you don't have to know so much about the insides of MyObject. One way to do that is an Abstract Factory.

candied_orange
  • 119,268