Search code examples
javaif-statementfactorizationcognitive-complexity

How can I avoid using too much if statements / refactor this method?


It looks horrible, but I don't see how can I factorize that ?
I thought of creating small boolean methods but I think it won't change too much, there will always be as many ifs ?

private String getFolderValue(TableRow row) {
        String cote = row.getCellValue("B");
        String typologie = row.getCellValue("G");
        String description = row.getCellValue("Q");
        if (cote.startsWith("DE")) {
            return "Dessins";
        }
        if (cote.startsWith("PH")){
            return "Photographies";
        }
        if(cote.startsWith("CA")) {
            return "Catalogues";
        }
        if(cote.startsWith("PU") && typologie.contains("affiche")){
            return "Publicité###Affiches";
        }

        if(cote.startsWith("PU") && typologie.contains("flyer")){
            return "Publicité###Flyers";
        }

        if(cote.startsWith("PU") && description.contains("presse")){
            return "Publicité###Presse";
        }

        if(cote.startsWith("PU") && (description.contains("Facture") || description.contains("devis"))){
            return "Documents###Vente";
        }

        if(typologie.contains("Emballage")){
            return "Visual Merchandising###Flyers";
        }

        if(typologie.contains("PLV")){
            return "Visual Merchandising###PLV";
        }
        if(description.contains("Correspondances")){
            return "Documents###Correspondances";
        }

        return null;

    }

Solution

  • Ok, first of all let's notice that one of your conditions is repeated a lot. Let's try with a nested "if" and let's use StreamAPI.

        private String getFolderValue(TableRow row) {
            String cote = row.getCellValue("B");
            String typologie = row.getCellValue("G");
            String description = row.getCellValue("Q");
            if (cote.startsWith("DE")) {
                return "Dessins";
            }
            if (cote.startsWith("PH")) {
                return "Photographies";
            }
            if (cote.startsWith("CA")) {
                return "Catalogues";
            }
            if (cote.startsWith("PU")) {
                final var topologyContains = Stream.of("flyer", "presse", "affiche").anyMatch(typologie::contains);
                if (topologyContains) {
                    return "Publicité###Affiches";
                }
    
                final var descriptionContains = Stream.of("Facture", "devic").anyMatch(description::contains);
                if (descriptionContains) {
                    return "Documents###Vente";
    
                }
            }
            
            if (typologie.contains("Emballage")) {
                return "Visual Merchandising###Flyers";
            }
    
            if (typologie.contains("PLV")) {
                return "Visual Merchandising###PLV";
            }
            if (description.contains("Correspondances")) {
                return "Documents###Correspondances";
            }
            return null;
        }
    

    I wouldn't get too much into refactoring here, as this is a simple strategy pattern and in some cases the more "generic" and fancy you go, the worse it is in the end, as it has to be simple to read.

    EDIT: Let's divide into functions. You need to test it as I was in a hurry, but you should get the gist:

       private String getFolderValue(TableRow row) {
            String cote = row.getCellValue("B");
            String typologie = row.getCellValue("G");
            String description = row.getCellValue("Q");
    
            return Stream.of(
                parseFromCote(cote),
                parseFromCotePU(cote, typologie, description),
                parseFromTypologie(typologie),
                parseFromDescription(description)
              )
              .filter(Objects::nonNull)
              .findFirst()
              .orElse(null);
        }
    
        private String parseFromCote(String cote) {
            if (cote.startsWith("DE")) {
                return "Dessins";
            }
            if (cote.startsWith("PH")) {
                return "Photographies";
            }
            if (cote.startsWith("CA")) {
                return "Catalogues";
            }
            return null;
        }
    
        private String parseFromCotePU(String cote, String typologie, String description) {
            if (cote.startsWith("PU")) {
                final var topologyContains = Stream.of("flyer", "presse", "affiche").anyMatch(typologie::contains);
                if (topologyContains) {
                    return "Publicité###Affiches";
                }
    
                final var descriptionContains = Stream.of("Facture", "devic").anyMatch(description::contains);
                if (descriptionContains) {
                    return "Documents###Vente";
    
                }
            }
            return null;
        }
    
        private String parseFromTypologie(String typologie) {
            if (typologie.contains("Emballage")) {
                return "Visual Merchandising###Flyers";
            }
    
            if (typologie.contains("PLV")) {
                return "Visual Merchandising###PLV";
            }
            return null;
        }
    
        private String parseFromDescription(String description) {
            if (description.contains("Correspondances")) {
                return "Documents###Correspondances";
            }
            return null;
        }
    

    If you're using Java8 replace final var with proper type. If you prefer to do it lazily you need to pass references to Functional Interfaces there, but I think even "eager" evaluation looks not that bad ;)