How to optimize java function which has lot of if else statements in java?

Question

I am a newbie and just started working on code, i have a task at hand and after doing a bit of research i am still not able to find the answer.

I have a java function which uses a lot of if else statements, my task is to optimize the code since nest if else is not very good programming so i've been told.

First i thought switch statement is the way to go but then i came across people telling to use map, i don't know what map is and the examples online are very hard to even understand.

Here is the code i have to optimize

public static Boolean SurveyValidObject(JSONObject jSONObject) {

    Boolean message = false;

    if (jSONObject != null) {

        Iterator it = jSONObject.keys();
        ArrayList<String> keysList = new ArrayList<String>();
        while (it.hasNext()) {
            String key = (String) it.next();
            if (key.contains("field_")) {
                int i = key.indexOf("_");
                String _fieldValue = key.substring(0, i + 1);
                keysList.add(_fieldValue);
            } else {
                keysList.add(key);
            }
        }
        ArrayList<String> userDatalist = new ArrayList<String>();
        userDatalist.add("survey_id");
        userDatalist.add("source_id");
        userDatalist.add("sso_id");
        userDatalist.add("email_id");
        userDatalist.add("field_");

        Boolean returnValue = keysList.containsAll(userDatalist);

        Iterator iterator = jSONObject.keys();
        if (returnValue) {
            while (iterator.hasNext()) {
                try {

                    String key = (String) iterator.next(); // get key
                    String value = jSONObject.getString(key); // get value

                    if (key.equals("survey_id")) {

                        if (value == null) {
                            message = false;
                            break;
                        } else {
                            Boolean checkInteger = value.matches("\\d+");
                            if (!checkInteger) {
                                message = false;
                                break;
                            }
                        }

                    }
                    if (key.equals("crm_id")) {

                        if (value == null) {
                            message = false;
                            break;
                        } else {
                            Boolean checkInteger = value.matches("\\d+");
                            if (!checkInteger) {
                                message = false;
                                break;
                            }
                        }

                    }
                    if (key.equals("source_id")) {
                        if (value == null) {
                            message = false;
                            break;

                        } else {
                            Boolean checkInteger = value.matches("\\d+");
                            if (!checkInteger) {
                                message = false;
                                break;
                            }
                        }
                    }
                    if (key.equals("sso_id")) {
                        if (value == null) {
                            message = false;
                            break;

                        } else {

                            Boolean checkId = value.matches("^[ A-Za-z0-9\\\"\\$%^&()!*:;<>?{}_@.\\/#+-/'']*$");
                            String compareSSOValue = "/";
                            if (!checkId || compareSSOValue.equals(value)) {
                                message = false;
                                break;
                            }
                        }
                    }

                    if (key.equals("email_id")) {
                        if (value == null) {
                            message = false;
                            break;

                        } else {

                            message = value.matches("\\b[\\w.%-'-]+@[-.\\w]+\\.[A-Za-z]{2,4}\\b");
                            if (!message) {
                                break;
                            }

                        }
                    }

                    if (key.contains("field_")) {
                        int i = key.indexOf("_");
                        String _fieldValue = key.substring(i + 1);
                        Boolean checkInteger = _fieldValue.matches("\\d+");
                        if (checkInteger) {
                            message = true;
                        } else {
                            message = false;
                            break;
                        }
                    }

                } catch (org.json.JSONException e) {

                }
            }
        }
    }
    return message;
}

So, are you trying to optimize performance (to which I would then ask if you know for a fact that you have a performance problem)? Or are you trying to make the code more readable (pretty, elegant, concise, etc.)? Those are 2 very different objectives. — sstan, Dec 24 '15 at 14:02
You have a `break` in both the inner `if` and the `else`. Since that means that the `break` has to be executed in either condition, you can move it outside. — Sinstein, Dec 24 '15 at 14:02
http://stackoverflow.com/questions/1199646/long-list-of-if-statements-in-java — Enno Shioji, Dec 24 '15 at 14:04
A map is basically an object with two similarly sized collections. One collections holds the so called keys, and the other holds the values. You can perform a .get(key) on your map and it will return the associated value (the value on the same index(index in collection 1) in the other collection). Eg: Map map = new HashMap(); map.put(3,"three"); map.put(4,"four"); map.get(4) // Returns the string "four"; map.get(3) // Returns the string three. You can in theory see it as a tabel with in the left column the keys, and on the right values. — MrKickkiller, Dec 24 '15 at 15:36

score 4 · Answer 1 · answered Dec 24 '15 at 14:21

The easiest (and most future-proof) option would be to pull all those checks out into their own place - I usually use an enum for that.

As you want to use a String to work out which check to make then we can make the enum build a Map.

static final Map<String, KeyCheck> lookup = new HashMap<>();

enum KeyCheck {

    SurveyId("survey_id"),
    CRMId("crm_id"),
    SourceID("source_id"),
    SSOId("sso_id") {
                @Override
                boolean check(String value) {
                    return value != null
                    && value.matches("^[ A-Za-z0-9\\\"\\$%^&()!*:;<>?{}_@.\\/#+-/'']*$")
                    && !value.equals("/");
                }
            },
    EMailId("email_id"){
                @Override
                boolean check(String value) {
                    return value != null
                    && value.matches("\\b[\\w.%-'-]+@[-.\\w]+\\.[A-Za-z]{2,4}\\b");
                }
            };

    KeyCheck(String key) {
        // All checks install in a Map.
        lookup.put(key, this);
    }

    // Default to just check not null & integer.
    boolean check(String value) {
        return value != null && value.matches("\\d+");
    }
}

Your loop then simplifies drastically:

            while (iterator.hasNext()) {
                try {

                    String key = (String) iterator.next(); // get key
                    String value = jSONObject.getString(key); // get value

                    KeyCheck check = lookup.get(key);
                    if (check != null) {
                        message = check.check(value);
                    }
                    // Do something else with this.
                    if (key.contains("field_")) {
                        int i = key.indexOf("_");
                        String _fieldValue = key.substring(i + 1);
                        Boolean checkInteger = _fieldValue.matches("\\d+");
                        if (checkInteger) {
                            message = true;
                        } else {
                            message = false;
                            break;
                        }
                    }

                } catch (org.json.JSONException e) {

                }
            }

BTW: Notice that you will change the value of message for all keys in the sequence so you are only actually checking the last key, not all of them. This may be a bug.

This is the best solution for a series of comparisons against a set of values known at compile time (+1). — scottb, Dec 24 '15 at 17:12

tharindu_DG · Answer 2 · 2015-12-24T14:23:08.747

There are lot of code duplicates. If you refactor them, you could remove lot of the if statements.

E.G null check for value is repeated in every if statement.

                    if (value == null) {
                        message = false;
                        break;
                    }

And the below if statement also repeated.

                   Boolean checkInteger = value.matches("\\d+");
                    if (!checkInteger) {
                        message = false;
                        break;
                    }

Instead of above repeated codes you could do the following.

                String key = (String) iterator.next(); // get key
                String value = jSONObject.getString(key); // get value

                if (value == null) {
                    return false; // return false and exit the method
                }

                Boolean checkInteger = value.matches("\\d+");

                if(key.equals("survey_id") || key.equals("crm_id")){
                    if (!checkInteger) {
                        return false;
                    }
                }

Likewise try to refactor the code isolating duplicates.

Below is what I could achieve by removing duplicate code.

try {

    String key = (String) iterator.next(); // get key
    String value = jSONObject.getString(key); // get value

    if (value == null) {
        return false;
    }

    if (key.equals("survey_id") || key.equals("crm_id") || key.equals("source_id")) {

        Boolean checkInteger = value.matches("\\d+");
        if (!checkInteger) {
            return false;
        }
    }

    if (key.equals("sso_id")) {

        Boolean checkId = value.matches("^[ A-Za-z0-9\\\"\\$%^&()!*:;<>?{}_@.\\/#+-/'']*$");
        String compareSSOValue = "/";
        if (!checkId || compareSSOValue.equals(value)) {
            message = false;
            break;
        }

    }

    if (key.equals("email_id")) {

        message = value.matches("\\b[\\w.%-'-]+@[-.\\w]+\\.[A-Za-z]{2,4}\\b");
        if (!message) {
            break;
        }
    }

    if (key.contains("field_")) {
        int i = key.indexOf("_");
        String _fieldValue = key.substring(i + 1);
        Boolean checkInteger = _fieldValue.matches("\\d+");
        if (checkInteger) {
            message = true;
        } else {
            message = false;
            break;
        }
    }

} catch (org.json.JSONException e) {

}

Hope this helps.

Bahramdun Adil · Answer 3 · 2015-12-24T14:24:12.800

In code above two places you can make more readable and can skip many if-else clauses:

In this place if (jSONObject != null) you can change it as

if (jSONObject == null) {
    return false;
}

// the rest of the code will goes here

And the same to if (returnValue)

In the second while loop, I saw many times if (value == null), you need check only once, then you can change it as below:

while (iterator.hasNext()) {
    try {
        String key = (String) iterator.next(); // get key
        String value = jSONObject.getString(key); // get value

        if (value == null) {
            // if the value was null, then directly break;
            message = false;
            break;
         }
           // then many if else clauses you can skip like:
           if (key.equals("survey_id")) {
               Boolean checkInteger = value.matches("\\d+");
               if (!checkInteger) {
                  message = false;
                  break;
                }
                // no need for ths
                /*if (value == null) {
                        message = false;
                        break;
                    } else {
                        Boolean checkInteger = value.matches("\\d+");
                        if (!checkInteger) {
                            message = false;
                            break;
                        }
                    }

}*/

         // the rest of the code goes here.
         //...
    }
}

These 2 points that I suggested. Hope this helps you!

Is using `if (jSONObject == null)` more performant than `if (jSONObject != null)`? Should the more common case go int the `if` block? — Eric, Jul 14 '23 at 23:06

How to optimize java function which has lot of if else statements in java?

3 Answers3