DB-to-Java value mapper

Question

In my company, I've inherited some Java library that I'm now writing tests to, refactoring and fixing Sonar issues.

One particular point that Sonar is complaining about is a big chaining of if/else if/else statements, that is used in a class responsible for parsing values that come from the DB to the corresponding values used in Java. The cyclomatic complexity of this method is 32, where Sonar is configured to a threshold of 15.

I would like some ideas of a better way to write this. Maybe some design-pattern that could be applied.

Since this is a pretty critical part of the application, it would be interesting not to add a considerable overhead.

private Object getValue(final Class<?> type, final int index, final ResultSet resultSet) throws SQLException {
    Object value = null;

    if (Boolean.class.isAssignableFrom(type) || boolean.class.isAssignableFrom(type)) {
        String v = resultSet.getString(index);
        if (v != null) {
            value = "Y".equalsIgnoreCase(v);
        }
    } else if (Byte.class.isAssignableFrom(type) || byte.class.isAssignableFrom(type)) {
        value = resultSet.getByte(index);
    } else if (Byte[].class.isAssignableFrom(type) || byte[].class.isAssignableFrom(type)) {
        value = resultSet.getBytes(index);
    } else if (Short.class.isAssignableFrom(type) || short.class.isAssignableFrom(type)) {
        value = resultSet.getShort(index);
    } else if (Integer.class.isAssignableFrom(type) || int.class.isAssignableFrom(type)) {
        value = resultSet.getInt(index);
    } else if (Long.class.isAssignableFrom(type) || long.class.isAssignableFrom(type)) {
        value = resultSet.getLong(index);
    } else if (Float.class.isAssignableFrom(type) || float.class.isAssignableFrom(type)) {
        value = resultSet.getFloat(index);
    } else if (Double.class.isAssignableFrom(type) || double.class.isAssignableFrom(type)) {
        value = resultSet.getDouble(index);
    } else if (BigDecimal.class.isAssignableFrom(type)) {
        value = resultSet.getBigDecimal(index);
    } else if (Timestamp.class.isAssignableFrom(type)) {
        value = resultSet.getTimestamp(index);
    } else if (Time.class.isAssignableFrom(type)) {
        value = resultSet.getTime(index);
    } else if (Date.class.isAssignableFrom(type)) {
        value = resultSet.getDate(index);
    } else if (LocalTime.class.isAssignableFrom(type)) {
        String vlrString = resultSet.getString(index);
        if (StringUtils.length(vlrString) == 6) {
            value = LocalTime.of(Integer.valueOf(vlrString.substring(0, 2)), Integer.valueOf(vlrString.substring(2, 4)), Integer.valueOf(vlrString.substring(4)));
        }
    } else if (LocalDate.class.isAssignableFrom(type)) {
        Timestamp timeStamp = resultSet.getTimestamp(index);
        value = timeStamp == null ? null : timeStamp.toLocalDateTime().toLocalDate();
    } else if (LocalDateTime.class.isAssignableFrom(type)) {
        Timestamp timeStamp = resultSet.getTimestamp(index);
        value = timeStamp == null ? null : timeStamp.toLocalDateTime();
    } else if (String.class.isAssignableFrom(type)) {
        value = resultSet.getString(index);
    } else if (type.isEnum()) {
        value = DomainUtils.get((Class<Enum>) type, resultSet.getString(index));
    } else {
        value = resultSet.getObject(index);
    }

    if (resultSet.wasNull() && !type.isPrimitive()) {
        value = null;
    }

    return value;
}

Answer 1

I think, performance wise, you won't get much better than the if-then-else construct. Since isAssignableFrom is not a direct lookup, but a rather complex task, but your runtime types are actually limited, it may be helpful to build a cache around the result. So, if you found a type match, keep that in a direct lookup cache to avoid walking the if-then.

To help with code-maintenance, I would refactor the comparisions into seperate stateless comparator classes, each will implement one function, the getValue method.

The list of comparators will be initialized at some early point in the lifecycle of your application and put in a list. The method getValue will be reduced to a for loop over the comparator-list.

Adding another type is now reduced to changing a comparator class. This makes it very obvious, what you have changed even in changelogs, since the class is mentioned in the committed-files-list.

To help maintenance even more, you could introduce either a search of your classpath for the comparator interface or introduce an annotation to mark the implementations for automatic building of the list.

This will give you an almost self-maintaining code for the given task. Whenever you need a new type, you just add it with the right interface/annotation and it will magically be seen on the next run.

This would be your main entry point.

public class ValueGetter {

    private static List<TypeConverter> converter;

    static {
        // might require synchronize
        converter = new ArrayList<>(1);
        converter.add(new FloatConverter());
        // ... add more or build using reflection or whatever
    }

    public Object getValue(final Class<?> type, final int index, final ResultSet resultSet) throws SQLException {
        Object value;

        for (TypeConverter c : converter) {
            value = c.getValue(type, resultSet, index);
            if (value != TypeConverter.NOTTYPE) {
                return value;
            }
        }

        throw new IllegalArgumentException("Cannot convert to requested type");
    }
}

This is the interface mentioned.

public interface TypeConverter {

    public static final String NOTTYPE = "not of this type";

    Object getValue(Class<?> type, ResultSet resultSet, int index) throws SQLException;
}

This is taking the float if-else

public class FloatConverter implements TypeConverter {
    @Override
    public Object getValue(Class<?> type, ResultSet resultSet, int index) throws SQLException {
        if (Float.class.isAssignableFrom(type) || float.class.isAssignableFrom(type)) {
            return resultSet.getFloat(index);
        }

        return NOTTYPE;
    }
}

Answer 2

I start digging into this code very carefully

My first idea was to extract every thing that I can from this very long method, because I don't really like it (to tell the truth I don't understand it)

I start dividing this method into logical parts and I've succeed (I think)

Result

private Object getValue(final Class<?> type, final int index, final ResultSet resultSet) throws SQLException {
  // part one - very long method for defining value type
  Object value = defineValueType(resultSet, type, index);

  // another checking
  if (resultSet.wasNull() && !type.isPrimitive()) {
    value = null;
  }

  // returning the result
  return value;
}

Firstly I want to reduce the size of the defineValueType method (all logic is hidden in this method)

Look at this part:

String vlrString = resultSet.getString(index);
if (StringUtils.length(vlrString) == 6) {
  value = LocalTime.of(Integer.valueOf(vlrString.substring(0, 2)), Integer.valueOf(vlrString.substring(2, 4)), Integer.valueOf(vlrString.substring(4)));
}

We have several problems:

1. Magic numbers, don't use magic numbers, replace it with constants
2. Very long string, nobody can't read it.

Solution for 2 problem

String vlrString = resultSet.getString(index);
if (StringUtils.length(vlrString) == 6) {
  value = LocalTime.of(
  Integer.valueOf(vlrString.substring(0, 2)), 
  Integer.valueOf(vlrString.substring(2, 4)), 
  Integer.valueOf(vlrString.substring(4))
  );
}

Solution for 1 problem

String vlrString = resultSet.getString(index);

// add some explanation what does it mean that length equals 6
int correctLength = 6;

// the same as for length
int firstInterval = 0;
int secondInterval = 2;
int thirdInterval = 4;

if (StringUtils.length(vlrString) == correctLength) {
  value = LocalTime.of(
  Integer.valueOf(vlrString.substring(firstInterval, secondInterval)), 
  Integer.valueOf(vlrString.substring(secondInterval, thirdInterval)), 
  Integer.valueOf(vlrString.substring(thirdInterval))
  );
} 

And I still don't like it Remove all this stuff into separate method

private Object calcTime(String vlrString) {
  int firstInterval = 0;
  int secondInterval = 2;
  int thirdInterval = 4;

  Object value = LocalTime.of(
    Integer.valueOf(vlrString.substring(firstInterval, secondInterval)), 
    Integer.valueOf(vlrString.substring(secondInterval, thirdInterval)), 
    Integer.valueOf(vlrString.substring(thirdInterval))
  );

  return value;
}

Now, we have:

else if (LocalTime.class.isAssignableFrom(type)) {
    String vlrString = resultSet.getString(index);

    int correctLength = 6;

    if (StringUtils.length(vlrString) == correctLength) {
        value = calcTime(vlrString)
    }
    // if length is not correct -> you just skip this part??
}

It still looks not good, add another method for it:

// add some comment to describe what for do you need it
private Object checkVlrString(ResultSet resultSet, int index) {
  String vlrString = resultSet.getString(index);

  int correctLength = 6;

  if (StringUtils.length(vlrString) == correctLength) {
      return calcTime(vlrString)
  }

  return null
  // if length is not correct -> you just skip this part??
}

Not, it's ok, result

else if (LocalTime.class.isAssignableFrom(type)) {
    value = checkVlrString(resultSet, index)
}

I haven't finished yet. In our long method we have:

if (Boolean.class.isAssignableFrom(type) || boolean.class.isAssignableFrom(type)) {
      String v = resultSet.getString(index);
      if (v != null) {
          value = "Y".equalsIgnoreCase(v);
      }
}

It's not good, extract the code inside if into separate method:

private Object checkIfEqualY(ResultSet resultSet, int index) Object {
  Object value = null;

  String v = resultSet.getString(index);
  if (v != null) {
      value = "Y".equalsIgnoreCase(v);
  }
}

But, we still have long if/else methods :(