Reducing the cyclomatic complexity of a java method

Question

I have the following method:

private void setClientAdditionalInfo(Map map, Client client, User user) {

    Map additionalInfo = (Map) map.get("additionalInfo");

    if (checkMapProperty(additionalInfo, "gender")) {
        client.setGender(additionalInfo.get("gender").toString());
    }
    if (checkMapProperty(additionalInfo, "race")) {
        client.setRace(additionalInfo.get("race").toString());
    }
    if (checkMapProperty(additionalInfo, "ethnicity")) {
        client.setEthnicity(additionalInfo.get("ethnicity").toString());
    }
   .....

12 more if statements are used in the similar way. The only difference being a different setter method name and a different parameter. Now, as the same pattern is repeated again and again, is there a way to reduce the code complexity?

You could create a map `{"race": Client::setRace, ...}` or use reflection to find the appropriate setter for a list of strings. Not sure if this will reduce the complexity, but it might reduce duplication. I guess I would just keep it that way. Better to think "why is this repeated 15 times?" than to think "what the heck is this code doing?" — tobias_k, Oct 22 '15 at 15:12
A question that may be worth asking: why is your information in a map? Could you not have stored your information in the `Client` earlier? Often the answer is "no" and you've got to bite the bullet but sometimes you can avoid having to populate objects from maps altogether. — biziclop, Oct 22 '15 at 15:21
Do you mean "cyclomatic complexity" or "fewer lines of code?" a for loop over N things can add 2^N to the cyclomatic complexity even though it might look better. You're getting a lot of answers for how to improve the code that don't change the cyclomatic complexity. — djechlin, Oct 22 '15 at 15:29
@djechlin I need to reduce the cyclomatic complexity first. But if that's not possible, I really won't like to keep repeating the same thing atleast. — Akeshwar Jha, Oct 22 '15 at 15:32
@djechlin Can you lower the cyclomatic complexity below the complexity of the input? If you have a collection of N independent attributes which are either present or absent, that gives you `2^N` different inputs. — biziclop, Oct 22 '15 at 15:33
@biziclop actually looks like I'm really wrong, now that I take time to understand the definition more thoroughly. It's a theorem that it's #(ifs or loops) + 1, and I think you have to count "else if" separately. So reducing to a loop is a win. — djechlin, Oct 22 '15 at 15:38
@biziclop some of the answers are still refactorings that just move the "if" somewhere else, though. — djechlin, Oct 22 '15 at 15:39

score 2 · Answer 1 · answered Oct 22 '15 at 15:11

Not easily, and not without using reflection.

Using reflection you could loop through the list of values and call the appropriate method in the client object. That would get rid of the complexity and be cleaner/more robust. However it would also perform slower.

Fundamentally though you have the case where you are doing nearly but not quite the same operation over and over, that's always tricky.

score 1 · Answer 2 · answered Oct 22 '15 at 15:28

You can do this with Java 8 functional interfaces. It'll at least get rid of the repeated conditional statements.

private void doRepetitiveThing(Map info, String key, Consumer<String> setterFunction) {
   if(checkMapProperty(info, key)) {
       setterFunction.accept(info.get(key).toString());
   }
}

private void setClientAdditionalInfo(Map map, Client client, User user) {

    Map additionalInfo = (Map) map.get("additionalInfo");

    doRepetitiveThing(additionalInfo, "gender", client::setGender);
    doRepetitiveThing(additionalInfo, "race", client::setRace);
    doRepetitiveThing(additionalInfo, "ethnicity", client::setEthnicity);
   .....

Same cyclomatic complexity. – djechlin Oct 22 '15 at 15:28 — djechlin, Oct 22 '15 at 15:28

score 1 · Answer 3 · answered Oct 22 '15 at 15:35

Not sure if this actually reduces the cyclomatic complexity, but it makes the code prettier. This is easier with Java 8.

private void setClientAdditionalInfo(Map<String, Object> map, Client client, User user) {
    Map<String, ?> additionalInfo = (Map<String, Object>) map.get("additionalInfo");
    setIfPresent(additionalInfo, "gender", client::setGender);
    setIfPresent(additionalInfo, "race", client::setRace);
    setIfPresent(additionalInfo, "ethnicity", client::setEthnicity);
}

private void <T> setIfPresent(Map<String, ?> additionalInfo,
                              String property,
                              Consumer<T> consumer) {
    if (checkMapProperty(additionalInfo, property)) {
        consumer.apply((T)additionalInfo.get(property));
    }
}

If you wanted, you could make a Map<String, Consumer<?>> to avoid the repeated calls.

OldCurmudgeon · Answer 4 · 2015-10-22T15:29:14.190

I'd use an enum to connect the setter up with the key to the map, using the name().toLowercase() as the key.

enum Setter {

    Gender {

                @Override
                void set(Client client, Thing value) {
                    client.setGender(value);
                }

            },
    Race {

                @Override
                void set(Client client, Thing value) {
                    client.setRace(value);
                }

            },
    Ethnicity {

                @Override
                void set(Client client, Thing value) {
                    client.setEthnicity(value);
                }

            };

    void setIfPresent(Client client, Map<String, Thing> additionalInfo) {
        // Use the enum name in lowercase as the key.
        String key = this.name().toLowerCase();
        // Should this one be set?
        if (additionalInfo.containsKey(key)) {
            // Yes! Call the setter-specific set method.
            set(client, additionalInfo.get(key));
        }
    }

    // All enums must implement one of these.
    abstract void set(Client client, Thing value);
}

private void setClientAdditionalInfo(Map map, Client client, User user) {
    Map additionalInfo = (Map) map.get("additionalInfo");
    for (Setter setter : Setter.values()) {
        setter.setIfPresent(client, additionalInfo);
    }
}

Same cyclomatic complexity. – djechlin Oct 22 '15 at 15:28 — djechlin, Oct 22 '15 at 15:28

score 0 · Answer 5 · answered Oct 22 '15 at 15:26

Assuming that the fields of Client can be set to null if the map doesn't contain a property, you can create a class:

class MapWrapper {
   private final Map map;

   MapWrapper(Map map) {
      this.map = map;
   }

   String get(String key) {
       if (checkMapProperty(map,key)) {
          return map.get(key).toString();
       } else {
          return null;
       }
       // or more concisely:
       // return checkMapProperty(map,key) ? map.get(key).toString() : null;
   }
}

And then

private void setClientAdditionalInfo(Map map, Client client, User user) {

    Map additionalInfo = (Map) map.get("additionalInfo");
    MapWrapper wrapper = new MapWrapper(additionalInfo);

    client.setGender(wrapper.get("gender"));
    ...
}

But if the requirement is to leave the fields of Client as they are when there's no corresponding key in the map, this won't help you.

score 0 · Answer 6 · answered Sep 18 '16 at 02:08

Only as an exercise for the purpose of answering OP's question I'd create a Map associating the properties with a setter interface:

private static Map<String, ClientSetter> setters = new HashMap<>();

interface ClientSetter {
    void set(Client client, Object value);
}

static {
    setters.put("gender", new ClientSetter() {
        @Override
        public void set(Client client, Object value) {
            client.setGender(value.toString());
        }
    });
    setters.put("race", new ClientSetter() {
        @Override
        public void set(Client client, Object value) {
            client.setRace(value.toString());
        }
    });
    setters.put("ethnicity", new ClientSetter() {
        @Override
        public void set(Client client, Object value) {
            client.setEthnicity(value.toString());
        }
    });
    // ... more setters
}

Iterate over the available properties and calling the set method for each one available in the setters map:

private void setClientAdditionalInfo(Map map, Client client, User user) {

    Map additionalInfo = (Map) map.get("additionalInfo");

    List<String> additionalInfoKeys = new ArrayList<>(additionalInfo.keySet());
    additionalInfoKeys.retainAll(setters.keySet());
    for (String key: additionalInfoKeys) {
        Object value = additionalInfo.get(key);
        setters.get(key).set(client, value);
    }
}

PS: Obviously this is not suggested for production code. Copying all elements of a collection to a list for intersecting the two sets - for the sake of preventing conditions in the code being tested/written - is quite expensive.

Reducing the cyclomatic complexity of a java method

6 Answers6