Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

Can't Split Class into Smaller Ones

Tags:

java

sonarlint

I have a trouble with splitting my class into smaller parts. We have a bad situation where a Dto holds 30 different Dtos. Now we need this selectDto's mapping which also force us make 30 different mapping class. (We also use mapstruct in the project, this scenario is different than mapstruct can handle)

Now where my problem starts:

I made all mappings in corresponding class. In the base selectDto, I have 26 mappers in my constructor which is horrible:

SonarLint: Constructor has 26 parameters, which is greater than 7 authorized

I think out the way how to split this kind of scenario but I couldn't find a way. Any suggestions?

My constructor holding 26 parameters:

    AssignedSelectMapper(AssignedOpDtoMapper assignedOpDtoMapper,
        AssignedOrderDtoMapper assignedOrderDtoMapper
// many more constructor parameters
) {
        this.assignedOptionCodeDtoMapper = assignedOptionCodeDtoMapper;
        this.assignedOrderCriteriaDtoMapper = assignedOrderCriteriaDtoMapper;
        // all settings
}

my public function to map which calls private functions for each mapping:

    public List<AssignedSelect> assignSelectFrom(SelectDto selectDto) {
        Objects.requireNonNull(selectionDto, "selectionDto can not be NULL");

        List<AssignedSelect> assignedSelects= new ArrayList<>();
        assignedSelects.addAll(this.mapOps(selectionDto.getOps()));
        assignedSelects.addAll(this.mapOra(selectionDto.getOra()));
        assignedSelects.addAll(this.mapOrs(selectionDto.getOrs()));
        assignedSelects.addAll(this.mapSs(selectionDto.getSs()));
        assignedSelects.addAll(this.mapDels(selectionDto.getDels()));
        assignedSelects.addAll(this.mapMs(selectionDto.getMs()));
        assignedSelects.addAll(this.mapBrs(selectionDto.getBrs()));
        assignedSelects.addAll(this.mapEqs(selectionDto.getEqs()));
        assignedSelects.addAll(this.mapPaints(selectionDto.getPaints()));
        assignedSelects.addAll(this.mapBas(selectionDto.getBas()));
// more ...
// and more...
return assignedSelects;
}

// example of my private function which calls corresponding mapper where all of my private function consist of different kind of class like OptionCodeDto here. They don't extend from same interface/class and can't.

 private List<AssignedSelectionCriteria> mapOps(List<OptionCodeDto> optionCodeDtos) {
        return this.assignedOpDtoMapper.mapCriterias(opDtos);
    }

// and here is reverse mapping. Which I need different kind of class from my mappings return type

// this is my public function for mapping.
public void assignSelectionTo(SelectionDto selectionDto,
    List<AssignedSelectionCriteria> assignedSelectionCriterias) {

    setOptionCodes(selectionDto, copyCriterias);
    setOrderCriteria(selectionDto, copyCriterias);
    // many more
}

and this is the reverse mapping private function which every mapping class returns different kind of dto's like OptionCodeDto where none of those Dto's extend from same class.

private void setOptionCodes(SelectionDto selectionDto, List<AssignedSelectionCriteria> assignedSelectionCriterias) {
// this is where I have trouble, each mapping returns different Dto's
        List<OptionCodeDto> optionCodeDtos =
             this.assignedOptionCodeDtoMapper.mapAssignedCriterias(assignedSelectionCriterias;
        selectionDto.setOptionCodes(optionCodeDtos);
    }
like image 874
cmlonder Avatar asked Jan 09 '19 11:01

cmlonder


2 Answers

This acts like an extension to @Michaels answer.

The idea with an interface is a great idea. Though in my opinion the interface can be changed to be more suiting for your use case:

interface SelectDtoProcessor {
     void process(SelectDto dto, 
                  Consumer<? super Collection<? extends AssignedSelect>> action);
}

Now every mapper implements this interface:

class AssignedOpDtoMapper implements SelectDtoProcessor {
    @Override
    public void process(SelectDto dto, 
                        Consumer<? super Collection<? extends AssignedSelect>> action){
        List<OptionCodeDto> ops = dto.getOps();
        consumer.accept(mapCriterias(ops));
}

And then inject all those Processors into your main class:

private final List<SelectDtoProcessor> processors;

AssignedSelectMapper(List<SelectDtoProcessor> processors) {
    this.processors = processors;
}

And finally iterate over all processors in your method:

public List<AssignedSelect> assignSelectFrom(SelectDto selectDto) {
    Objects.requireNonNull(selectionDto, "selectionDto can not be NULL");

    List<AssignedSelect> assignedSelects= new ArrayList<>();
    for(SelectDtoProcessor processor: processors) {
        processor.process(selectDto, assignedSelects::addAll);
    }
    return assignedSelects;
}
like image 174
Lino Avatar answered Oct 18 '22 18:10

Lino


Create an interface AssignedSelectProvider

interface AssignedSelectProvider
{
    List<AssignedSelect> getAssignedSelects();
}

each mapper implements the interface, and each private method moves to the relevant class:

class AssignedOpDtoMapper implements AssignedSelectProvider
{
    public List<AssignedSelect> getAssignedSelects()
    {
        return mapOps(getOps());
    }
}

AssignedSelectMapper gets a list of providers in the constructor instead of 26 parameters:

class AssignedSelectMapper
{
    AssignedSelectMapper(List<AssignedSelectProvider> mappers)
    {
        //...
    }
}

and that's 26 parameters down to 1.

like image 2
Michael Avatar answered Oct 18 '22 18:10

Michael