jackrabbit-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jukka Zitting" <jukka.zitt...@gmail.com>
Subject Re: [jira] Commented: (JCR-569) WorkspaceImporter Refactoring
Date Tue, 12 Sep 2006 21:45:38 GMT

On 9/12/06, Nicolas <ntoper@gmail.com> wrote:
> Sure. Here it is:

Cool, thanks.

> Issues I am trying to solve:

I pretty much agree with your analysis of the issues, as I've faced
similar problems working on JCR-325. I labeled the issues for brevity:

   A: Methods are too long
   B: Complexity
   C: Factorizing/Code duplicate
   D: Bugs
   E: Lack of flexibility

> Bugs: jcr:root is not handled correctly and other bugs have been detected.

For reference: the jcr:root issue and the preferred fix is discussed
in JCR-535. Please file and refer to separate bug reports for other
specific issues. It's better to keep the refactoring issue separate
from functional changes.

> Solution
> My proposal is in four parts:

In my opinion it would be best to split the solution to separate
issues to make it easier to review the changes and to understand the
rationale for each change.

> - Add some private methods to abstract some of the complex issue this class
> is managing

Do you mean the isAddable, checkNode, createProperties, createNode,
isSkipped, isCorrect, and postProcess methods? Are they supposed to be
private or protected, i.e. can a subclass use them directly? Are they
final, i.e. can a subclass override them?

I can see how this would help with goals A, B, and C. Could you make a
patch that changes the WorkspaceImporter class accordingly so we can
see these methods in effect. The patch should improve A, B, and C and
still pass all unit tests for me to apply it.

> - Add more control in the constructor (for instance for our backup tool):
> add a constructor with the BatchedItemOperation so we can choose if needed
> the needed ItemStateManager. (For instance to restore the version history).
> - Add a raw flag in the constructor to import the content as it is without
> initializing versioning and escaping some checks. This is a use case already
> encountered

Both of these parts are related to goal E. They are extra features and
pretty much independent of structural refactoring. I'd propose these
changes as a a separate issue and a separate patch.

> - With this new design, solve remaining bugs and handle the jcr:root issue.

This would be goal D. Again, I would propose a fix for the jcr:root
issue in JCR-535 and in separate reports for any other issues.

> Please have a look as the patch proposed earlier to have a look at the
> design and the new code (it is far from over and a lot of methods are
> incomplete).

I like your approach, but as you mention it still needs some work. I'd
be happy to review an updated version.

I'm a bit concerned that the flow in the proposed class, especially in
the startNode() method isn't directly based on the flow within the
existing WorkspaceImporter. Are you certain that all special cases are
handled as before? Could we add some unit tests to verify that all
those special cases remain operational? I don't think the current
WorkspaceImporter class has full test coverage.


Jukka Zitting

Yukatan - http://yukatan.fi/ - info@yukatan.fi
Software craftsmanship, JCR consulting, and Java development

View raw message