o.j.dom.DocumentNavigator methods do not return null where expected

We should evaluate the whole idea of returning null when an unexpected node type is received. In general, I think throwing an exception would be a much better option than returning null.

I guess its worth pointing out that these methods are never called incorrectly in jaxen: there's always a call to 'isElement()' prior to 'getElementStringValue' for example. (the calls should probably be to getNodeType() in most cases, but thats another issue)

I wouldn't 'fix' these methods as it would involve duplicating the check that needs to be done outside the call anyway. How about changing the documentation so that 'calling getElementStringValue on a non-Element node has an undefined result'?

On your point about nulls: totally agree. There is no null in the xpath datamodel. Null nodesets should be empty Lists, null Strings should be "", everything else follows from the type conversion rules.

These are public methods so we do need to document what happens when unexpected objects are passed to them. Even if Jaxen never passes the wrong type in, we can't assume all other code will be equally correct.

I see it differently: the point of the Navigator interface is to specify what jaxen needs from a third party object model. Hence jaxen defines how Navigators should be called. The interface is only public so that it can be implemented by third parties, not called by them. Hence, defining behaviour and adding checks for a calling pattern other than jaxen's would only end up slowing jaxen down.

That being said, adding assertions would be the right way to go, if jaxen required 1.4+ to build.

(just my opinion, I'm a committer not the project lead. Its worth discussing on the list)

I've updated the code so that none of the get*Iterator methods return null. This makes the code dealing with them simpler (and faster) as they no longer have to check the return value. There's still nearly 100 unnecessary checks for nulls dealing with these stringvalue methods, I'd like to whittle these down next.

Performance wise, and code-wise, I think what should happen is

1. We just make the cast without any check; that is, assume we're being passed the right type.
2. If the type is incorrect, allow the ClasscastException to be thrown.

We could maintain existing behavior if necessary, by catching the ClassCastException and only then returning null.