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

Question

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;

    }

You could make them all enumerations and hide the method in the enumeration class. You could also use a switch statement, but that will essentially look like the if — Beez, Aug 23 '22 at 18:11
Questions about reviewing running is better asked at [codereview.se] — Jens, Aug 23 '22 at 18:13
There are several [variations on this question](https://www.google.com/search?q=java+avoid+many+if+statements+site:stackoverflow.com) already, on Stack Overflow. Have you looked into any of their solutions? — andrewJames, Aug 23 '22 at 18:15

score 2 · Answer 1 · answered Aug 23 '22 at 18:42

2

Use hashmap to store the data and retrive the data.

answered Aug 23 '22 at 18:42

Uday Chauhan

1,071
1
9
19

1

I would do the same. One line of code to test also. – Pp88 Aug 23 '22 at 19:54

score 1 · Answer 2 · answered Aug 23 '22 at 18:49

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.

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

Please, chain of responsibility for such a simple streategy? That's a massive overkill — JFCorleone, Aug 23 '22 at 18:58
Not necessarily. The chain would be much easier to maintain if another case pops up. Just add another handler. Furthermore, the complexity of each handler would be very small and easy to unterstand. Especially if you have experience developers who have knowledge of design patterns. And writing tests for the handlers would also be easier due to their reduced complexity. At the end of the day, you can object to every sophisticated implementation that it is overkill. In this case I would say it isn't. — Erunafailaro, Aug 23 '22 at 19:02

JFCorleone · Accepted Answer · 2022-08-23T18:36:59.240

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

Thanks for your reply, I forgot to mention that i'm using java 8, is this still usable ? — JhinKazama, Aug 23 '22 at 18:22
If you want to maybe divide it into more readable parts I would split into multiple methods, let me quickly check and I'll update the answer — JFCorleone, Aug 23 '22 at 18:26
Thanks, i'm trying to use a map but since there are differents key/values I guess it is not worth — JhinKazama, Aug 23 '22 at 18:31
good coding imo is not always about the fanciest solution but most readable one. you need to be able to navigate through code like through a good book :) — JFCorleone, Aug 23 '22 at 18:40
You're right, thanks for your reply, it's so much better like this — JhinKazama, Aug 23 '22 at 18:45
This solution seems to be missing a few return values suggesting incorrectness: `Publicité###Flyers` , `Publicité###Presse` — Computable, Aug 23 '22 at 19:19
@Gardener yes you're completely right, as I said, I was in a hurry, was trying to get the idea out there. He can work on the details from there ;) — JFCorleone, Aug 23 '22 at 20:39

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

3 Answers3