0
\$\begingroup\$

I have an entity called Travel which has list of costCenter. I have created a code for handling CRUD operation for cost centers

protected void assignCostCenter(CostCenterDTO costCenterDTO, Travel travel, Long detailsId) throws BusinessException {
    CostCenter costCenter = null;
    if (costCenterDTO.getCostCenters() != null) {
        Center center = commonDao.find(costCenterDTO.getCostCenters().getId());
        costCenter = costCenterDTOMapper.mapToDTO(costCenterDTO.getCostCenters(), travel, detailsId, center);
        if (costCenter != null && !travel.getCostCenters().contains(costCenter)) {
            travel.getCostCenters().add(costCenter);
        }
    } else {
        travel.setCostCenters(travel.getCostCenters().stream().filter(costCenter1 -> !(costCenter1.getDetailId().equals(detailsId))).collect(Collectors.toList()));
    }
}

And in the mapper

 public CostCenter mapToCostCenter(CostCenterDTO costCenterDTO, Travel travel, Long claimDetail, Center center) throws BusinessException {
        CostCenter costCenter = new CostCenter();
        CostCenter costCenterToUpdate = travel.getCostCenters()
                .stream()
                .filter(costCenter1 -> costCenter1.getDetailId().equals(claimDetail))
                .findFirst()
                .orElse(null);
        if (costCenterToUpdate == null) {
            costCenter.setCenter(center);
            costCenter.setTravel(travel);
            costCenter.setDetailId(claimDetail);
        } else {
            costCenterToUpdate.setCenter(center);
            return costCenterToUpdate;
        }
        return costCenter;
    }

The functionality is that if new CostCenter is assigned, I fetch Center from db which was assigned and pass to mapper. In mapper I call travel.getCostCenters to see if there is no costCenter already assigned with same detail id if there is I just update it, if there is no I create new and return, and add to list if doesn't exist. Also if CostCenterDTO comes as null then I remove from list costCenter by detailId. My question is how I could refactor it because it doesn't look tidy?

\$\endgroup\$
1
\$\begingroup\$

I think the mapper looks ok, but in the cost centers management method, you violate encapsulation by directly manipulating the travel's cost centers list.

I'd rather leave the list management to the travel class:

class Travel {
    ...
    void addCostCenter(CostCenter c) {
       ...
    }

    void removeCostCenter(CostCenter c) {
       ...
    }

    boolean containsCostCenter(CostCenter c) {
        ...
    }

    CostCenter findByDetails(Long detailsId) {
        ....
    }
}

... and use it like this:

protected void assignCostCenter(CostCenterDTO costCenterDTO, Travel travel, Long detailsId) throws BusinessException {
    // note: moved outside declaration of CostCenter to smaller scope
    if (costCenterDTO.getCostCenters() != null) {
        Center center = commonDao.find(costCenterDTO.getCostCenters().getId());
        CostCenter costCenter = costCenterDTOMapper.mapToDTO(costCenterDTO.getCostCenters(), travel, detailsId, center);
        // can costCenterDTOMapper.mapToDTO actually return null? If not, declare it like that and
        // remove the ugly null-check
        if (costCenter != null && !travel.containsCostCenter(costCenter)) {
            // depending on the business logic, you might also consider
            //    travel.addCostCenterIfNotExists(costCenter)
            // and skip the if
            travel.addCostCenter(costCenter);
        }
    } else {
        travel.removeCostCenter(travel.findByDetails(detailsId));
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You should move the contains and not-null check of the costCenter to Travel (as in your comment). It's the responibility of Travel to keep its own state consistent. \$\endgroup\$ – RobAu May 12 '17 at 8:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.