Mapping from multiple sources to destination using JMapper

Question

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<D> {

    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<S1, S2> implements ObjectMapper<DestinationClazz> {


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

    private Class<S1> source1;
    private Class<S2> source2;

    private final JMapper<DestinationClazz, S1> mapper1;
    private final JMapper<DestinationClazz, S2> mapper2;

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

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

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

    public static <S1, S2> ObjectMapper<DestinationClazz> getJMapperInstance(Class<DestinationClazz> destination, Class<S1> source1, Class<S2> source2) {
        if(INSTANCE == null) {
            INSTANCE = new JMapperImpl<S1, S2>(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) {
                Class<? extends Object> sourceClass = source.getClass();
                if(sourceClass == source1) {
                    destination = mapper1.getDestination(destination, (S1)source);
                }
                else if(sourceClass == source2) {
                    destination = mapper2.getDestination(destination, (S2)source);
                }
            }
        } catch (InstantiationException e) {
            throw new CustomException("Couldn't instantiate DestinationClazz class:"+e.getMessage());
        } catch (IllegalAccessException e) {
            throw new CustomException("Couldn't access DestinationClazz class:"+e.getMessage());
        } catch (Exception e) {
            throw new CustomException("Exception while mapping source objects to DestinationClazz object:"+e.getMessage());
        }
        return destination;
    }
}

Answer 1

All you need is RelationalJMapper. You must map Destination class towards other classes, in your case towards s1 and s2.

For example, suppose that your Destination class is the following:

class Destination {
  String name;
  String surname;
}

and S1, S2 classes:

class S1{             class S2{
  String name;           String name;
  String surname;        String surname;
}                     }

You only need to configure the Destination class towards S1 and S2:

@JGlobalMap(classes={S1.class, S2.class})
class Destination {
  String name;
  String surname;
}

And put the Destination class to RelationalJMapper as follows:

RelationalJMapper<Destination> mapper = new RelationalJMapper<>(Destination.class);

with the function manyToOne you can obtain an instance of Destination from any source defined in the configuration. In your case:

Destination destination = mapper.manyToOne(new S1("S1name","S1surname"));

assertEqual(destination.getName(), "S1name");
assertEqual(destination.getSurname(), "S1surname");

destination = mapper.manyToOne(new S2("S2name","S2surname"));

assertEqual(destination.getName(), "S2ame");
assertEqual(destination.getSurname(), "S2surname");

For more information visit the Explicit relations wiki page.

Answer 2

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

1. need to transform class A to class B.
2. also need to transform class C to class B.
3. JMapper can help.
4. there are other classes that need transformations as well
5. we should make a generic factory for it.
6. 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 <T>), 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<D> {

    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<S1, S2> implements ObjectMapper<DestinationClazz> {

but then your constructor is:

private JMapperImpl(Class<DestinationClazz> destination, Class<S1> source1, Class<S2> 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<S1> source1, Class<S2> 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<S1, S2, D> implements ObjectMapper<D> {
    private JMapperImpl(Class<D> destination, Class<S1> source1, Class<S2> 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 <S1, S2> ObjectMapper<DestinationClazz> getJMapperInstance(Class<DestinationClazz> destination, Class<S1> source1, Class<S2> source2) {
    if(INSTANCE == null) {
        INSTANCE = new JMapperImpl<S1, S2>(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 <S1, S2> here are not the same ones as those on the class (public class JMapperImpl<S1, S2> 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, Long>(Double.class, Long.class), or new JMapperImpl<String, StringBuilder>(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....

You should:

• add the D Destination generic type to your class definition
• completely remove the static INSTANCE and factory method
• make the constructor public.... these are light-weight classes, and you can easily have multiple instances.
• change the mapping method name to something more meaningful.
• change the mapping argument to a single source parameter which is the only thing to map.
• throw an exception if the source does not match one of the anticipated sources.