Retrieve data from an XML file in Java

Question

few months back I did a code test for a company and I quickly managed to solve it within 5-10 minutes. But, I got rejected because it wasnt "good enough".

What is good enough? they said its mainly how I wrote the code and that I was missing exception handling..

Anyways I was supposed to retrieve data from an XML file and store it into a file. Here is my code:

 public static void main(String[] args) throws ParserConfigurationException, IOException, SAXException { xmlDataToFile("src/resources/xml-File.xml", "41"); } static void xmlDataToFile(String url, String value) throws ParserConfigurationException, IOException, SAXException { DocumentBuilder documentBuilder = DocumentBuilderFactory.newInstance().newDocumentBuilder(); Document document = documentBuilder.parse(url); NodeList nodeList = document.getElementsByTagName("target"); for (int i = 0; i < nodeList.getLength(); i++){ Node node = nodeList.item(i); if (node.getParentNode().getAttributes().item(0).getTextContent().equals(value)){ Files.write(Paths.get("textFile.txt"), Collections.singleton(nodeList.item(i).getTextContent())); } } 

How can I improve this ?

Answer 1

Well, there's a problem here, which is that your question is being read here by people who know the subject far better than your assessors; so you're not actually asking for what the experts consider to be best practice, but for what your assessors, who aren't experts, consider to be best practice.

Many experts will tell you not to use DOM for this. 90% of Java programmers probably would use DOM (as you are doing), but there are far better libraries available (for example JDOM2 or XOM). The only reason anyone uses DOM is because they haven't bothered to look at alternatives. But your assessors are probably expecting a DOM solution.

Now the only thing that strikes me as really bad about your code (and it's hard to judge without seeing the specification of the input XML) is

getParentNode().getAttributes().item(0).getTextContent().equals(value) 

because that's looking at the first attribute of the parent of the current target element, and the order of attributes is unpredictable. Even if you know that the parent element will always have exactly one attribute, it's best not to make that assumption in your code. I would expect to see you accessing a specific attribute of the parent element by name, for example

value.equals(getParentNode().getAttibute("name")) 

which is also written so it doesn't fail if the attribute is absent.

Another thing I'm not keen on in your answer is the repeated opening of the file in

Files.write(Paths.get("textFile.txt") 

That feels thoroughly inefficient to me, though I've no idea if it's really a problem or just my imagination.

Personally, and I have no idea whether your assessors would be satisfied with the answer, I would do this with a one-line XPath 3.1 expression. For example, using the Saxon API because it's the one I know best:

 Processor proc = new Processor(false): XPathCompiler xp = proc.newXPathCompiler(); XdmItem sourceDoc = proc.newDocumentBuilder().build( new StreamSource("src/resources/xml-File.xml")); XdmItem result = xp.evaluate( "string-join(//target[../@name='42'], '\n')", sourceDoc); Files.write(Paths.get("textFile.txt"), result.getStringValue());