Adam Heath wrote:
Hmm. I just switched my checkout to your branch. I'm seeing some
things that need to be reviewed. I'm going to need to make some time
for this.
Initially, ArtifactPath implements Iterator<String>. I would have
done that as Iterable<String>, which would then allow ArtifactPath to
be used in for(type name: var) constructs.
Also, in the same class, getPathAsString has inconsistent used of
'this' when referencing the internal stringBuilder object.
Actually, what is the prefered way of making use of 'this'? Should it
always be used on local method access? local variable access?
Personally, I think it should *not* be used, as the compiler will
figure it out for you, and the computer should be used for what it is
good for, automation.
The static PATH_ROOT variable could get corrupted, if calling code
doesn't properly do a saveState/restoreState pair. This is based on
my earlier emails in this thread.
Aie, this one is bad. getPathAsString uses an instance variable,
stringBuilder, then manipulates it's state. This could break in
multi-threaded situations, esp. when considering that PATH_ROOT is a
shared static.
Based on ContextUtil and ArtifactPath, it looks like
saveState/restoreState are *always* called. This means that
ArtifactPath.stack will *always* be set to a FastList instance. It
would be better to remove the null checks, and make stack a final
instance variable.
The array based ArtifactPath constructor should use ... instead of [],
to allow varargs.
It's generally bad form have an object take a parameter, then not
internalize said parameter. Ie, the array constructor doesn't take
ownership of the pathElementArray, thereby allowing calling code to
manipulate it. If this is by design, it should be listed as such in
the javadocs.
Does ArtifactPath support federation? Ie, why not use jndi for this,
as it has path/name support stuff.
Those are good suggestions - thank you for taking the time to look
things over. Good catch on the static PATH_ROOT - I hadn't thought of that.
I need to put this on the shelf for a while and get back to work on the
trunk. If anyone wants to make changes to the branch, they are welcome
to do so.
-Adrian
