snippetjavaMinor
Parse strings and convert the value to .java types
Viewed 0 times
theconvertvalueparsejavatypesandstrings
Problem
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
```
package com.github.drapostolos.typeparser;
import java.io.File;
import java.util.ArrayList;
import java.util.List;
class DefaultTypeParsers {
static List> list() {
List> result = new ArrayList>();
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 {
@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{
@Override
pu
TypeParser's, for non-standard java types (Or provide a static factory method named: valueOf(String) in your class.).- Is the usage of generics done in a correct way?
- Is the registration mechanism of custom made
TypeParser's made with best practices way?
- Is the usage of the Builder pattern done the right way?
- Any other comments?
package com.github.drapostolos.typeparser;
public interface TypeParser {
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> list() {
List> result = new ArrayList>();
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 {
@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{
@Override
pu
Solution
Overall, your code is very clean and looks good to me. However, I have some feedback, especially concering your use of reflection:
-
In your
-
It is a good Java practice to use constants instead of local variables or magic values. Therefore, rather use something like
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
-
I feel like the trimming of input values should be up to the user and not some library magic. For example, you are trimming
where a user could then easily implement a
or anything that is required for a specific use case. Also, this would for example allow to replace
-
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:
You will not read
Alternatively, I would rather add a method to
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.)
-
This is a minor issue but I do not think that the
-
It might be a nice feature to allow for default values when a value cannot be parsed. Such as:
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.
-
I fear that
-
In my personal oppinion, I would recommend you to use immutability whereever possible. Therefore, wrap your
-
-
Why not mov
-
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.-
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.-
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 ofinterface 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.-
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 implements TypeParser {
// some abstract implementation
}
class MyTypeParser extends MyAbstractTypeParser {
// 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 parse(String value);
Class 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.)
-
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.-
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.
-
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.-
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.-
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.-
Why not mov
Code Snippets
private static final String ERROR_MESSAGE = "\"%s\" is not parsable to a Boolean.";interface InputPreprocessor {
String prepare(String input, Class<?> targetType);
}class TrimmingInputPreprocessor implements InputPreprocessor {
@Override
public String prepare(String input, Class<?> targetType) {
return input == null ? null : input.trim();
}
}abstract class MyAbstractTypeParser<T> implements TypeParser<T> {
// some abstract implementation
}
class MyTypeParser extends MyAbstractTypeParser<MyType> {
// some actual implementation
}public interface TypeParser<T> {
T parse(String value);
Class<T> getParsedType();
}Context
StackExchange Code Review Q#38145, answer score: 3
Revisions (0)
No revisions yet.