[KULRICE-10110] Cleanup on copy methods Created: 08/Aug/13  Updated: 21/Apr/14  Resolved: 04/Sep/13

Status: Closed
Project: Kuali Rice Development
Component/s: Development, User Experience (UX)
Affects Version/s: None
Fix Version/s: 2.4
Security Level: Public (Public: Anyone can view)

Type: Task Priority: Major
Reporter: Jerry Neal (Inactive) Assignee: Matthew Wuertz (Inactive)
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: 2 days, 4 hours
Time Spent: Not Specified
Original Estimate: 2 days, 4 hours

Epic Link: Performance
Rice Module:
KRAD
Sprint: 2.4.0-m2 Sprint 2
KAI Review Status: Not Required
KTI Review Status: Not Required
Code Review Status: Not Required
Include in Release Notes?:
Yes

 Description   

• Properties missing from the copy method, all properties should be copied (there is one exception I have found, viewIndex on View should not be copied)
• Copy properties method does not call super
• Nested components are copied without calling .copy
• Some classes missing copy properties method (ex ‘GroupValidationMessages’). Verify all classes that implement Component and LayoutManager have this method implemented
• Change added setters to protected, move to correct location, and add javadoc (see message I sent earlier)
• Scope issues, for example the following code:

if(addLineActions != null) {
List<Action> addLineActions = Lists.newArrayListWithExpectedSize(getAddLineActions().size());
for (Action addLineAction : this.addLineActions)

{ addLineActions.add((Action)addLineAction.copy()); }

}

collectionGroupCopy.setAddLineActions(addLineActions);

Notice the assignment is outside the declaration block of the local addLineActions, so it picks up the global addLineActions for the setter which causes no change. I think it is best to use a different variable name for the local variable so there is no confusion, like this:

if(addLineActions != null) {
List<Action> addLineActionsCopy = ists.newArrayListWithExpectedSize(getAddLineActions().size());
for (Action addLineAction : this.addLineActions)

{ addLineActionsCopy.add((Action)addLineAction.copy()); }

collectionGroupCopy.setAddLineActions(addLineActionsCopy);
}

One way I would recommend to review the methods, is right click on the tab for class and select ‘Split Vertically’. The will give you another editor tab for the same class to the right, you can then scroll up to see the fields listed in the class and compare to the statements in copyProperties. A good practice would be to make the setter calls in the same order as the fields are declared, which would make it easier to determine if something is missing.

That is all we need for 2.3, and I will fill more comfortable with it. Please update first because I have already fixed several issues.

After 2.3, I would like to do some further cleanup. One issue is our copy method with the generics is not working as planned. One of the goals of using the generics is so you don’t have to cast the type for each setter. For example instead of having to do this:

groupCopy.setDisclosure((Disclosure) this.disclosure.copy());

We want to be able to just do this:

groupCopy.setDisclosure(this.disclosure.copy());

Other cleanup (post 2.3):

• Only need one public <T> T copy() { per hierarchy. Since this is in DictionaryBeanBase, not needed in any subclass (Component, LayoutManager, etc).

• Create Copyable interface in org.kuali.rice.krad.datadictionary. Make DictionaryBeanBase an other objects with the copy method (not subclasses) implement

• ‘replacement’ property on PropertyReplacer needs to be coped. Could be a component, List of components, Map of components, or Layout manager. Need to check for interface

• Create helper methods in ComponentUtils for copying a List of Copyables and Map of Copyables so logic does not need to be duplicated everywhere, then change statements in copy properties methods to use the new helper methods

• For nested objects that we are setting directly (not calling copy), you don’t need the null check before (just can set the value as null). This will shorten our copy methods a bit

• Copy properties method does not use @see comment consistently, and sometimes method is after validate method

• Formatting wise I would rather see these lines broken up:

super.copyProperties(component);
ContainerBase containerBaseCopy = (ContainerBase) component;
containerBaseCopy.setDefaultItemPosition(this.defaultItemPosition);

to

super.copyProperties(component);
// blank line
ContainerBase containerBaseCopy = (ContainerBase) component;
// blank line
containerBaseCopy.setDefaultItemPosition(this.defaultItemPosition);

  • Remove use of Google Util lists
  • Changed addded setter methods to use protected method

Generated at Sun Sep 27 19:27:33 CDT 2020 using JIRA 7.0.11#70121-sha1:19d24976997c1d95f06f3e327e087be0b71f28d4.