Improving large switch case-CodePudding

I have an API (built with spring boot) which has a method that provides a template file on format .xlxs delivered on a response DTO object with a base64 inside of it.

    @GetMapping("/getLoadTemplate")
    public ResponseEntity<?> getLoadTemplate(@RequestParam(name = "type", defaultValue = "!") String type) {
        ResponseDTO responseDTO;
        if (notInTypes(type))
            return ResponseEntity.badRequest().body(badRequest("Tipo de archivo invalido."));
        try {
            responseDTO = loadQuotasService.getFileTemplate(type);
            return ResponseEntity.status(responseDTO.getStatus()).body(responseDTO);
        } catch (Exception e) {
            return ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE).body(errorResponse(logger, e));
        }
    }

The request object is a simple string that can be on any of the types of the classes that have a xlxs template, so each class has it own LOAD_NAME value. Example public final static String LOAD_NAME = "POLITICAS_AFILIADO"

public static boolean notInTypes(String type){
        if(type == null) return true;
        String[] validTypes = {QuotaLoad.LOAD_NAME, QuotaModify.LOAD_NAME, QuotaState.LOAD_NAME,
                AffiliatePolitics.LOAD_NAME, AffiliateRisk.LOAD_NAME, BuyerEvaluation.LOAD_NAME,
                HabeasRegister.LOAD_NAME, QuotaBuyer.LOAD_NAME, ClintonList.LOAD_NAME, BuyerQualification.LOAD_NAME};
        return !Arrays.asList(validTypes).contains(type);
    }

PROBLEM This is the service method ... It's filled of repetitive code and a super big switch block. Will be difficult to mantainf

public ResponseDTO getFileTemplate(String type) {
        ASRFile template = null;
        switch (type) {
            case QuotaLoad.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(QuotaLoad.TEMPLATE, QuotaLoad.CATEGORY);
                break;
            case QuotaModify.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(QuotaModify.TEMPLATE, QuotaModify.CATEGORY);
                break;
            case QuotaState.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(QuotaState.TEMPLATE, QuotaState.CATEGORY);
                break;
            case AffiliatePolitics.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(AffiliatePolitics.TEMPLATE,
                        AffiliatePolitics.CATEGORY);
                break;
            case AffiliateRisk.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(AffiliateRisk.TEMPLATE,
                        AffiliateRisk.CATEGORY);
                break;
            case BuyerEvaluation.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(BuyerEvaluation.TEMPLATE,
                        BuyerEvaluation.CATEGORY);
                break;
// more equal code ...

How can I impprove this repetitive code?

CodePudding user response：

You can just use a Map (as I don't know the type of your category, I'll use TypeOfCategory instead):

private static final Map<String, TypeOfCategory> typeCategories = Map.of(
        QuotaLoad.TEMPLATE, QuotaLoad.CATEGORY,
        ... );

You can use this map to rewrite notInTypes but getFileTemplate will be reduced significantly:

public ResponseDTO getFileTemplate(String type) {
    if (typeCategories.containsKey(type)) {
         return fileRepository.findByFileOriginalNameAndCategory(type, typeCategories.get(type));
    }
    return null; // or a dummy, or throw an exception...
}

CodePudding user response：

You can merge your 2 service methods in the following way, so that in the end you can remove completely the method boolean notInTypes(String type), which has duplicate references of the static code that you use in the other service method getFileTemplate(String type).

        @GetMapping("/getLoadTemplate")
        public ResponseEntity<?> getLoadTemplate(@RequestParam(name = "type", defaultValue = "!") String type) {
         
            try {
                ResponseDTO responseDTO = loadQuotasService.getFileTemplate(type);
                return responseDTO != null ? ResponseEntity.status(responseDTO.getStatus()).body(responseDTO) : ResponseEntity.badRequest().body(badRequest("Tipo de archivo invalido."))
            } catch (Exception e) {
                return ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE).body(errorResponse(logger, e));
            }
        }

Then you would have

public ResponseDTO getFileTemplate(String type) {
     
        //Switch will break if passed argument is null!
        if (type == null) {
           return null;
        }
        ASRFile template = null;
        switch (type) {
            case QuotaLoad.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(QuotaLoad.TEMPLATE, QuotaLoad.CATEGORY);
                break;
            case QuotaModify.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(QuotaModify.TEMPLATE, QuotaModify.CATEGORY);
                break;
            case QuotaState.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(QuotaState.TEMPLATE, QuotaState.CATEGORY);
                break;
            case AffiliatePolitics.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(AffiliatePolitics.TEMPLATE,
                        AffiliatePolitics.CATEGORY);
                break;
            case AffiliateRisk.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(AffiliateRisk.TEMPLATE,
                        AffiliateRisk.CATEGORY);
                break;
            case BuyerEvaluation.LOAD_NAME:
                template = fileRepository.findByFileOriginalNameAndCategory(BuyerEvaluation.TEMPLATE,
                        BuyerEvaluation.CATEGORY);
                break;
             default:
             return null;  // <---------- If type is not matched in switch then it is not a valid type
     }

CodePudding user response：

You can use Spring to autowire little objects for you that contain all the information without knowledge of them inside your service. For example define

public class FileInfo {
    public final String type;
    public final String template;
    public final String category;

    public FileInfo(String type, String template, String category) {
        this.type = type;
        this.template = template;
        this.category = category;
    }
}

And in each of your classes (QuotaLoad, ...) give spring an instance of that type.

public class SomeClass {
    @Component
    static class SomeClassInfo extends FileInfo {
        public SomeClassFileProvider() {
            super("TYPE_A", "abfcf134===", "CATEGORY_A");
        }
    }
}

Now use them in a Service like so

@Service
class MyService {
    private final Map<String, FileInfo> supportedTypes;

    // all of them are autowired here
    public MyService(List<FileInfo> fileInfos) {
        this.supportedTypes = fileInfos.stream().collect(
            Collectors.toMap(
                it -> it.type,
                it -> it
            ));
    }

    public boolean isValidType(String type) {
        return supportedTypes.containsKey(type);
    }

    public String getTemplate(String type) {
        FileInfo info = supportedTypes.get(type);
        return info != null ? info.template : null;
    }

    public String getCategory(String type) {
        FileInfo info = supportedTypes.get(type);
        return info != null ? info.category : null;
    }

    // that would be the former big switch
    public ResponseDTO getFileTemplate(String type) {
        FileInfo info = supportedTypes.get(type);
        if (info == null)
            return null;
        return fileRepository.findByFileOriginalNameAndCategory(info.template, info.category);
    }
}

If your classes already are spring beans, you can also just autowire them and enforce the required fields in them by letting them implement a common interface.