Home > Software design >  How can I avoid using too much if statements / refactor this method?
How can I avoid using too much if statements / refactor this method?

Time:08-24

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;

    }

CodePudding user response:

Use hashmap to store the data and retrive the data.

CodePudding user response:

In general, a design pattern called Chain of responsibility descibed with UML

Picture by Vanderjoe -- license: CC BY-SA 4.0

CodePudding user response:

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 ;)

  • Related