Handling various types of nodes when traversing a DOM tree

Question

How to minimize the following code using java's features ... looking for some workaround with the switch-case statement

I've seen several question regarding switch-case design pattern / best practices but because I am new to java, I am having difficulties in implementing them.

Here is the code:

protected void readTree(Node node, Branch current)
{
  Element element = null;
  Document document = null;

  if(current instanceof Element)
  {
     element = (Element) current;
  }
  else
  {
     document = (Document) current;
  }



  String nodeVal = node.getNodeValue();
  String nodeName = node.getNodeName();

  switch(node.getNodeType())
  {
  case ELEMENT_NODE:
     readElement(node, current);

     break;

  case PROCESSING_INSTRUCTION_NODE:

     if(current instanceof Element)
     {
        element.addProcessingInstruction(nodeName, nodeVal);
        break;
     }
     document.addProcessingInstruction(nodeName, nodeVal);

     break;

  case COMMENT_NODE:

     if(current instanceof Element)
     {
        element.addComment(nodeVal);
        break;
     }

     document.addComment(nodeVal);

     break;

  case DOCUMENT_TYPE_NODE:

     DocumentType domDocType = (DocumentType) node;

     document.addDocType(domDocType.getName(), domDocType.getPublicId(), domDocType.getSystemId());

     break;

  case TEXT_NODE:
     element.addText(nodeVal);

     break;

  case CDATA_SECTION_NODE:
     element.addCDATA(nodeVal);

     break;

  case ENTITY_REFERENCE_NODE:

     if(node.getFirstChild() != null)
     {
        element.addEntity(nodeName, node.getFirstChild().getNodeValue());
        break;
     }
     element.addEntity(nodeName, "");

     break;

  case ENTITY_NODE:
     element.addEntity(nodeName, nodeVal);

     break;

  default:
     System.out.println("WARNING: Unknown node type: " + node.getNodeType());
  }
}

Answer 1

I don't really like this:

case ENTITY_REFERENCE_NODE:
     if(node.getFirstChild() != null)
     {
        element.addEntity(nodeName, node.getFirstChild().getNodeValue());
        break;
     }
     element.addEntity(nodeName, "");

     break;

Idiomatically, more than one break in a case is confusing. You should either refactor to an if...else:

if(node.getFirstChild() != null) {
   element.addEntity(nodeName, node.getFirstChild().getNodeValue());
}
else {
   element.addEntity(nodeName, "");
}
break;

or (I prefer this, by the way) create a method that does this task for you:

case ENTITY_REFERENCE_NODE:
    addNodeChildValue(node);
    break;

In my experience, switch statements are easiest to read when you minimize the number of tasks done in the cases. That's not to say you should only do one, at most two tasks (or some other arbitrary number). You should look for tasks to extract to methods or code that is repeated in several cases that should not be part of the switch.

Answer 2

Consider creating an object which can handle each case:

interface NodeHandler {
    void handle(Node node, Element current, ...);
}

class ElementHandler implements NodeHandler {
    public void handle(Node node, Element current, ...) {
        readElement(node, current);
    }
}

You can then populate a map, like this, to look up the appropriate handler for a node:

private static Map<Integer, NodeHandler> handlerMap;
static {
    handlerMap = new HashMap<Integer, NodeHandler>();
    handlerMap.put(Node.ELEMENT_NODE, new ElementHandler());
    //...
}

and then...

handlerMap.get(node.getNodeType()).handle(...);

I don't think this makes the code shorter, but it does make the code more maintainable. Each object has a single, clear responsibility, and you don't have to deal with issues like fall-through cases, or state getting mingled between different handlers. You could conceivably invent new handlers without actually changing the implementation of the readTree() method, or impacting the behavior of existing handlers.

Answer 3

I think you should eliminate the element and document variables. (Principles behind that: It's better to minimize variable scope, to not make null variables, don't make variables just to cast things to their subtypes.) And when the method you call is a method on Node, you certainly don't need to cast it.

I would do:

try{ .. case Node.PROCESSING_INSTRUCTION_NODE:

//Why cast it in this case?

 node.addProcessingInstruction(nodeName, nodeVal);

 break;

... Node.CDATA_SECTION_NODE: //Do the casting tight like this. Don't make extra variables. //Don't write extra lines of code that don't make the code more clear.

 ((Element)node).addCDATA(nodeVal);

 break;

//Explicitly throw and handle an exception rather than a //silent fallthrough. (This better if there is a way to the error.) //Explain why this can happen in a comment

}Catch (ClassCastException ce){ //Some error handling

}