15
\$\begingroup\$

Goals:

So the plan was simple: Provide a factory to instantiate implementations of a certain interface (ModelConverter<T>), depending on what model-class you want to convert.

The approach is relatively straightforward:

statically map model-classes to ModelConverter implementations

use that mapping to obtain the implementation class
use reflection to get a constructor and invoke it
return the instance

If any of these steps fail, fail hard, because the application relies on the ModelConverter instances.

My code currently looks as follows (note that I am tied to java 6 so please don't recommend multi-catch):

public final class ModelConverterFactory {

    private enum ClassMapping {
        DEFAULT(Object.class, AbstractConverter.class), RIGHT(Right.class,
                RightConverter.class);

        private Class<?> modelClass;
        private Class<?> converterClass;

        private ClassMapping(final Class<?> modelClass, final Class<?> clazz) {
            if (modelClass == null || clazz == null) {
                throw new BrokenCodeException(
                        "Cannot create a mapping without modelClass and converterClass");
            }

            this.modelClass = modelClass;
            this.converterClass = clazz;
        }

        public static ClassMapping fromModelClass(final Class<?> modelClass) {
            for (ClassMapping mapping : ClassMapping.values()) {
                if (mapping.modelClass.equals(modelClass)) {
                    return mapping;
                }
            }
            return DEFAULT;
        }

        public Class<?> getConverterClass() {
            return this.converterClass;
        }

        public Class<?> getModelClass() {
            return this.modelClass;
        }
    }

    public static Collection<Class<?>> supportedModelClasses() {
        Collection<Class<?>> supported = new HashSet<Class<?>>();
        for (ClassMapping mapping : ClassMapping.values()) {
            if (mapping != ClassMapping.DEFAULT) {
                supported.add(mapping.getModelClass());
            }
        }
        return supported;
    }

    public static <T> ModelConverter<T> createConverterFor(final Class<?> clazz) {
        ClassMapping mapping = ClassMapping.fromModelClass(clazz);
        Class<?> converterClass = mapping.getConverterClass();

        @SuppressWarnings("unchecked")
        Constructor<ModelConverter<T>> converterConstructor = ((Constructor<ModelConverter<T>>) converterClass
                .getDeclaredConstructors()[0]);

        ModelConverter<T> instance = null;
        try {
            instance = converterConstructor.newInstance((Object[]) null);
        } catch (IllegalArgumentException e) {
            Log.doLogError(e.toString());
            e.printStackTrace();
            throw new BrokenCodeException(
                    "Could not instantiate ModelConverter, because the Arguments didn't match",
                    e);
        } catch (InstantiationException e) {
            Log.doLogError(e.toString());
            e.printStackTrace();
            throw new BrokenCodeException(
                    "Something happened, when we tried to instantiate ModelConverter:\n",
                    e);
        } catch (IllegalAccessException e) {
            Log.doLogError(e.toString());
            e.printStackTrace();
            throw new BrokenCodeException(
                    "Could not access Constructor to for specific ModelConverter Implementation:",
                    e);
        } catch (InvocationTargetException e) {
            Log.doLogError(e.toString());
            e.printStackTrace();
            throw new BrokenCodeException(
                    "For some reason, the Constructor of ModelConverter threw an exception:",
                    e);
        }
        return instance;
    }
}

Further Notes:

  • I am aware that the current approach requires Implementations to declare only a single parameterless constructor. For the current state of the project this is used for, that is actually quite fine.
  • Again I want to mention that I am required to use java 6
  • BrokenCodeException is a simple RuntimeException with no additional info.

Concerns:

  • Unchecked casting: Do I really need it? It feels somewhat dirty to me :(
  • Again, am I overly complicating a simple task? This feels like it's the way around and below to come from behind! Note that I didn't want to use CDI, to make this approach usable for non-JEE applications.
\$\endgroup\$
6
\$\begingroup\$

Things will be greatly simplified if ClassMapping didn't map a class to a class, but a class to an instance.

public final class ModelConverterFactory {

    private enum ClassMapping {
        DEFAULT(Object.class, new AbstractConverter()), RIGHT(Right.class, new RightConverter());

        private Class<?> modelClass;
        private ModelConverter<?> converter;

        private <T> ClassMapping(final Class<T> modelClass, final ModelConverter<T> modelConverter) {
            this.modelClass = modelClass; // got rid of null checks, we control all invocations, none pass null.
            this.converter = modelConverter;
        }

        public static ClassMapping fromModelClass(final Class<?> modelClass) {
            for (ClassMapping mapping : ClassMapping.values()) {
                if (mapping.modelClass.equals(modelClass)) { 
                    return mapping;
                }
            }
            return DEFAULT;
        }

        public ModelConverter<?> getConverter() {
            return this.converter;
        }

        public Class<?> getModelClass() {
            return this.modelClass;
        }
    }

    public static Collection<Class<?>> supportedModelClasses() {
        Collection<Class<?>> supported = new HashSet<Class<?>>();
        for (ClassMapping mapping : ClassMapping.values()) {
            if (mapping != ClassMapping.DEFAULT) {
                supported.add(mapping.getModelClass());
            }
        }
        return supported;
    }

    public static <T> ModelConverter<T> getConverterFor(final Class<T> clazz) {
        return (ModelConverter<T>) ClassMapping.fromModelClass(clazz).getConverter(); // safe cast : we know all entries will match
    }
}

This gets rid of that nasty introspection bit. Of course this means ModelConverter implementations ought to be reentrant.

I would also be inclined to replace the enum with a Map. You say you use an enum to disallow runtime changing of mappings, but with proper encapsulation that can be perfectly achieved with a Map as well.

\$\endgroup\$
  • \$\begingroup\$ very nice suggestion. A small thought on the map. The class mapping needs to be static. Personally I think static Map initializations look ugly, and would prefer an enum like displayed wherever possible ;) \$\endgroup\$ – Vogel612 Jul 23 '14 at 18:57
  • 1
    \$\begingroup\$ Look into guava's ImmutableMap and matching builder, you'll love how elegant that initializes a Map. The enum is just less handy to use : linear search time and manually building the available classes. With a HashMap it's O(1) search for a key, and you get the available Classes through getKeys(). \$\endgroup\$ – bowmore Jul 23 '14 at 22:03
5
\$\begingroup\$

Quick answer:

Log.doLogError(e.toString());
e.printStackTrace();

Duplication! Make a method logAndPrintException. You could even put the whole throw new BrokenCodeException section in there... provided you are planning to only throw BrokenCodeException. Which seems dubious to me.

Additionally, any specific reason that you use \n here?

throw new BrokenCodeException(
        "Something happened, when we tried to instantiate ModelConverter:\n",
        e);

Because all the other BrokenCodeExceptions don't use \n.

I don't have any answers regarding your concerns, however.

\$\endgroup\$
  • \$\begingroup\$ no specific reason, prefer to keep Log.doLog* separate from e.printStackTrace(); because the later is bound to get removed when we get to release, actually my Logger is currently in No-Operation mode :(. Also split the BrokenCodeExceptions, because I want to provide (at least minimally) meaningful error messages. Nice finds though... \$\endgroup\$ – Vogel612 Jul 23 '14 at 12:06
  • \$\begingroup\$ Don't put the newline in the exception message, this will lead to atypical output from your exception. These messages are for developers not end users and shouldn't have built in formatting. \$\endgroup\$ – Robin Jul 23 '14 at 18:37
5
\$\begingroup\$

I don't really like your current solution as it seems overly complicated, instead of fixing it, I'll suggest my own implementation, the things I found are:

  • You are using enums without much reason behind them in my opinion, you would better simply use some of Map. The default implementation is a fair point, and it can always be added in any solution.
  • Your type safety between the model and the converter class is on runtime, and not on compile time.
  • When converting, you loop through all enum values, this is the reason a Map is better, it gives (even if marginal) better performance.
  • You use reflection, which is bad, unless you have very good reasons to do so. Now your reasons are not invalid at all, but there are better ways around it that do not use reflection, so I wouldn't use reflection.

In response to your German overengineering™, I raise you some Dutch overengineering™.

I have managed to get rid of almost all unsafe things, like creating instances via reflection etc. My implementation has one unsafe cast, but I'll explain that it's no issue as we go.

I have decided to re-implement your solution as a whole, so this will not be a review about existing code.

Your problem statement, according to me is that:

Various kinds of model classes need to be mapped to a custom view, hence a custom converter is needed that converts the model classes to the custom view.

My implementation and example code is the following:

public class AlphaModel { }

A dummy model class.


public class BetaModel { }

A dummy model class.


public interface ModelConverter<I, O> {
    O convertToView(final I input);
}

A converter that converts input to output, it's really nothing more than a Function<I, O>.


public class AlphaModelConverter implements ModelConverter<AlphaModel, String> {
    @Override
    public String convertToView(final AlphaModel alphaModel) {
        return "ALPHA";
    }
}

A converter that converts every AlphaModel instance to its string representation, dummy code.


public class BetaModelConverter implements ModelConverter<BetaModel, String> {
    @Override
    public String convertToView(final BetaModel betaModel) {
        return "BETA";
    }
}

A converter that converts every BetaModel instance to its string representation, dummy code.


public class ModelConverterBacking<O> {
    private final Map<Class<?>, ModelConverter<?, O>> modelConverters = new HashMap<Class<?>, ModelConverter<?, O>>();

    public <T> void put(final Class<T> clazz, final ModelConverter<T, O> modelConverter) {
        Objects.requireNonNull(clazz, "clazz");
        Objects.requireNonNull(modelConverter, "modelConverter");
        modelConverters.put(clazz, modelConverter);
    }

    @SuppressWarnings("unchecked")
    public <T> ModelConverter<T, O> get(final Class<T> clazz) {
        return (ModelConverter<T, O>)modelConverters.get(clazz);
    }
}

A mapping that can safely map Class<T> instances to ModelConverter<T, O>. The only methods to interact with it are put and get, the put ensures that the types are correct on compile time, it also uses Objects.requireNonNull which is from Java 7, but you should be able to convert that. The get method can safely cast back to ModelConverter<T, O>, because the put method ensures the types.


@SuppressWarnings("unchecked")
public enum ModelConverters {
    INSTANCE;

    private final ModelConverterBacking<String> modelConverterBacking = new ModelConverterBacking<String>();
    {
        modelConverterBacking.put(AlphaModel.class, new AlphaModelConverter());
        modelConverterBacking.put(BetaModel.class, new BetaModelConverter());
    }

    public static <T> String convert(final T input) {
        ModelConverter<T, String> modelConverter = INSTANCE.modelConverterBacking.get((Class<T>)input.getClass());
        if (modelConverter == null) {
            throw new IllegalStateException("No mapping found for " + input.getClass());
        }
        return modelConverter.convertToView(input);
    }
}

The method where the real deal is being done. Here you set up the mappings and convert your data. A tricky fact to observe is that in convert it has a T input, which is really of type ? input, hence getClass() returns Class<?>, but since we know that it is of type T, we can safely cast it to Class<T>.

I've verified and this code works on Java 6 minus Objects.requireNonNull.

The code should be called like:

AlphaModel alphaModel = obtainItFromSomewhere();
String alphaView = ModelConverters.convert(alphaModel);
\$\endgroup\$
  • \$\begingroup\$ I don't really like how you've gone all the way to use Objects.requireNonNull, but then didn't add a helpful message, instead just naming the argument. \$\endgroup\$ – Pimgd Jul 24 '14 at 9:39
  • \$\begingroup\$ @Pimgd I don't see the benefit of adding x may not be null over simply x, you will see it is a NullPointerException in the stacktrace either way, and then you can read what variable is null. \$\endgroup\$ – skiwi Jul 24 '14 at 9:41
  • \$\begingroup\$ You could indicate the consequences of null, sometimes programmers need a reminder. Cryptic error messages are a pet peeve of mine. Particularily with binaries. \$\endgroup\$ – Pimgd Jul 24 '14 at 9:43
4
\$\begingroup\$

Reassuming my previous comments:

  • I would not use an Enum, preferring a more light approach like the following (map-oriented):

    [...]
    public static void addConverter(Class<?> modelClass, Class<?> converterClass){
        converters.put(modelClass, converterClass);
    }
    
    public static <T> ModelConverter<T> getConverterFor(Class<?> modelClass) {
       Class converterClass = converters.get(modelClass);
    
       if(converterClass == null){
            converterClass = AbstractConverter.class;
            converters.put(modelClass, converterClass);
       }
    
       //try/catch etc
       ModelConverter<T>  instance = (ModelConverter<T>) converterClass.newInstance();
       return instance ;
    }
    [...]
    

If you want to keep read-only the runtime configuration, you can get it with good encapsulation as @bowmore suggested (i.e. addConverter private or package)

  • If the converters are set at build time and will stay readonly, why such low-level catches? The callers can be sure the converters will be good otherwise they can assume an installation error: just catch the Throwable one time and wrap it inside a BrokenCodeException. This one anyway can provide further details with its getCause(). IMHO in this way you would improve the reader experience of the code and make easier for you adding further error-handling logic (that is, change just one block).

        try {
            instance = converterConstructor.newInstance((Object[]) null);
        } catch (Throwable t) {
            err(t);
            throw new BrokenCodeException(t);
        }
    
        void err(Throwable t){
            Log.doLogError(t.toString());
            t.printStackTrace();
        }
    

The caller is anyway expecting a BrokenCodeException. If it have to inspect further details, it can do in the following way:

        try {
            //...
        } catch (BrokenCodeException t) {
            if(t.getCause() instanceof InstantiationException){
                //...
            }
        }

Though I would expect this kind of code only inside your test client. If you the API-creator are really planning of giving different semantical errors to the caller of course build and throw new custom types.

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