Adrian Crum wrote: > Adam Heath wrote: >> 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. > > That was by design. I didn't put a great deal of thought into that class > initially - it started out as a convenience. As development progressed, > it evolved. Writing JavaDocs is a LONG way off. > > A lot of the improvements you suggested (and are committing) I would > have gotten around to eventually. My focus was to get it operational so > it can be evaluated. > > By evaluated I mean on a higher level than what you're doing here. I > need the *concept* evaluated and commented on. Will this approach to > security work? Can we agree on its advantages? Try converting a > component over to the new security - was it easier or harder than > expected? How will this fit in with your projects? How can it be > expanded/improved/made easier?
To do that kind of high-level evaluation, I need quick scans, and let my gut respond. It has a hard time doing that when there are littly nit-picky style issues. Plus, going thru all files like I have done allows me to see all the files quickly, and try to get a feel for them. I haven't read the design docs, haven't read the readme file, just trying to read the code to see if it makes sense. Initially, I like the idea of what is occuring here. There is more than I thought, based on just reading the list. Some of the things you are doing we've wanted to do with webslinger, so it's good that there is an implementation in ofbiz now. I'll continue to review, but it'll be a bit before I can actually sit down for an extended period.
