2
\$\begingroup\$

I am trying to ensure type in the below method. How to reduce its cognitive complexity to below 15 from the current 27? It looks more readable as it is? Is there a way to break this down to make it more readable?

Shifting some of the checks to separate method will be one way but is there a better way?

private Object ensureType(Class<?> expectedClass, Object value) {
    if (value == null)
        return null;
    Class<?> valueClass = value.getClass();
    if (expectedClass.isAssignableFrom(valueClass))
        return value;

    if (expectedClass.isAssignableFrom(String.class)) {
        return value.toString();
    }
    if (expectedClass.isAssignableFrom(Date.class)) {
        if (Number.class.isAssignableFrom(valueClass)) {
            return new Date(((Number) value).intValue());
        }
        try {
            return formatter.parse(value.toString());
        } catch (ParseException e) {
            throw new ClassCastException("argument " + value + " of type " + valueClass + " cannot be converted to "
                    + expectedClass + ":" + e);
        }
    }
    if (expectedClass.isAssignableFrom(Integer.class)) {
        if (Number.class.isAssignableFrom(valueClass))
            return ((Number) value).intValue();
        return Integer.parseInt(value.toString());
    }
    if (expectedClass.isAssignableFrom(Double.class)) {
        if (Number.class.isAssignableFrom(valueClass))
            return ((Number) value).doubleValue();
        return Double.parseDouble(value.toString());
    }
    if (expectedClass.isAssignableFrom(Float.class)) {
        if (Number.class.isAssignableFrom(valueClass))
            return ((Number) value).floatValue();
        return Float.parseFloat(value.toString());
    }
    if (expectedClass.isAssignableFrom(Short.class)) {
        if (Number.class.isAssignableFrom(valueClass))
            return ((Number) value).shortValue();
        return Short.parseShort(value.toString());
    }
    if (expectedClass.isAssignableFrom(Byte.class)) {
        if (Number.class.isAssignableFrom(valueClass))
            return ((Number) value).byteValue();
        return Byte.parseByte(value.toString());
    }
    if (expectedClass.isAssignableFrom(Long.class)) {
        if (Number.class.isAssignableFrom(valueClass))
            return ((Number) value).longValue();
        return Long.parseLong(value.toString());
    }
    if (expectedClass.isAssignableFrom(Boolean.class)) {
        return Boolean.parseBoolean(value.toString());
    }

    throw new ClassCastException(
            "argument " + value + " of type " + valueClass + " cannot be converted to " + expectedClass);
}

PS: I am moving this question from Stackoverflow to CR as it was suggested in the comments.

\$\endgroup\$
  • 1
    \$\begingroup\$ The site standard is for the title to simply state the task accomplished by the code, not your concerns about it. Please see How do I ask a good question? for examples, and revise the title accordingly. \$\endgroup\$ – Martin R Oct 14 at 5:04
  • \$\begingroup\$ The title should state what your code does, the body should explain your code thoroughly and you should post your full project here for us the review it. Moreover, take your time and read How to Ask. \$\endgroup\$ – Aryan Parekh Oct 14 at 5:08
  • \$\begingroup\$ You're trying to ensure type. What does this mean? Java is already enforcing type-safety, isn't it? \$\endgroup\$ – Mast Oct 14 at 11:35
  • \$\begingroup\$ I'm sure there are other (better?) ways do this, however that depends on the bigger picture and what the use case for this is. Where do the "expected class" and the "value" come from? What kind of objects will the "value" be? How are the resulting objects used? \$\endgroup\$ – RoToRa Oct 20 at 9:26
  • \$\begingroup\$ BTW, you may also want to look into Spring. One of it's basic features is a conversion framework with some basic conversion you have implemented built-in: baeldung.com/spring-type-conversions docs.spring.io/spring-framework/docs/current/… \$\endgroup\$ – RoToRa Oct 20 at 9:32
2
\$\begingroup\$
private Object ensureType(Class<?> expectedClass, Object value) {

Your names could be slightly better, given that this function does not only ensure something, but also converts it.

private Object convert(Class<?> targetClass, Object value) {

    throw new ClassCastException(
            "argument " + value + " of type " + valueClass + " cannot be converted to " + expectedClass);

Given that the function converts, I find the ClassCastException inappropriate. An IllegalStateException or IllegalArgumentException might be better fitting.


What you're doing here is complex, and there is not really an easier way to do it either. You have n classes that you want to map to m classes, so the complexity will always be there in one way or another. The best thing you can do is make it more readable. For example with extracting the converting parts into functions:

    // ...
    
    if (targetClass.isAssignableFrom(String.class)) {
        return convertToString(value);
    }

    if (targetClass.isAssignableFrom(Date.class)) {
        return convertToDate(value);
    }
    
    if (targetClass.isAssignableFrom(Integer.class)) {
        return convertToInteger(value);
    }
    
    // ...

That is as readable as it gets, in my opinion.

If you like classes, you can always extract the conversion into a single class for each mapping, and then iterate over that:

public interface Converter {
    public Class<?> getTargetType();
    public Object convert(Object value);
}

// ...

List<Converter> converters = new ArrayList<>();
converters.add(new StringConverter());
converters.add(new DateConverter());
converters.add(new IntegerConverter());
// ...

// ...

for (Converter converter : converters) {
    if (targetClass.isAssignableFrom(converter.getTargetType())) {
        return converter.convert(value);
    }
} 

This, however, has the downside to split everything over many classes. A possible remedy to that might be to use single functions in the same class, like this:

HashMap<Class<?>, Function<Object, Object>> converters = new HashMap<>();
converters.put(String.class, this::convertToString);
converters.put(Date.class, this::convertToDate);
converters.put(Integer.class, this::convertToInteger);
// ...

// ...

for (Entry<Class<?>, Function<Object, Object>> converterEntry : converters.entrySet()) {
    if (targetClass.isAssignableFrom(converterEntry.getKey())) {
        return converterEntry.getValue().apply(value);
    }
} 

That might be one of the easier readable and maintainable variants.

There is another, completely insane option, actually, which I feel to need to point out. You can use the above setup, with a method for every conversion, and reflection to build this list dynamically. Something along these lines:

private Integer convertToInteger(Object value);
private Date convertToDate(Object value);
private String convertToString(Object value);

// Pseudo-Code follows:

for (Method method : getClass().getDeclaredMethods()) {
    if (method.getName().startsWith("convert")) {
        if (targetValue matches method return type
                && value matches first method parameter) {
            return method.invoke(this, value);
        }
    }
}

But that should be an absolute last resort, as it is not quite easy to maintain without knowing what you've got at your hands there.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.