Details

    • Type: Task Task
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.4
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Similar issues:
      KULRICE-9940Cleanup from new copy methods
      KULRICE-10052Cleanup parseExpression method
      KULRICE-12958xapool cleanup
      KULRICE-7783Finish cleanup of Ajax code
      KULRICE-6864Cleanup of UIF footer beans
      KULRICE-10773Add permission check the Transactional Document copy method
      KULRICE-13138Apply the new deep copy method to Rice objects
      KULRICE-1869Create BO to DTO convenience methods
      KULRICE-1811Code cleanup
      KULRICE-6608Misc. KRMS Code cleanup
    • Epic Link:
    • 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

        Activity

        No work has yet been logged on this issue.

          People

          • Assignee:
            Matthew Wuertz (Inactive)
            Reporter:
            Jerry Neal (Inactive)
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 2 days, 4 hours
              2d 4h
              Remaining:
              Remaining Estimate - 2 days, 4 hours
              2d 4h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Agile

                  Structure Helper Panel