4
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Its a good question, I have one query , this method getValue() is returning a Object, so the code where this method is being called, there what about casting back, there again are there if-else chain for casting the Object to the original class type by checking the object type with instanceof operator. \$\endgroup\$ – pbajpai21 Jun 16 '16 at 17:23
  • \$\begingroup\$ Sorry, I didn't understand. Is this a question or an statement? \$\endgroup\$ – Rafael Eyng Jun 16 '16 at 19:26
  • \$\begingroup\$ Sorry for my bad english....its a question ...I want to know that the places where this method is used, how there It is handled because it is returning object so the calling method has to again typecast it from object to some other class? \$\endgroup\$ – pbajpai21 Jun 17 '16 at 1:49
  • \$\begingroup\$ As a code refactor solution I myself would go with the answer here. If that is too much and you just want sonar to stop complaining then //NOSONAR goes a long way ;) \$\endgroup\$ – user109225 Jun 17 '16 at 16:30
  • \$\begingroup\$ @pbajpai21The object value is actually used to set the value of a Field in an entity. The Field value is set using the field name, that is not present in this method. \$\endgroup\$ – Rafael Eyng Jun 17 '16 at 17:09
3
\$\begingroup\$

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;
    }
}
\$\endgroup\$
1
\$\begingroup\$

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 :(

\$\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.