CODEWITHSUNDEEP
javacyclomatic-complexity
Fenero
Fenero

Reputation: 11

Reducing cyclomatic complexity

I have a class with some 20+ fields of the same type that are populated during different stages of the object lifecycle.

One of the class methods should return the field value based on the field name.

So far I have something like this:

public String getFieldValue(String fieldName){
switch (fieldName.toLowerCase(){
case "id": return getId();
case "name": return getName();
.....

the problem with this is high cyclomatic complexity.

What would be the easiest way to tackle this?

Upvotes: 0

Views: 260

Answers (4)

Arnaud Claudel
Arnaud Claudel

Reputation: 3138

Edit: Thanks to @Filippo Possenti for his comment

Instead of a switch, you can use a Map.

Here is an example.

static interface C {
    String getA();
    String getB();
    String getC();
}

@FunctionalInterface
static interface FieldGetter {
    String get(C c);
}

static Map<String, FieldGetter> fields = Map.of(
        "a", C::getA,
        "b", C::getB,
        "c", C::getC
);

static String getField(C object, String fieldNameToRetrieve) {
    var getter = fields.get(fieldNameToRetrieve);
    if(getter == null) {
        throw new IllegalArgumentException("unknown field");
    }
    return getter.get(object);
}

Why don't you use reflexion or an existing library for this ? (Or why do you even have this kind of method)

Upvotes: 2

daniu
daniu

Reputation: 14999

Instead of the switch or using a Map, you can use an enum.

enum FieldExtractor implements Function<YourClass, String> {
    ID(YourClass::getId),
    NAME(YourClass::getName); // and so on

    private final Function<YourClass, String> delegate;

    FieldExtractor(Function<YourClass, String> delegate) {
        this.delegate = delegate;
    }
    @Override public String apply(YourClass extractFrom) {
        return delegate.apply(extractFrom);
    }

    static FieldExtractor fromString(String name) {
        return Stream.of(FieldExtractor.values())
                     .filter(fe -> fe.name().equalsIgnoreCase(name))
                     .findFirst()
                     .orElseThrow(IllegalArgumentException::new);
    }
}

Now you can use

public String getFieldValue(String fieldName) {
    return FieldExtractor.fromString(fieldName).apply(this);
}

in your client code.

Upvotes: 0

Filippo Possenti
Filippo Possenti

Reputation: 1410

Assuming that the fieldName possible values match the getters on the bean, you can use Apache's BeanUtils:

https://commons.apache.org/proper/commons-beanutils/apidocs/org/apache/commons/beanutils/PropertyUtils.html#getSimpleProperty-java.lang.Object-java.lang.String-

Basically, you could do something like this:

public String getFieldValue(String fieldName){
    return PropertyUtils.getSimpleProperty(fieldName.toLowerCase());
}

This is more about improving code readability than improving cyclomatic complexity so if it's pure performance what you're after, this may not be your solution.

If pure performance is what you're after, you could try and leverage lambdas and a Map.

import java.util.Map;
import java.util.HashMap;
import java.util.function.Function;

public class HelloWorld{
    public static class MyClass {
        private static Map<String, Function<MyClass, Object>> descriptor;

        static {
            descriptor = new HashMap<>();
            descriptor.put("id", MyClass::getId);
            descriptor.put("name", MyClass::getName);
        }

        private String id;
        private String name;

        public String getId() {
            return id;
        }

        public String getName() {
            return name;
        }

        public void setId(String value) {
            id = value;
        }

        public void setName(String value) {
            name = value;
        }

        public Object getFieldValue(String fieldName) {
            Function fn = descriptor.get(fieldName);
            return fn.apply(this);
        }
    }


    public static void main(String []args){
        MyClass mc = new MyClass();
        mc.setId("hello");
        mc.setName("world");

        System.out.println(mc.getFieldValue("id") + " " + mc.getFieldValue("name"));
    }
}

To note that in the above example the cyclomatic complexity is somewhat still there, but it's moved in the class' static initialiser. This means that you'll suffer a modest penalty during application startup but enjoy higher performance in subsequent calls of getFieldValue. Also, if performance is what you're after you may want to eliminate the need for toLowerCase... which in my example I removed.

Upvotes: 0

Karol Dowbecki
Karol Dowbecki

Reputation: 44932

In theory you could reduce the getFieldValue() method complexity by:

  • storing the getter method reference as Producer<?> in Map<String, Producer<?>>
  • using reflection to lookup fields
  • using 3rd party library that supports querying the bean by property name e.g. commons-beanutils.

Each of these approaches will however increase the getFieldValue() method complexity and potentially reduce the performance. Both are worse problems than high complexity.

It feels like you should review why you need the getFieldValue() method in the first place, maybe it should be a Map<String, ?>?

Upvotes: 0

Related Questions