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?

Answer 1

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.

Answer 2

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.

Answer 3

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

Answer 4

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

Answer 5

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.

Answer 6

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.