[
https://issues.apache.org/jira/browse/JCR-556?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12474961
]
Jukka Zitting commented on JCR-556:
-----------------------------------
Re: VisitorPattern200207.patch; Looks good, details below:
The main issue is about visiting NodeReferences, basically all the information
required to recreate the NodeReferences is already included in the Node- and
PropertyStates, as the NodeReferences objects simply list the PropertyIds of
all PropertyStates that have reference values pointing to given Nodes. If a
visitor needs that reference information, it can recreate the NodeReferences
objects on the fly based on the visited PropertyState instances. Thus I'd drop
the visit(NodeReferences) method from the visitor interface.
The code flow in accept(String, ItemStateVisitor) is a bit cumbersome, though
it's perhaps just me and my taste. The "if (sql.equals(...))" statements seem a
bit unnecessary since the state information already exists higher up the call
chain at accept(ItemStateVisitor). I'd rather duplicate the JDBC statement
handling code than use such a switch structure.
There is a somewhat essential issue regarding the extra whitespace being added
to SimpleDbPersistenceManager. Since you make no other changes to that class,
it would be better if the patch didn't touch the file at all.
Alltogether it's good work, feel free to continue based on that. I'll however
not apply the changes to the Jackrabbit trunk until there's also some client
code that uses the new interfaces.
> Refactoring of the BackupTool
> -----------------------------
>
> Key: JCR-556
> URL: https://issues.apache.org/jira/browse/JCR-556
> Project: Jackrabbit
> Issue Type: Improvement
> Reporter: Nicolas Toper
> Priority: Minor
> Attachments: BIO.patch, patch-backup-040906.txt,
> patch-jr-010906-NodeVersionHistoriesUpdatableStateManager.txt,
> patch-jr-010906-PropInfo.txt, patch-jr-010906-RepositoryImpl.txt,
> patch-jr-010906-SysViewImporter.txt, patch-jr-010906-VersionManagerImpl.txt,
> PropInfo.patch, VisitorPattern200207.patch
>
>
> The BackupTool has still some refactoring to perform. I will propose the
> patch on this new issue since we cannot change the JCR-442 (Google wants to
> see a place where all the Google SoC work is).
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
