![]() |
|
|
|
|
Change By:
|
Philip Mundt
(28/Jun/13 11:03 AM)
|
|
Description:
|
It's an abstract mess in general grown over time. This could be simplified a lot by simple refactorings, documentation and probably get rid of some classes.
all those methods in there take a node or an Id to perform actions, but the the classes are wrapping/adapting a single property or node, so you could theoretically create an adapter and pass a not relating node into it. Why pass those references around?
---
info.magnolia.ui.vaadin.integration.jcr.JcrPropertyAdapter
is using the same lists as the node adapters to keep track of changed properties. The properties beeing used inside a property are:
- ModelConstants.JCR_NAME
- JcrPropertyAdapter#VALUE_PROPERTY
- JcrPropertyAdapter#TYPE_PROPERTY
It's very confusing. There are properties inside properties. They override the same abstract functions, but do different things.
The constants are locally stored, but also using ModelConstants.
info.magnolia.ui.vaadin.integration.jcr.AbstractJcrAdapter#changedProperties
info.magnolia.ui.vaadin.integration.jcr.AbstractJcrAdapter#removedProperties
---
methods doing almost the same but not really, confusing:
info.magnolia.ui.vaadin.integration.jcr.AbstractJcrNodeAdapter#updateProperty
info.magnolia.ui.vaadin.integration.jcr.JcrPropertyAdapter#updateProperty
info.magnolia.ui.vaadin.integration.jcr.AbstractJcrNodeAdapter#getItemProperty
info.magnolia.ui.vaadin.integration.jcr.JcrPropertyAdapter#updateProperty
---
property and nodes using getJcrItem, which is not a good idea from a performance perpective. And we do know on which type of item we are working on, so change it to getProperty and getNode. We end up casting every time we get a property or node from the repo.
info.magnolia.ui.vaadin.integration.jcr.AbstractJcrAdapter#getJcrItem
---
info.magnolia.ui.vaadin.integration.jcr.AbstractJcrNodeAdapter#getNode
contains these lines of code:
// get Node from repository
node = (Node) getJcrItem();
50 lines earlier, we have a method:
info.magnolia.ui.vaadin.integration.jcr.AbstractJcrNodeAdapter#getNodeFromRepository() {
return (Node) getJcrItem();
---
DefaultProperty is used for creating an actual default property based on some default value but mostly it's used to create properties with non-default values. The term default may be misleading. same goes for DefaultPropertyUtil.
As we're using generics in DefaultProperty now, maybe introducing generics in Adapters would make sense?
---
DefaultPropertyUtil vs info.magnolia.jcr.util.PropertyUtil
---
From code MGNLUI-568 review:
The putting/removal of a property from the changedProperties and removedProperties HashMap in {{info.magnolia.ui.vaadin.integration.jcr.AbstractJcrAdapter}} should be synchronized or a synchronized map could be used instead.
A marginal remark
the while loop at {{info.magnolia.ui.vaadin.integration.jcr.AbstractJcrAdapter.updateProperties(Item)}} could be rewritten as a for loop
{code}
for (Entry<String, Property> entry : changedProperties.entrySet()) {
...
}
{code}
ATM, removing properties will clear removedProperties HashMap, however, applying changes to (saving) properties doesn't remove properties from the changedProperties HashMap.
|
|
|
|
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
|
----------------------------------------------------------------
For list details, see: http://www.magnolia-cms.com/community/mailing-lists.html
Alternatively, use our forums: http://forum.magnolia-cms.com/
To unsubscribe, E-mail to: <
[email protected]>
----------------------------------------------------------------
