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()); } }

What are you hoping to get, performance or usability? It's already pretty concise, and I don't think you can squeeze performance out of it as asked here. You could import org.w3c.dom.Node and remove it from the code, but that's not really necessary (IMO). Is there a reason you want to reduce it? — dann.dev, CommentedMar 22, 2012 at 23:44
academic purpose... trying to practice efficient ways to write a code ... i don't believe that in java there is good justification to write such a long method ... looking for a way to minimize the code — user534976, CommentedMar 22, 2012 at 23:52
how to minimize the provided code using java's features ... looking for some workaround with the switch-case statement — user534976, CommentedMar 22, 2012 at 23:55
Haha this is hardly a long method... I have seen some true horrors before, not the point though. Look at the purpose of the method, does it fufill it. In this case it takes in the Node, and performs an action on it depending on what kind it is. There's not really any reason to shorten it. You could move what happens in each case to a separate method, but then you are just doing it for the hell of it, which you should always avoid — dann.dev, CommentedMar 23, 2012 at 0:00

Michael K · Accepted Answer · 2012-03-23 13:13:39Z

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.

point taken , thanks. By the way, in java if you have one statement after if and after else then it is ok to write the if-else without brackets — user534976, CommentedMar 24, 2012 at 17:58
I know - that's a personal preference. This answer sums up why if you're interested. — Michael K, CommentedMar 24, 2012 at 18:42

RMorrisey · Accepted Answer · 2012-03-23 00:05:07Z

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.

The get method will throw a NullPointerException if it encounters aj unknown node type. Generally, I think this is overengineered. It will maybe make the code more maintainable, but also less readable by introducing new classes and an interface. — Jakub Zaverka, CommentedMar 23, 2012 at 0:21
@Jakub Zaverka: A little extra code can easily be added to handle the "default" case. I chose to try to make the sample code above as simple as possible, just to illustrate the concept. I don't feel that the code is less readable; each new class has a single responsibility and a clear purpose. "More classes" doesn't automatically equate to "less readable". Since the number of cases is small, and each one is short, the OP's code is fairly readable, but he was looking for design improvements. I feel like this is a good potential improvement for a long switch-statement pattern. — RMorrisey, CommentedMar 23, 2012 at 0:30
Thanks, vary useful pattern indeed, red much more about it as a result of your suggestion :) but in my case - when i have limited and deterministic number of cases which won't change, this approach is an overkill .... — user534976, CommentedMar 24, 2012 at 18:01

JoeJoe · Accepted Answer · 2012-03-23 00:45:12Z

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

}

The formatting got a little funky. There should be more line breaks, but you catch my drift. — Joe, CommentedMar 23, 2012 at 0:46

Stack Exchange Network

Handling various types of nodes when traversing a DOM tree

3 Answers 3

Hot Network Questions

Handling various types of nodes when traversing a DOM tree

3 Answers 3

Related

Hot Network Questions