Below is a generic code for parsing strings (read from file for example) and converting them to .java types, such as primitives (and their wrappers), java.io.File, Enum etc. There is also possible to register custom made TypeParser's, for non-standard java types (Or provide a static factory method named: valueOf(String) in your class.).

  1. Is the usage of generics done in a correct way?
  2. Is the registration mechanism of custom made TypeParser's made with best practices way?
  3. Is the usage of the Builder pattern done the right way?
  4. Any other comments?

package com.github.drapostolos.typeparser;

public interface TypeParser<T> {
    T parse(String value);
}

package com.github.drapostolos.typeparser;
import java.io.File;
import java.util.ArrayList;
import java.util.List;

class DefaultTypeParsers {

    static List<TypeParser<?>> list() {
        List<TypeParser<?>> result = new ArrayList<TypeParser<?>>();
        for(Class<?> c : DefaultTypeParsers.class.getDeclaredClasses()){
            if(TypeParser.class.isAssignableFrom(c)){
                TypeParser<?> instance;
                try {
                    instance = (TypeParser<?>) c.newInstance();
                    result.add(instance);
                } catch (Exception e) {
                    throw new RuntimeException(e);
                } 
            }
        }
        return result;
    }
    static class BooleanTypeParser implements TypeParser<Boolean> {
        @Override
        public Boolean parse(final String value0) {
            String value = value0.trim().toLowerCase();
            if(value.equals("true") || value.equals("1")){
                return Boolean.TRUE;
            } else if(value.equals("false") || value.equals("0")){
                return Boolean.FALSE;
            }
            String message = "\"%s\" is not parsable to a Boolean.";
            throw new IllegalArgumentException(String.format(message, value0));
        }
    }
    static class CharacterTypeParser implements TypeParser<Character>{
        @Override
        public Character parse(String value) {
            if(value.length() == 1){
                return Character.valueOf(value.charAt(0));
            }
            String message = "\"%s\" must only contain a single character.";
            throw new IllegalArgumentException(String.format(message, value));
        }
    }
    static class ByteTypeParser implements TypeParser<Byte> {
        @Override
        public Byte parse(String value) {
            return Byte.valueOf(value.trim());
        }
    }
    static class IntegerTypeParser implements TypeParser<Integer>{
        @Override
        public Integer parse(String value) {
            return Integer.valueOf(value.trim());
        }
    }
    static class LongTypeParser implements TypeParser<Long>{
        @Override
        public Long parse(String value) {
            return Long.valueOf(value.trim());
        }
    }
    static class ShortTypeParser implements TypeParser<Short>{
        @Override
        public Short parse(String value) {
            return Short.valueOf(value.trim());
        }
    }
    static class FloatTypeParser implements TypeParser<Float>{
        @Override
        public Float parse(String value) {
            return Float.valueOf(value);
        }
    }
    static class DoubleTypeParser implements TypeParser<Double>{
        @Override
        public Double parse(String value) {
            return Double.valueOf(value);
        }
    }
    static class FileTypeParser implements TypeParser<File>{
        @Override
        public File parse(String value) {
            return new File(value.trim());
        }
    }
    static class StringTypeParser implements TypeParser<String>{
        @Override
        public String parse(String value) {
            return value;
        }
    }
}

package com.github.drapostolos.typeparser;

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

public final class StringToTypeParserBuilder {
    private final static Map<Class<?>, TypeParser<?>> defaultTypeParsers = new HashMap<Class<?>, TypeParser<?>>();
    private final static Map<Class<?>, Class<?>> wrapperToPrimitiveMapper = new HashMap<Class<?>, Class<?>>();

    private Map<Class<?>, TypeParser<?>> typeParsers;

    static {
        wrapperToPrimitiveMapper.put(Boolean.class, boolean.class);
        wrapperToPrimitiveMapper.put(Byte.class, byte.class);
        wrapperToPrimitiveMapper.put(Short.class, short.class);
        wrapperToPrimitiveMapper.put(Character.class, char.class);
        wrapperToPrimitiveMapper.put(Integer.class, int.class);
        wrapperToPrimitiveMapper.put(Long.class, long.class);
        wrapperToPrimitiveMapper.put(Float.class, float.class);
        wrapperToPrimitiveMapper.put(Double.class, double.class);

        for(TypeParser<?> typeParser : DefaultTypeParsers.list()){
            Class<?> type = Util.extractTypeParameter(typeParser);
            defaultTypeParsers.put(type, typeParser);
            if(wrapperToPrimitiveMapper.containsKey(type)){
                // add primitive version, example int.class, boolean.class etc.
                defaultTypeParsers.put(wrapperToPrimitiveMapper.get(type), typeParser);
            }
        }
    }

    StringToTypeParserBuilder() {
        // Initialize with the default typeParsers
        typeParsers = new HashMap<Class<?>, TypeParser<?>>(defaultTypeParsers);
    }

    public StringToTypeParserBuilder unregisterTypeParser(Class<?> type){
        if(type == null) {
            throw new NullPointerException(Util.nullArgumentErrorMsg("type"));
        }
        typeParsers.remove(type);
        return this;
    }
    public StringToTypeParserBuilder registerTypeParser(TypeParser<?> typeParser){
        if(typeParser == null) {
            throw new NullPointerException(Util.nullArgumentErrorMsg("typeParser"));
        }
        typeParsers.put(Util.extractTypeParameter(typeParser), typeParser);
        return this;
    }
    public <T> StringToTypeParserBuilder registerTypeParser(Class<T> type, TypeParser<? extends T> typeParser){
        if(typeParser == null) {
            throw new NullPointerException(Util.nullArgumentErrorMsg("typeParser"));
        }
        if(type == null) {
            throw new NullPointerException(Util.nullArgumentErrorMsg("type"));
        }
        typeParsers.put(type, typeParser);
        return this;
    }

    public StringToTypeParser build(){
        return new StringToTypeParser(new HashMap<Class<?>, TypeParser<?>>(typeParsers));
    }
}

package com.github.drapostolos.typeparser;

import java.io.File;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Map;

public final class StringToTypeParser {
    private static final StringToTypeParser defaultTypeParser = newBuilder().build();
    private final Map<Class<?>, TypeParser<?>> typeParsers;

    public static StringToTypeParserBuilder newBuilder() {
        return new StringToTypeParserBuilder();
    }

    public static <T> T parse(String value, Class<T> type){
        return defaultTypeParser.parseType(value, type);
    }

    StringToTypeParser(HashMap<Class<?>, TypeParser<?>> typeParsers) {
        this.typeParsers = typeParsers;
    }
    public <T> T parseType(String value, Class<T> type) {
        if (value == null) {
            throw new NullPointerException(Util.nullArgumentErrorMsg("value"));
        }
        if (type == null) {
            throw new NullPointerException(Util.nullArgumentErrorMsg("type"));
        }

        // convert "null" string to null type.
        if (value.trim().equalsIgnoreCase("null")) {
            if (type.isPrimitive()) {
                String message = "'%s' primitive can not be set to null.";
                throw new IllegalArgumentException(String.format(message,                         type.getName()));
            }
            return null; 
        }

        Object result = null;
        if (typeParsers.containsKey(type)) {
            result = callTypeParser(value, type);
        } else if ((result = callFactoryMethodIfExisting("valueOf", value, type)) != null) {
            //
        } else if ((result = callFactoryMethodIfExisting("of", value, type)) != null) {
            //
        } else {
            String message = "There is no registered 'TypeParser' for that type, or that "
                    + "type does not contain one of the following static factory methods: "
                    + "'%s.valueOf(String)', or '%s.of(String)'.";
            message = String.format(message, type.getSimpleName(), type.getSimpleName());
            message = canNotParseErrorMsg(value, type, message);
            throw new IllegalArgumentException(message);
        }

        /*
         * This cast is correct, since all above checks ensures we're casting to
         * the right type.
         */
        @SuppressWarnings("unchecked")
        T temp = (T) result;
        return temp;
    }

    private Object callTypeParser(String value, Class<?> type) {
        try {
            return typeParsers.get(type).parse(value);
        } catch (NumberFormatException e) {
            String message = canNotParseErrorMsg(value, type, numberFormatErrorMsg(e));
            throw new IllegalArgumentException(message, e);
        } catch (RuntimeException e) {
            throw new IllegalArgumentException(canNotParseErrorMsg(value, type, e.getMessage()), e);
        }
    }

    private static String numberFormatErrorMsg(NumberFormatException e) {
        return String.format("Number format exception %s.", e.getMessage());
    }

    private static String canNotParseErrorMsg(String value, Class<?> type, String message) {
        return String.format("Can not parse \"%s\" to type '%s' due to: %s", value, type.getName(), message);
    }

    private static Object callFactoryMethodIfExisting(String methodName, String value, Class<?> type) {
        Method m;
        try {
            m = type.getDeclaredMethod(methodName, String.class);
            if (!Modifier.isStatic(m.getModifiers())) {
                // Static factory method does not exists, return null
                return null;
            }
        } catch (Exception e) {
            // Static factory method does not exists, return null
            return null;
        }

        try {
            if(type.isEnum()){
                return m.invoke(null, value.trim());
            } else {
                return m.invoke(null, value);
            }
        } catch (InvocationTargetException e) {
            throw new IllegalArgumentException(makeErrorMsg(methodName, value, type), e.getCause());
        } catch (Throwable t) {
            throw new IllegalArgumentException(makeErrorMsg(methodName, value, type), t);
        }
    }

    private static String makeErrorMsg(String methodName, String value,  Class<?> type) {
        String methodSignature = String.format("%s.%s('%s')", type.getName(), methodName, value);
        String message = " Exception thrown in static factory method '%s'. See underlying "
                + "exception for additional information.";
        return canNotParseErrorMsg(value, type, String.format(message, methodSignature));
    }
}

package com.github.drapostolos.typeparser;

import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;

class Util {
    private Util() { /* Not meant for instantiation */}

    static String nullArgumentErrorMsg(String argName) {
        return String.format("Argument named '%s' is illegally set to null!", argName);
    }

    static Class<?> extractTypeParameter(TypeParser<?> typeParser) {
        return extractTypeParameter(typeParser.getClass(), typeParser);
    }
    /*
     * Pass the original TypeParser as well, to be able to print 
     * it in error message.
     */
    private static Class<?> extractTypeParameter(Class<?> c, TypeParser<?> typeParser) {
        if(c == null){
            String message = "Can not find parametirized type in %s !?!?!? Must be a  bug somewhere...";
            throw new IllegalArgumentException(String.format(message, typeParser));
        }
        for (Type t : c.getGenericInterfaces()){
            if(t instanceof ParameterizedType){
                ParameterizedType type = (ParameterizedType) t;
                if(TypeParser.class.equals(type.getRawType())){
                    return  (Class<?>) type.getActualTypeArguments()[0];
                }
            }
        }
        return extractTypeParameter(c.getSuperclass(), typeParser);
    }
}

I should also give some usage examples. Use the below static method for simple usage (only supports the default java types):

Integer i1 = StringToTypeParser.parse(" 1", Integer.class);
int i2 = StringToTypeParser.parse("2", int.class);
float f = StringToTypeParser.parse("2.2", float.class);
Number n = StringToTypeParser.parse("2.2", float.class);
MyEnum a = StringToTypeParser.parse("BBB", MyEnum.class);
File file = StringToTypeParser.parse("/path/to", File.class);
Boolean b1 = StringToTypeParser.parse("true", Boolean.class);
boolean b2 = StringToTypeParser.parse("0", Boolean.class);

where

public enum MyEnum{AAA, BBB};

Or if you want to register your own TypeParsers, build your own StringToTypeParser instance (which by default supports the java types) as follow:

StringToTypeParser parser = StringToTypeParser.newBuilder()
.registerTypeParser(new CarTypeParser())
.registerTypeParser(int.class, new MySpecialIntTypeParser())
.build();

Car volvo = parser.parseType("volvo", Car.class);

Edit: after comments from review:

Thanks for the great comments in the answer! Below are the final outcome of the changes made. The Util.java class is now removed, so is also all the reflection code, which makes a much nicer/cleaner code.


The TypeParser interface remains the same

package com.github.drapostolos.typeparser;

public interface TypeParser<T> {
    T parse(String value);
}

Replaced the static inner classes with anonymous classes instead.

package com.github.drapostolos.typeparser;

import java.io.File;
import java.util.HashMap;
import java.util.Map;

class DefaultTypeParsers {
    private DefaultTypeParsers() { new AssertionError("Not meant for instantiation"); }
    private static final String BOOLEAN_ERROR_MESSAGE = "\"%s\" is not parsable to a Boolean.";
    private static final String CHARACTER_ERROR_MESSAGE = "\"%s\" must only contain a single character.";
    private static final Map<Class<?>, TypeParser<?>> DEFAULT_TYPE_PARSERS = new HashMap<Class<?>, TypeParser<?>>();
    private static final Map<Class<?>, Class<?>> WRAPPER_TO_PRIMITIVE = new HashMap<Class<?>, Class<?>>();

    static Map<Class<?>, TypeParser<?>> copy() {
        return new HashMap<Class<?>, TypeParser<?>>(DEFAULT_TYPE_PARSERS);
    }

    static{
        WRAPPER_TO_PRIMITIVE.put(Boolean.class, boolean.class);
        WRAPPER_TO_PRIMITIVE.put(Byte.class, byte.class);
        WRAPPER_TO_PRIMITIVE.put(Short.class, short.class);
        WRAPPER_TO_PRIMITIVE.put(Character.class, char.class);
        WRAPPER_TO_PRIMITIVE.put(Integer.class, int.class);
        WRAPPER_TO_PRIMITIVE.put(Long.class, long.class);
        WRAPPER_TO_PRIMITIVE.put(Float.class, float.class);
        WRAPPER_TO_PRIMITIVE.put(Double.class, double.class);
    }

    private static <T> void put(Class<T> type, TypeParser<? extends T> typeParser){
        DEFAULT_TYPE_PARSERS.put(type, typeParser);
        if(WRAPPER_TO_PRIMITIVE.containsKey(type)){
            // add primitive targetType if existing, example int.class, boolean.class etc.
            Class<?> primitiveType = WRAPPER_TO_PRIMITIVE.get(type);
            DEFAULT_TYPE_PARSERS.put(primitiveType, typeParser);
        }
    }

    static{
        put(Boolean.class, new TypeParser<Boolean>() {
            @Override
            public Boolean parse(final String value0) {
                String value = value0.trim().toLowerCase();
                if(value.equals("true")){
                    return Boolean.TRUE;
                } else if(value.equals("false")){
                    return Boolean.FALSE;
                }
                throw new IllegalArgumentException(String.format(BOOLEAN_ERROR_MESSAGE, value0));
            }
        });

        put(Character.class, new TypeParser<Character>() {
            @Override
            public Character parse(String value) {
                if(value.length() == 1){
                    return Character.valueOf(value.charAt(0));
                }
                throw new IllegalArgumentException(String.format(CHARACTER_ERROR_MESSAGE, value));
            }
        });

        put(Byte.class, new TypeParser<Byte>() {
            @Override
            public Byte parse(String value) {
                return Byte.valueOf(value.trim());
            }
        });

        put(Integer.class, new TypeParser<Integer>() {
            @Override
            public Integer parse(String value) {
                return Integer.valueOf(value.trim());
            }
        });

        put(Long.class, new TypeParser<Long>() {
            @Override
            public Long parse(String value) {
                return Long.valueOf(value.trim());
            }
        });

        put(Short.class, new TypeParser<Short>() {
            @Override
            public Short parse(String value) {
                return Short.valueOf(value.trim());
            }
        });

        put(Float.class, new TypeParser<Float>() {
            @Override
            public Float parse(String value) {
                return Float.valueOf(value);
            }
        });

        put(Double.class, new TypeParser<Double>() {
            @Override
            public Double parse(String value) {
                return Double.valueOf(value);
            }
        });

        put(File.class, new TypeParser<File>() {
            @Override
            public File parse(String value) {
                return new File(value.trim());
            }
        });

        put(String.class, new TypeParser<String>() {
            @Override
            public String parse(String value) {
                return value;
            }
        });
    }
}

Removed the static StringToTypeParser.parse(...) method for simplicity.

package com.github.drapostolos.typeparser;

import java.io.File;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Map;

public final class StringToTypeParser {
    private static final Object STATIC_METHOD = null;
    private final Map<Class<?>, TypeParser<?>> typeParsers;

    public static StringToTypeParserBuilder newBuilder() {
        return new StringToTypeParserBuilder();
    }

    StringToTypeParser(Map<Class<?>, TypeParser<?>> typeParsers) {
        this.typeParsers = Collections.unmodifiableMap(new HashMap<Class<?>, TypeParser<?>>(typeParsers));
    }

    public <T> T parse(String value, Class<T> type) {
        if (value == null) {
            throw new NullPointerException(nullArgumentErrorMsg("value"));
        }
        if (type == null) {
            throw new NullPointerException(nullArgumentErrorMsg("type"));
        }

        // convert "null" string to null type.
        if (value.trim().equalsIgnoreCase("null")) {
            if (type.isPrimitive()) {
                String message = "'%s' primitive can not be set to null.";
                throw new IllegalArgumentException(String.format(message, type.getName()));
            }
            return null; 
        }

        Object result = null;
        if (typeParsers.containsKey(type)) {
            result = callTypeParser(value, type);
        } else if ((result = callFactoryMethodIfExisting("valueOf", value, type)) != null) {
            //
        } else if ((result = callFactoryMethodIfExisting("of", value, type)) != null) {
            //
        } else {
            String message = "There is no registered 'TypeParser' for that type, or that "
                    + "type does not contain one of the following static factory methods: "
                    + "'%s.valueOf(String)', or '%s.of(String)'.";
            message = String.format(message, type.getSimpleName(), type.getSimpleName());
            message = canNotParseErrorMsg(value, type, message);
            throw new IllegalArgumentException(message);
        }
        /*
         * This cast is correct, since all above checks ensures we're casting to
         * the right type.
         */
        @SuppressWarnings("unchecked")
        T temp = (T) result;
        return temp;
    }

    /**
     * This method is static because it is also called from {@link StringToTypeParserBuilder}.
     */
    static String nullArgumentErrorMsg(String argName) {
        return String.format("Argument named '%s' is illegally set to null!", argName);
    }

    private Object callTypeParser(String value, Class<?> type) {
        try {
            return typeParsers.get(type).parse(value);
        } catch (NumberFormatException e) {
            String message = canNotParseErrorMsg(value, type, numberFormatErrorMsg(e));
            throw new IllegalArgumentException(message, e);
        } catch (RuntimeException e) {
            throw new IllegalArgumentException(canNotParseErrorMsg(value, type, e.getMessage()), e);
        }
    }

    private String numberFormatErrorMsg(NumberFormatException e) {
        return String.format("Number format exception %s.", e.getMessage());
    }

    private String canNotParseErrorMsg(String value, Class<?> type, String message) {
        return String.format("Can not parse \"%s\" to type '%s' due to: %s", value, type.getName(), message);
    }

    private Object callFactoryMethodIfExisting(String methodName, String value, Class<?> type) {
        Method m;
        try {
            m = type.getDeclaredMethod(methodName, String.class);
            m.setAccessible(true);
            if (!Modifier.isStatic(m.getModifiers())) {
                // Static factory method does not exists, return null
                return null;
            }
        } catch (Exception e) {
            // Static factory method does not exists, return null
            return null;
        }

        try {
            if(type.isEnum()){
                value = value.trim();
            }
            return m.invoke(STATIC_METHOD, value);
        } catch (InvocationTargetException e) {
            // filter out the InvocationTargetException stacktrace/message.
            throw new IllegalArgumentException(makeErrorMsg(methodName, value, type), e.getCause());
        } catch (Throwable t) {
            throw new IllegalArgumentException(makeErrorMsg(methodName, value, type), t);
        }
    }

    private String makeErrorMsg(String methodName, String value,  Class<?> type) {
        String methodSignature = String.format("%s.%s('%s')", type.getName(), methodName, value);
        String message = " Exception thrown in static factory method '%s'. See underlying "
                + "exception for additional information.";
        return canNotParseErrorMsg(value, type, String.format(message, methodSignature));
    }
}

Removed registerTypeParser(TypeParser) method, which allowed for removing reflection code.

package com.github.drapostolos.typeparser;

import java.util.Map;

public final class StringToTypeParserBuilder {
    private Map<Class<?>, TypeParser<?>> typeParsers;

    StringToTypeParserBuilder() {
        // Initialize with the default typeParsers
        typeParsers = DefaultTypeParsers.copy();
    }

    public StringToTypeParserBuilder unregisterTypeParser(Class<?> type){
        if(type == null) {
            throw new NullPointerException(StringToTypeParser.nullArgumentErrorMsg("type"));
        }
        typeParsers.remove(type);
        return this;
    }

    public <T> StringToTypeParserBuilder registerTypeParser(Class<T> type, TypeParser<? extends T> typeParser){
        if(typeParser == null) {
            throw new NullPointerException(StringToTypeParser.nullArgumentErrorMsg("typeParser"));
        }
        if(type == null) {
            throw new NullPointerException(StringToTypeParser.nullArgumentErrorMsg("type"));
        }
        typeParsers.put(type, typeParser);
        return this;
    }

    public StringToTypeParser build(){
        return new StringToTypeParser(typeParsers);
    }
}
  • 1
    fyi. Have a look at this jodd.org/doc/typeconverter.html – Rafael Winterhalter Jan 6 '14 at 0:32
  • Thanks! Just had a quick look a the jodd type-converter and it seems to not parse a string to a type, but just convert from one type to another (example: List<Object> to int[]) – drapostolos Jan 7 '14 at 22:53
  • [too long time passed for me to edit above comment] Thanks, I did not know about the jodd library! Just had a quick look at jodd type-converter and it seems to be able to do the same as the code above :) However, I'm currently updating the type-parser library to be able to parse a string to Parameterized types (example: parse string "1,2,3,4" to a List<Integer> [1,2,3,4]). Jodd type-converter does not seem to be able to do that. But I guess this update will be a separate review :) – drapostolos Jan 7 '14 at 23:04
  • Jodd type coverter knows to parse a string to a type. And you are right, it can parse "1,2,3,4" to e.g. int[] but not List<Integer> because of type erasure in java. – игор Mar 16 '14 at 21:27
  • 1
    First version of the type-parser library is now available in maven central and github here: github.com/drapostolos/type-parser – drapostolos May 1 '14 at 13:38
up vote 3 down vote accepted

Overall, your code is very clean and looks good to me. However, I have some feedback, especially concering your use of reflection:

  1. In your DefaultTypeParsers class, you are enlisting all TypeParser implementations that are inner classes of your DefaultTypeParsers class via reflection. I would avoid that. You are both increasing the run time burden of your application (what is not too bad) and moving application (type) safety away from compile time to run time with this aproach. Also, you might introduce very subtle bugs when refactoring this code at some time in the future. I would rather list all TypeParser classes explicitly, even though your reflective solution is very elegant from an academic point of view.

  2. It is a good Java practice to use constants instead of local variables or magic values. Therefore, rather use something like

    private static final String ERROR_MESSAGE = "\"%s\" is not parsable to a Boolean.";
    

    instead of the local variable. This also allows the Java compiler to directly inline the statement and further improves the readability of your code. This advice applies to several segments of your code. Also, note that static variables are spelled in upper caps by Java convention.

  3. I feel like the trimming of input values should be up to the user and not some library magic. For example, you are trimming Integer values but not Double values. Why is that? Finally, the user should decide that. Maybe, you want to offer an interface for this purpose? You could for example offer some sort of

    interface InputPreprocessor {
      String prepare(String input, Class<?> targetType);
    }
    

    where a user could then easily implement a

    class TrimmingInputPreprocessor implements InputPreprocessor {
      @Override
      public String prepare(String input, Class<?> targetType) {
        return input == null ? null : input.trim();
      }
    }
    

    or anything that is required for a specific use case. Also, this would for example allow to replace null with some default value.

  4. I personally do not like libraries that use reflection for reading generic types. Especially if this is not known to the user. Your library will for example not be able to read the generic type of the following implementation:

    abstract class MyAbstractTypeParser<T> implements TypeParser<T> {
      // some abstract implementation
    }
    
    class MyTypeParser extends MyAbstractTypeParser<MyType> {
      // some actual implementation
    }
    

    You will not read MyType since it is not stored with the implemented TypeParser interface but with the super class. Reading generic types is actually more tricky than it often seems and cannot always be done (when the used type is still generic). If you still feel the urge of going for this solution, use a library such as gentyref that dedicates to such reflection access. You might also be interessted in this summary I once wrote on this aproach. Alternatively, I would rather add a method to TypeParser that returns the handled type:

    public interface TypeParser<T> {
      T parse(String value);
      Class<T> getParsedType();
    }
    

    This requires the interface implementor to name the type explicitly and will get rid of all trouble with the reflective soltion. (Also note that reflection is terrible to inline for your JVM if your code is executed a lot and the JVM wants to speed up its execution.)

  5. This is a minor issue but I do not think that the BooleanTypeParser should read 0 as false and 1 as true by default. This PHP-ish aproach can introduce subtle bugs.

  6. It might be a nice feature to allow for default values when a value cannot be parsed. Such as:

    Integer i = StringToTypeParser.parse("foo", Integer.class, 0);
    

    This would encapsulate clean up logic and make user code more concise. I guess such a library would be mostly used for dealing with user input such that I feel this would be a common use case.

  7. I fear that com.github.drapostolos is not applying to the Java conventions for package naming since GitHub could (I guess the chance is close to zero) want to host a Java application bound to the domaindrapostolos.github.com while you are only associated with github.com/drapostolos such that you do not (theoretically) own this package name.

  8. In my personal oppinion, I would recommend you to use immutability whereever possible. Therefore, wrap your StringToTypeParser's map by Collections#unmodifiableMap(Map) what disallows reflective changes of this map.

  9. Util is not a great name for a Java class. If I am searching for classes named Utility in one of our production projects, I find several classes by that name since in several of its many dependencies. I think the name is too generic and should be replaced by something more specific. Not for your own's sake but for those using your library such that they will get an impression what the class is a part of and can be used for only be knowing its name. You could name it TypeParserUtility or something.

  10. Why not move your DefaultTypeParsers#list() method to this utility? DefaultTypeParsers is dead weight and can be removed (also in order to make your class hierarchy leaner to the user). If you want to keep the class, make sure to declare a constructor that makes your intentions for this class explicit:

    private DefaultTypeParsers() {
      throw new AssertionError("Do not instantiate");
    }
    

    This prohibits instanciation of this class (even by reflection).

Other than that: good code.

The builder pattern is applied by the book: with collections involved, I would keep the builder mutable and only make the end product immutable. Therefore, it's a good choice that you make a copy of the map when handing it over to the TypeParser. The registration mechanism is also fine. I would however add to the TypeParser's javadoc (and thus to its implict contract) that TypeParsers should be immutable what would allow their concurrent use and reuse throghout several converters. (If you want that for your library.)

Keep up the good work!

  • THANK YOU for your constructive feedback! :) I'll make changes to the code. Should I re-edit the original post with the code changes, or post the code-changes in a new post? (Sorry, I'm new to codereview.stackexchange.com) – drapostolos Dec 27 '13 at 15:20
  • Regarding 3) Java's Double.valueOf(String) and Float.valueOf(String) removes whitespaces by default see their javadocs. To make the StringToTypeParser API more consistent I added the trim() to the rest of the Number classes. – drapostolos Dec 27 '13 at 15:48
  • I have not been to active here either but I do not think that you are supposed to post your changes as it would make the postings hard to read for others. Glad that you found my answer helpful. Consider marking it as accepted if you feel like your question is answered. – Rafael Winterhalter Dec 27 '13 at 16:12
  • Edited the question and added final versison of the code. – drapostolos Dec 28 '13 at 0:43

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.