Project

General

Profile

task #8456

FieldUnitDTO: remove newInstanceMethod

Added by Andreas Kohlbecker 3 months ago. Updated 2 months ago.

Status:
Closed
Priority:
New
Assignee:
Category:
cdmlib
Target version:
Start date:
08/12/2019
Due date:
% Done:

100%

Severity:
normal

Description

copied from #8414

FieldUnitDTO providing a newInstance method. This has to be removed:

    /**
     * @param fieldUnit
     */
    public FieldUnitDTO(FieldUnit fieldUnit) {
        super(fieldUnit);
    }


    public static FieldUnitDTO newInstance(FieldUnit fieldUnit){
        FieldUnitDTO fieldUnitDto = new FieldUnitDTO(fieldUnit);
        if (fieldUnit.getGatheringEvent() != null){
            fieldUnitDto.gatheringEvent = GatheringEventDTO.newInstance(fieldUnit.getGatheringEvent());
        }
        fieldUnitDto.setRecordBase(fieldUnit.getRecordBasis().getMessage());
        fieldUnitDto.setListLabel(fieldUnit.getTitleCache());

        return fieldUnitDto;

    }

is this by purpose? To me this looks like an error....

maybe this can be included into the constructor.

OccurrenceServiceImpl.assembleFieldUnitDTO(..) is using the constructor and adds the gathering event to the dto which is done in the newInstance method. --> TODO: needs review and harmonization (I think the newInstance Method should be removed completely as it causes confusion)


Related issues

Related to Edit - task #8425: FieldUnitDTO: remove taxonRelatedDerivedUnits Closed 07/25/2019
Copied from Edit - task #8414: occurrence controller code cleaning and harmonization Closed 07/25/2019

Associated revisions

Revision ed34f59f (diff)
Added by Katja Luther 2 months ago

fix #8456: remove newInstance methof in FieldUnitDTO

Revision a853ae40 (diff)
Added by Katja Luther 2 months ago

fix NPE in FieldUnitDto

History

#1 Updated by Andreas Kohlbecker 3 months ago

  • Related to task #8457: FieldUnitDTO: remove taxonRelatedDerivedUnits added

#2 Updated by Andreas Kohlbecker 3 months ago

  • Copied from task #8414: occurrence controller code cleaning and harmonization added

#3 Updated by Andreas Kohlbecker 3 months ago

  • Related to deleted (task #8457: FieldUnitDTO: remove taxonRelatedDerivedUnits)

#4 Updated by Andreas Kohlbecker 3 months ago

  • Related to task #8425: FieldUnitDTO: remove taxonRelatedDerivedUnits added

#5 Updated by Andreas Kohlbecker 3 months ago

  • Tracker changed from bug to task

#6 Updated by Andreas Müller 3 months ago

  • Assignee changed from Andreas Müller to Katja Luther
  • Target version changed from Unassigned CDM tickets to Release 5.10

Passing this ticket to Katja or Patrick as I never worked on/with this class.

Note: syntax of the method name is also not according to our standards. If it stays we should call it "NewInstance"

#7 Updated by Katja Luther 2 months ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 50

#8 Updated by Katja Luther 2 months ago

  • Assignee changed from Katja Luther to Patrick Plitzner
  • % Done changed from 50 to 0

please review

#9 Updated by Patrick Plitzner 2 months ago

  • Status changed from Resolved to Feedback
  • Assignee changed from Patrick Plitzner to Katja Luther
  • % Done changed from 0 to 90

looks fine. Only on question: Why did you remove the null check when creating the GatheringEvent -> https://dev.e-taxonomy.eu/redmine/projects/edit/repository/revisions/ed34f59fbc07b3029f7c6f939d2108fc324a7d08/diff

before:

        if (fieldUnit.getGatheringEvent() != null){
            fieldUnitDto.gatheringEvent = GatheringEventDTO.newInstance(fieldUnit.getGatheringEvent());
        }
        fieldUnitDto.setRecordBase(fieldUnit.getRecordBasis().getMessage());
        fieldUnitDto.setListLabel(fieldUnit.getTitleCache());

after:

    public FieldUnitDTO(FieldUnit fieldUnit) {
        super(fieldUnit);
        gatheringEvent = GatheringEventDTO.newInstance(fieldUnit.getGatheringEvent());
        setRecordBase(fieldUnit.getRecordBasis().getMessage());
        setListLabel(fieldUnit.getTitleCache());
    }

#10 Updated by Katja Luther 2 months ago

Patrick Plitzner wrote:

looks fine. Only on question: Why did you remove the null check when creating the GatheringEvent -> https://dev.e-taxonomy.eu/redmine/projects/edit/repository/revisions/ed34f59fbc07b3029f7c6f939d2108fc324a7d08/diff

before:

      if (fieldUnit.getGatheringEvent() != null){
          fieldUnitDto.gatheringEvent = GatheringEventDTO.newInstance(fieldUnit.getGatheringEvent());
      }
      fieldUnitDto.setRecordBase(fieldUnit.getRecordBasis().getMessage());
      fieldUnitDto.setListLabel(fieldUnit.getTitleCache());

after:

    public FieldUnitDTO(FieldUnit fieldUnit) {
        super(fieldUnit);
        gatheringEvent = GatheringEventDTO.newInstance(fieldUnit.getGatheringEvent());
        setRecordBase(fieldUnit.getRecordBasis().getMessage());
        setListLabel(fieldUnit.getTitleCache());
    }

the null check is now in the gatheringEventDto

#11 Updated by Patrick Plitzner 2 months ago

Katja Luther wrote:

the null check is now in the gatheringEventDto

The parameter gathering is not checked in the newInstance() method.

  public static GatheringEventDTO newInstance(GatheringEvent gathering){
        GatheringEventDTO dto = new GatheringEventDTO();
        gathering = HibernateProxyHelper.deproxy(gathering);
        if (gathering.getLocality() != null){

That code woul fail if gathering was null.

#12 Updated by Katja Luther 2 months ago

Katja Luther wrote:

Patrick Plitzner wrote:

looks fine. Only on question: Why did you remove the null check when creating the GatheringEvent -> https://dev.e-taxonomy.eu/redmine/projects/edit/repository/revisions/ed34f59fbc07b3029f7c6f939d2108fc324a7d08/diff

before:

        if (fieldUnit.getGatheringEvent() != null){
            fieldUnitDto.gatheringEvent = GatheringEventDTO.newInstance(fieldUnit.getGatheringEvent());
        }
        fieldUnitDto.setRecordBase(fieldUnit.getRecordBasis().getMessage());
        fieldUnitDto.setListLabel(fieldUnit.getTitleCache());

after:

    public FieldUnitDTO(FieldUnit fieldUnit) {
        super(fieldUnit);
        gatheringEvent = GatheringEventDTO.newInstance(fieldUnit.getGatheringEvent());
        setRecordBase(fieldUnit.getRecordBasis().getMessage());
        setListLabel(fieldUnit.getTitleCache());
    }

the null check is now in the gatheringEventDto

do you have the actual code??

in my repository it looks like this:

public FieldUnitDTO(FieldUnit fieldUnit) {
        super(fieldUnit);
        if (fieldUnit.getGatheringEvent() != null){
            gatheringEvent = GatheringEventDTO.newInstance(fieldUnit.getGatheringEvent());
        }
        setRecordBase(fieldUnit.getRecordBasis().getMessage());
        setListLabel(fieldUnit.getTitleCache());
    }

#13 Updated by Katja Luther 2 months ago

sorry I missed to add the last commit to this ticket.

#14 Updated by Katja Luther 2 months ago

  • Status changed from Feedback to Resolved
  • Assignee changed from Katja Luther to Patrick Plitzner

#15 Updated by Patrick Plitzner 2 months ago

  • Status changed from Resolved to Closed
  • Assignee changed from Patrick Plitzner to Katja Luther
  • % Done changed from 90 to 100

Katja Luther wrote:

sorry I missed to add the last commit to this ticket.

Yes, I just realized that, too.

cdmlib|a853ae404e985fb1a72c7f1ea1561c01424f603b is missing in this ticket

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 40 MB)