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

236 views Asked by At

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;

    }
3

There are 3 answers

10
JFCorleone On BEST ANSWER

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

1
Uday Chauhan On

Use hashmap to store the data and retrive the data.

2
Erunafailaro On

In general, a design pattern called The Chain of Responsibility can help to reduce the complexity that comes with problems that arise when a lot of cases need to be handled by your code.

In (very) short: instead of many many if statements, you would chain a lot of java objects ("receivers"). Each of those would check if:

  • They are responsible for the current situation an then
  • return their result.

If they are not responsible, they would pass on the handling to the next receiver (i.e. their successor).

Ultimately, the chain should contain at least one responsible receiver who would provide an answer.

Each receiver/handler would only contain exactly one if statement.

So what this pattern does, basically, is dividing the complexity and separate it over multiple classes.

Chain of responsibility descibed with UML

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