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

            //add it to list
            Spell.addSpell(s);
        }
    }
}

-
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

-
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.

StackExchange Code Review Q#49028, answer score: 5