Mapping from multiple sources to destination using JMapper

I am using JMapper to map from multiple sources to a destination class. Could you review it and let me know if the code looks OK to you? Please suggest any changes that I can make to improve it.

Interface to Map sources to destination object:

public interface ObjectMapper {

    public D getDestination(Object... sources);

}

Class that implements this interface to perform the desired mapping using JMapper:

```
import org.apache.commons.lang.WordUtils;
import com.googlecode.jmapper.JMapper;

public class JMapperImpl implements ObjectMapper {

private static ObjectMapper INSTANCE;
private static final String CONVERSION_END = "Conversion.xml";

private Class source1;
private Class source2;

private final JMapper mapper1;
private final JMapper mapper2;

private JMapperImpl(Class destination, Class source1, Class source2) {
final String mappingFile = getMappingFile(destination);
mapper1 = new JMapper(destination, source1, mappingFile);
mapper2 = new JMapper(destination, source2, mappingFile);

this.source1 = source1;
this.source2 = source2;
}

/**
* @param destination
* @return uncapitalizes the first letter of destination class name.
*/
private String getMappingFile(Class destination) {
return WordUtils.uncapitalize(destination.getSimpleName())+CONVERSION_END;
}

public static ObjectMapper getJMapperInstance(Class destination, Class source1, Class source2) {
if(INSTANCE == null) {
INSTANCE = new JMapperImpl(destination, source1, source2);
}
return INSTANCE;
}

/**
* Returns new instance of destination object with its properties mapped from source objects
*/
@Override
public DestinationClazz getDestination(Object... sources) {
DestinationClazz destination;
try {
destination = DestinationClazz.class.newInstance();
for(Object source : sources

I can understand the usefulness of a class/tool like this. The basic thinking is:

• need to transform class A to class B.

• also need to transform class C to class B.

• JMapper can help.

• there are other classes that need transformations as well

• we should make a generic factory for it.

• we can also make the concept referencable as an interface, so things other than JMapper can do the mapping if necessary.

This is what I assume your thought process was, so I will review based on that assumption...

Interface

First up, the interface idea is a good one.... but the varargs Object parameter is a problem. You should only have a single argument. Also, since you want to keep the input argument anonymous (Object instead of `), you should have some exception handling for those times when invalid data is supplied....

The reason for suggesting you have only a single input parameter is because the logic for choosing which instance is used for the transform is not easy to specify when there's multiple candidate input values. You should let the calling code make the decision as to which instance to convert from (note, this is related to a second problem I will address again in a moment)

I would also recommend renaming the interface method to something more verb-like, such as mapToDestination(...) instead of getDestination(...)

Bottom line, I would recommend an interface like:

public interface ObjectMapper {

    public D mapToDestination(Object source) throws IllegalArgumentException;

}

Right, now for the actual implementation.... using JMapper. Unfortunately there are some real concerns I have in here.

DestinationClazz

Do you have an actual class called DestinationClazz? I imagine not. I think this is supposed to be a generic type, but then you are not treating it like a normal type. For example, your class definition is:

public class JMapperImpl implements ObjectMapper {

but then your constructor is:

private JMapperImpl(Class destination, Class source1, Class source2) {
    final String mappingFile = getMappingFile(destination);

This makes no sense because since you have declared the ObjectMapper as an exactly typed value, you may as well have a constructor that knows it is production DestinationClazz instances... no need to pass in the class:

private JMapperImpl(Class source1, Class source2) {
    final String mappingFile = getMappingFile(DestinationClazz.class);

So, because of this inconsistency, I imagine that you really want the DestinationClazz to be a generic type.... and you should specify it as part of the class signature:

public class JMapperImpl implements ObjectMapper {
    private JMapperImpl(Class destination, Class source1, Class source2) {
        final String mappingFile = getMappingFile(destination);
        ....

    @Override
    public D getDestination(Object source) {
        ....

Static Singleton

This code is broken for a number of reasons:

public static  ObjectMapper getJMapperInstance(Class destination, Class source1, Class source2) {
    if(INSTANCE == null) {
        INSTANCE = new JMapperImpl(destination, source1, source2);  
    }
    return INSTANCE;
}

Here you have a lot of pretty complicated problems going on. I will try to address them in a sensible way....

• synchronization - you are vulnerable to having multiple instances of the same JMapper since multiple threads could be calling this at any one time, and ending up with different instances.

• misleading generics - This is a generic method and not a method with generic arguments. What I am saying is that the here are not the same ones as those on the class (public class JMapperImpl implements ....). Your code implies that this static method takes the same generic arguments as the INSTANCE instance, but this static method is not related to any particular class instance anywhere.






• Generic Erasure - when a class with Generics is compiled, the compiler uses the generics to do compile-time validation of the code. Once the compile is validated though, the generic information is completely erased. The effect of this is that there is actually only one class called JMapperImpl. You can create many instances of this class each with different generic types: new JMapperImpl(Double.class, Long.class), or new JMapperImpl(String.class, StringBuilder.class), etc.... but only the first one you create using the static method will be the INSTANCE, and the others will never exist.... ;-)




Where to, Sir?

In your actual mapping method you loop through the input source variables, and, if they match one of your defined source classes, you create a destination instance using the JMapper.

Unfortunately, you keep going until you run out of input source values.... If the user supplies 100 source values, you map them all! and only return the last one....

Which one is the right destination` (what's the destination? ... where to, Sir)?

Conclusion...

StackExchange Code Review Q#39749, answer score: 6