SpellParser for a text-based RPG

Fellow students are supposed to review and tell what my code is doing for a grade, this is why I'm asking on here first. I want to see if its clear enough.

public class SpellParser {

private static final String SPELL_PACKAGE_LOCATION = "edu.swosu.wordcraft.spells.";

private Document dom;

public SpellParser() {
setUpDom();
}

public void setUpDom(){
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
try {
DocumentBuilder docBuilder = dbf.newDocumentBuilder();
dom = docBuilder.parse("resources/Spells.xml");
} catch(Exception e) {
e.printStackTrace();
}
}

public void parse(){
//get the root element
Element docEle = dom.getDocumentElement();

//get a nodelist of elements
NodeList nl = docEle.getElementsByTagName("spell");
if(nl != null && nl.getLength() > 0) {
for(int i = 0 ; i < nl.getLength();i++) {

//get the spell element
Element el = (Element)nl.item(i);

//get the Employee object
Spell s = getSpell(el);

}
}
}

• Welcome to CR! I have edited the code block so as to include the fist few lines, feel free to edit the indentation to reflect what's in your IDE (might have to do with the fact that our "ask-a-question" boxes don't like tabs). May 6 '14 at 2:06

• Your comments don't help much. They should explain why you're doing what you're doing, not just paraphrase the code. If you can't tell the reader anything more than the code already does, then a comment is pretty much just noise.

• setUpDom() should probably be private. Or possibly, done away with altogether, and its contents moved into one of the other methods. It's doing stuff that looks like it should only be done once per XML document, and thus either once over the whole lifetime of the object or once each time you parse. But it shouldn't be done by anything but your code.

• If you can't handle an exception, you shouldn't be catching it. And "printing a stack trace" is not handling. :P

Once setUpDom() fails, the object is broken, so it doesn't make much sense to continue. Let the exceptions propagate (declaring them in a throws clause instead), or throw your own exception type that describes what went wrong, including e as the inner exception.

• You don't need to check that nl is an object, or worry about the length. The Document class should always hand you back a NodeList, and your for loop will already loop 0 times if the list is empty, as the condition i < nl.getLength() will be false before the very first iteration.

• The getSpell function and Spell class don't seem to be here. So there's not a whole lot more to say, other than "fix your indents". (I half-expect that that is a copy/paste issue, but it should still be mentioned.)

It does appear that Spell has some static variables, though, which is usually not a great solution. And you're hard-coding where the data comes from and where it goes.

I'd personally suggest a SpellBook class or the like, that holds spells. You could pass an instance of such a type (and the name of the XML file) either to your constructor or to parse. Among other things, that leaves open the possibility for multiple sets of spells.

If that's one of your classmates' part, though, and you can't change that...eh. Carry on.

• IMHO I disagree that the "comments don't help much" - given today's syntax highlighting, basic comments like this allow the reader to skim through a lot of code much more quickly than otherwise. If I'm taking on a project, new to a team, this type of commented summary helps a lot getting a feel for the project. Bigger comments are appropriate for more complex tasks, but I would never advocate that comments are noise. May 6 '14 at 6:34
• @RichardLeMesurier: A prime example of why these comments are noise: // get the Employee object Spell s = getSpell(el); Wait, what? Employee? This code has changed since that comment was introduced. But since no one updated the comment as well, it is now lying to us. At some point in the code's life, this almost always happens. And you wouldn't know it if you weren't already reading the code too. That makes the comment useless.
– cHao
May 6 '14 at 13:18

Is SPELL_PACKAGE_LOCATION used? This also seems like the case for a public static parse() method, which would call the private static setupDom() function. This would eliminated the need to make a new Object every time you want to read the spells. As cHao mentioned, you should probably pass some kind of a list of spells as a parameter to the parse() function and read into that list.