ConverterService: type conversion multitool

Question

I got sick and tired of type-related errors and decided to write a type conversion service

It's not particularly pretty what with those casts and suppressions, but it seems to do what it was designed for

A few things to note:

1. I didn't write javadocs: those are to be written in a non-English language. The most important thing to know is S means "source type", T means "target type"
2. In case you're wondering, I had to introduce the S generic type parameter in Converter solely for this to work:

    @Override
    @SuppressWarnings("unchecked")
    public <S> Optional<T> convert(S value) {
        Class<?> valueType = value.getClass();
        // I can't write <? super valueType>
        Function<? super S, Optional<T>> mapping = (Function<? super S, Optional<T>>) getMappingForType(valueType);
        // if it was an unbounded wildcard, this line wouldn't compile
        Optional<T> optionalValue = mapping.apply(value);
        return optionalValue;
    }

3. I realize I don't account for overflows and NumberFormatExeptions. If truth be told, I couldn't figure out a good way to handle that

(should ConverterService catch? should it be the responsibility of individual Converters? how do I make it not ugly? try-catches are ugly as is, let alone in lambdas)

Let it throw, for the time being (or forever)

How can I improve it?

I was hoping to make it cleaner and more robust.

Converter:

package solution.common.converter;

import java.util.Optional;

public interface Converter<T> {
    <S> Optional<T> convert(S value);
}

Converter implementation:

package solution.common.converter;

import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.function.Function;

public class BasicConverter<T> implements Converter<T> {
    private final Map<Class<?>, Function<?, Optional<T>>> mappings = new HashMap<>();

    public <S> void addMapping(Class<S> sourceType, Function<S, Optional<T>> mappingFunction) {
        mappings.put(sourceType, mappingFunction);
    }

    @Override
    @SuppressWarnings("unchecked")
    public <S> Optional<T> convert(S value) {
        Class<?> valueType = value.getClass();
        Function<? super S, Optional<T>> mapping = (Function<? super S, Optional<T>>) getMappingForType(valueType);
        Optional<T> optionalValue = mapping.apply(value);
        return optionalValue;
    }

    private Function<?, Optional<T>> getMappingForType(Class<?> type) {
        Function<?, Optional<T>> mapping = mappings.containsKey(type) ?
                mappings.get(type) :
                findSuperTypeMapping(type);
        return mapping;
    }

    @SuppressWarnings("unchecked")
    private <S> Function<? super S, Optional<T>> findSuperTypeMapping(Class<S> valueType) {
        for (Class<?> aClass : mappings.keySet()) {
            if (aClass.isAssignableFrom(valueType))
                return (Function<? super S, Optional<T>>) mappings.get(aClass);
        }
        return noopMapping();
    }

    private <S> Function<S, Optional<T>> noopMapping() {
        return input -> Optional.empty();
    }
}

ConverterService:

package solution.common.converter;

import java.util.Optional;

public interface ConverterService {
    <T> Optional<T> convert(Object value, Class<T> targetType);
}

ConverterService implementation:

package solution.common.converter;

import java.util.HashMap;
import java.util.Map;
import java.util.Optional;

public class BasicConverterService implements ConverterService {
    private final Map<Class<?>, Converter<?>> converterMap = new HashMap<>();

    public <T> void addConverter(Class<T> targetType, Converter<T> converter) {
        converterMap.put(targetType, converter);
    }

    @Override
    @SuppressWarnings("unchecked")
    public <T> Optional<T> convert(Object value, Class<T> targetType) {
        if (targetType.isAssignableFrom(value.getClass()))
            return (Optional<T>) Optional.of(value);
        Converter<?> converter = (converterMap.containsKey(targetType)) ?
                converterMap.get(targetType) :
                findSubtypeConverter(targetType);
        return (Optional<T>) converter.convert(value);
    }

    private Converter<?> findSubtypeConverter(Class<?> targetType) {
        for (Class<?> aClass : converterMap.keySet()) {
            if (targetType.isAssignableFrom(aClass))
                return converterMap.get(aClass);
        }
        return noopConverter();
    }

    private <T> Converter<T> noopConverter() {
        return new Converter<T>() {
            @Override
            public <S> Optional<T> convert(S value) {
                return Optional.empty();
            }
        };
    }
}

Tests:

package solution.common.converter;

import org.junit.jupiter.api.Test;

import java.util.Optional;

import static org.junit.jupiter.api.Assertions.*;

class BasicConverterServiceTest {
    @Test
    void convert_ifNoConvertersAdded_andTargetTypeDoesNotMatchSourceType_returnsEmptyOptional() {
        BasicConverterService converterService = new BasicConverterService();
        Optional<Integer> valueOptional = converterService.convert("42", Integer.class);
        assertFalse(valueOptional.isPresent());
    }

    @Test
    void convert_ifNoConverterAdded_butTargetTypeMatchesSourceType_returnsOptionalOfInputValue() {
        BasicConverterService converterService = new BasicConverterService();
        Optional<String> valueOptional = converterService.convert("42", String.class);
        assertTrue(valueOptional.isPresent());
        assertEquals("42", valueOptional.get());
    }

    @Test
    void convert_ifConverterMatchingTypeAdded_returnsOptionalOfThatConvertersResult() {
        BasicConverterService converterService = new BasicConverterService();
        converterService.addConverter(Integer.class, new Converter<Integer>() {
            @Override
            public <S> Optional<Integer> convert(S value) {
                return Optional.of(Integer.parseInt((String) value));
            }
        });
        Optional<Integer> valueOptional = converterService.convert("42", Integer.class);
        assertTrue(valueOptional.isPresent());
        assertEquals(42, valueOptional.get());
    }

    @Test
    void convert_ifConverterMatchingSubtypeAdded_returnsOptionalOfThatConvertersResult() {
        BasicConverterService converterService = new BasicConverterService();
        converterService.addConverter(Integer.class, new Converter<Integer>() {
            @Override
            public <S> Optional<Integer> convert(S value) {
                return Optional.of(Integer.parseInt((String) value));
            }
        });
        Optional<Number> valueOptional = converterService.convert("42", Number.class);
        assertTrue(valueOptional.isPresent());
        assertEquals(42, valueOptional.get());
    }
}

Here's a factory that returns a more fully-featured ConverterService. It seems to cover most, if not all, of our project's needs. It contains one line that doesn't compile, the one that involves DateUtils, but I decided to omit that utility for the purpose of this code review. Basically, it checks if the input object is Date and casts if so; if it's a string, it tries a number of date formats to parse it, one by one

package solution.common.di;

import solution.common.converter.BasicConverter;
import solution.common.converter.BasicConverterService;
import solution.common.converter.Converter;
import solution.common.converter.ConverterService;
import solution.common.date.DateUtils;

import java.math.BigDecimal;
import java.math.BigInteger;
import java.util.Date;
import java.util.Optional;

public class Converters {
    public static ConverterService converterService() {
        BasicConverterService converterService = new BasicConverterService();
        converterService.addConverter(Date.class, toDateConverter());
        converterService.addConverter(Byte.class, toByteConverter());
        converterService.addConverter(Short.class, toShortConverter());
        converterService.addConverter(Integer.class, toIntegerConverter());
        converterService.addConverter(BigInteger.class, toBigIntegerConverter());
        converterService.addConverter(Long.class, toLongConverter());
        converterService.addConverter(Float.class, toFloatConverter());
        converterService.addConverter(Double.class, toDoubleConverter());
        converterService.addConverter(BigDecimal.class, toBigDecimalConverter());
        converterService.addConverter(String.class, toStringConverter());
        return converterService;
    }

    private static Converter<Date> toDateConverter() {
        BasicConverter<Date> converter = new BasicConverter<>();
        converter.addMapping(Object.class, DateUtils::asDate);
        return converter;
    }

    private static Converter<Byte> toByteConverter() {
        BasicConverter<Byte> converter = new BasicConverter<>();
        converter.addMapping(String.class, input -> Optional.of(input)
                .map(Byte::parseByte));
        converter.addMapping(Number.class, input -> Optional.of(input).map(Number::byteValue));
        return converter;
    }

    private static Converter<Short> toShortConverter() {
        BasicConverter<Short> converter = new BasicConverter<>();
        converter.addMapping(String.class, input -> Optional.of(input)
                .map(Short::parseShort));
        converter.addMapping(Number.class, input -> Optional.of(input).map(Number::shortValue));
        return converter;
    }

    private static Converter<Integer> toIntegerConverter() {
        BasicConverter<Integer> converter = new BasicConverter<>();
        converter.addMapping(String.class, input -> Optional.of(input)
                .map(Integer::parseInt));
        converter.addMapping(Number.class, input -> Optional.of(input).map(Number::intValue));
        return converter;
    }

    private static Converter<Long> toLongConverter() {
        BasicConverter<Long> converter = new BasicConverter<>();
        converter.addMapping(String.class, input -> Optional.of(input)
                .map(Long::parseLong));
        converter.addMapping(Number.class, input -> Optional.of(input).map(Number::longValue));
        return converter;
    }

    private static Converter<BigInteger> toBigIntegerConverter() {
        BasicConverter<BigInteger> converter = new BasicConverter<>();
        converter.addMapping(String.class, input -> Optional.of(input)
                .map(BigInteger::new));
        converter.addMapping(Number.class, input -> Optional.of(input)
                .map(Number::longValue)
                .map(BigInteger::valueOf));
        return converter;
    }

    private static Converter<Float> toFloatConverter() {
        BasicConverter<Float> converter = new BasicConverter<>();
        converter.addMapping(String.class, input -> Optional.of(input)
                .map(Float::parseFloat));
        converter.addMapping(Number.class, input -> Optional.of(input).map(Number::floatValue));
        return converter;
    }

    private static Converter<Double> toDoubleConverter() {
        BasicConverter<Double> converter = new BasicConverter<>();
        converter.addMapping(String.class, input -> Optional.of(input)
                .map(Double::parseDouble));
        converter.addMapping(Number.class, input -> Optional.of(input).map(Number::doubleValue));
        return converter;
    }

    private static Converter<BigDecimal> toBigDecimalConverter() {
        BasicConverter<BigDecimal> converter = new BasicConverter<>();
        converter.addMapping(String.class, input -> Optional.of(input)
                .map(BigDecimal::new));
        converter.addMapping(Number.class, input -> Optional.of(input)
                .map(Number::doubleValue)
                .map(BigDecimal::new));
        return converter;
    }

    private static Converter<String> toStringConverter() {
        BasicConverter<String> converter = new BasicConverter<>();
        converter.addMapping(Object.class, input -> Optional.of(input).map(Object::toString));
        return converter;
    }
}

Answer 1

Potential Pitfalls

The practicality of this idea is questionable. You can make it work, but probably "the juice is not worth the squeeze".

Its application is very limited. This generic global converter will hardly be helpful with anything but basic types (as in your example), which can be dealt with without it. When it comes to domain types, purpose-built methods and abstractions will do better (will be more expressive and intuitive for the code reader) than faceless ConverterService.convert().

Let me outline the issues.

Same Type - Different Semantic

Presented logic for handling type hierarchies has a problem: it gives no certainty on which conversion will be performed.

Consider the following example:

var converter = new BasicConverterService();
        
var toInteger = new BasicConverter<Integer>();
converter.addConverter(Integer.class, toInteger);
        
toInteger.addMapping(
    Object.class,
    o -> Optional.of(o.hashCode())
);
        
toInteger.addMapping(
    ChronoLocalDate.class,
    date -> Optional.of(date.lengthOfYear())
);
        
Optional<Integer> days = converter.convert(LocalDate.now(), Integer.class);
System.out.println(days);

You might expect the result to be Optional[366] (since 2024 is a leap year it has 366 days), but on my machine this code gives the output:

Optional[4145808]

Because your implementation is happy with the first Converter which can handle the input type.

Note: you might think that in the case of absence of the exact match of the given input type, selecting all potentially suitable converters and choosing the one with input type which is lowest in hierarchy will solve the problem. But an ambiguity still might occur due to the possibility to inherit from multiple unrelated interfaces.

Exceptions

First, let's address your question.

should ConverterService catch?

should it be the responsibility of individual Converters?

how do I make it not ugly? try-catches are ugly as is, let alone in lambdas

Based on your general strategy of lenient conversion, I assume that your intention is to return an empty Optional from a catch block.

If so, catching on the ConverterService level will be error-prone. Because you're loosing a context of what types of Exceptions ConverterService.convert() should handle. Catching super-types (Exceptions, Throwable) isn't a good idea because it'll also hide any RuntimeException caused by a bug.

Handling Exception inside a Converter will be a better option. And it doesn't mean that you have to right multiline lambda expressions with try-catch, which is absolutely unreadable. Instead, you should define a method (for each converter) attempting to catch only the exact Exception type that this specific conversion might cause (e.g.: NumberFormatException, DateTimeParseException). And then use this method in a method reference or call it from a lambda expression.

And here's an issue.

If you implement behavior as described above, then in cases when conversion triggers an Exception, the result is an empty Optional. And an empty Optional will be produced if the requested conversion doesn't even exist. There would be no way to distinguish between the two.

No support for Multi-level Conversions

With your current implementation, defining conversions A -> B and B -> C will not automatically give you conversion A -> C for free. If you need it, then it should be registered separately.

Design

Converter is basically an implementation detail. Exposing it only complicates reasoning about the code configuring an instance of ConverterService.

It would be easier to follow the code if configuration looked like this:

var converter = new BasicConverterService()
    .addMapping(from, to, mapperFunction)
    .addMapping(from, to, mapperFunction)
    .addMapping(from, to, mapperFunction);

Also, you didn't provide any reason why for defining ConverterService interface. What does this additional abstraction give you in this case? Do you plan to have more than one implementation?

^{Introducing an interface for each and every class isn't a good design decision, especially when you're just exploring the problem at hand. Because, firstly, you might not need this class (let alone the interface), secondly it better to postpone defining an interface until you need it, that ensures that you fully understand the contract (and thus not trying to cram another implementation into a poorly designed contract) and don't spawn unnecessary abstractions.}

And, by the way, regarding multi-level conversions, in case if you're interested in supporting them, you might consider implementing your ConverterService as a Graph. So that a conversion from A to B can be described the shortest path between the nodes representing these types.

Minor suggestions

• Legacy Types

Don't use java.util.Date, as well as other legacy date-time types such as java.sql.Date, GregorianCalendar, etc. With the introduction of Java 8 were superseded by classes from the java.time package.

• "Inline variable" Refactoring

There are several places in your code where you're introducing a variable just in order to return it.

Examples:

Optional<T> optionalValue = mapping.apply(value);
return optionalValue;

Function<?, Optional<T>> mapping = mappings.containsKey(type) ?
    mappings.get(type) :
    findSuperTypeMapping(type);
return mapping;

Such local variables are redundant, instead you can inline them by moving the corresponding logic to the return statement. When if it feels like return has become cognitively heavy, and you want to "explain" what it does, that's a clear sign that you need to introduce a method, not a local variable.

• Tests

Consider using a fluent assertion library such as AssertJ, or Hamcrest.

^{Note: in case if you're using Spring Boot, then AssertJ is probably already on your class path (it comes with the Spring Boot Test starter dependency).}

These tools are better that plain JUnit assertions because:

1. they will allow you to write more expressive tests and easier to read test;

2. you'll get a more descriptive error messages in case of test failures.

Example, here you have a single logical assertion (that optional contains a certain value) described with 2 JUnit assertions:

assertTrue(valueOptional.isPresent());
assertEquals("42", valueOptional.get()); 

In case if the first assertion fails you'll get:

Expected :true
Actual   :false

With AssertJ the same assertion will be written as:

assertThat(optional).contains(42);

Which better communicates the intent. And gives a more helpful error message, in case if optional is empty you'll get:

Expected :Optional[42]
Actual   :Optional.empty