Code Review Asked by Sourabh on October 28, 2020
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.
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.
Correct answer by Bobby on October 28, 2020
Get help from others!
Recent Questions
Recent Answers
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP