0

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?

Fenero
  • 11
  • You could use a single return. But would this change really make things more clear/readable/maintainable? Sometimes, those flags can just be ignored. – jhamon Feb 11 '20 at 10:27
  • 1
    This is something that should not be done. But if you really have to, this would be *the* usecase for reflection. – Nicktar Feb 11 '20 at 10:34
  • I can use a single return but this would not impact the cyclomatic complexity score. Regarding reflection, if there is no other way I will resort to reflection but I would rather try a different path, if there is a different path. – Fenero Feb 11 '20 at 10:39
  • doesn't each `return` increment the cyclomatic complexity? – jhamon Feb 11 '20 at 10:43
  • You could store the actual field values in a `Map`, then implement the getters by `public String getId() { return fields.get("id"); }` for example. – kaya3 Feb 11 '20 at 11:38

4 Answers4

2

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)

Arnaud Claudel
  • 3,000
  • 19
  • 25
0

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, ?>?

Karol Dowbecki
  • 43,645
  • 9
  • 78
  • 111
0

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.

Filippo Possenti
  • 1,300
  • 8
  • 18
0

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.

daniu
  • 14,137
  • 4
  • 32
  • 53