5
\$\begingroup\$

I am building a CSV file parser, and in order to get the appropriate Object to represent different data types found on the parsed files, I wrote the following function:

public static Object stringToDataType(String valueAsString) throws ParseException{
    // detections ordered by probability of occurrence in Buffer_Bank.
    String decimalPattern = detectDecimal(valueAsString);
    if(decimalPattern != null){
        return stringToBigDecimal(valueAsString, decimalPattern);
    }else{
        String integerPattern = detectInteger(valueAsString);
        if(integerPattern != null){
            return stringToBigInteger(valueAsString);
        }else{
            String datePattern = detectDate(valueAsString);
            if(datePattern != null){
                return stringToDate(valueAsString, datePattern);
            }else{
                // value is a String... nothing else to do!
                return valueAsString;                   
            }
        }
    }
}

In order to call the appropriate 'casting' methods (stringToDate, stringToBigInteger, stringToBigDecimal), I need to use the specific pattern string that gets returned when detecting if a particular value is of that type. Hence, the pattern value gets used for routing to the appropriate function, and also as an argument in that function call.

To make this method logic 'flatter', I could avoid passing the pattern as an argument, and calling the pattern detection method (detectDecimal, detectInteger, detectDate) again from within the 'casting' method, but that would lead to parsing the same value two times, which seems wasteful at best.

So, is this the best way this method can be written without getting to call the detection methods twice?

EDIT

Here are the detectDate and stringToDate methods, as requested (the methods for BigInteger and BigDecimal are very similar). Please let me know if I need to post anything else:

private static String detectDate(String dateString) {
    for (String regexp: DATE_FORMAT_REGEXPS.keySet()) {
        if(dateString.toLowerCase().matches(regexp)) {
            return DATE_FORMAT_REGEXPS.get(regexp);
        }
    }
    return null; // value is not a date
}

public static Date stringToDate(String valueAsString, String format) throws ParseException{
    SimpleDateFormat dateFormat = new SimpleDateFormat(format);
    Date date = dateFormat.parse(valueAsString);
    return date;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Sounds like a dubious approach, since there is overlap between the different types. I'd rather have the caller specify the type per-column. \$\endgroup\$ – CodesInChaos Apr 29 '16 at 6:06
  • \$\begingroup\$ @CodesInChaos you have a valid point when pointing out the overlap. In my case, an EAN code is something that would get detected as BigInteger, but the target field type should be String (because the Item Code can be an EAN, GTIN. UPC or any unique character combination that the client decides to use). I will reconsider my overall architectural approach. Anyway, the answers provided are still valid and very insightful regarding my coding approach. \$\endgroup\$ – carlossierra Apr 29 '16 at 14:20
6
\$\begingroup\$

Yes, you can make that code a lot more readable by noticing a simple fact. When you have the code

String decimalPattern = detectDecimal(valueAsString);
if(decimalPattern != null){
    return stringToBigDecimal(valueAsString, decimalPattern);
}else{
    // ...
}

The else part is actually unnecessary. If the test is true then we return with a value. This means that you exit early from the method, without going further down.

So you can rewrite it like this:

String decimalPattern = detectDecimal(valueAsString);
if(decimalPattern != null){
    return stringToBigDecimal(valueAsString, decimalPattern);
}
// ...

As you found out, this has the advantage that the subsequent code is shifted by one indentation to the left, making it easier to read. So only with this change, the code now looks like:

public static Object stringToDataType(String valueAsString) throws ParseException {
    // detections ordered by probability of occurrence in Buffer_Bank.
    String decimalPattern = detectDecimal(valueAsString);
    if (decimalPattern != null) {
        return stringToBigDecimal(valueAsString, decimalPattern);
    }
    String integerPattern = detectInteger(valueAsString);
    if (integerPattern != null) {
        return stringToBigInteger(valueAsString);
    }
    String datePattern = detectDate(valueAsString);
    if (datePattern != null) {
        return stringToDate(valueAsString, datePattern);
    }
    return valueAsString;
}

You posted an example of your detect* and stringTo* methods. In fact, you don't need two methods. You only need a single one that is specific to the current type you want to test. What you want is:

  • Try to interpret the value as a decimal and return it;
  • If not, try to interpret the value as an integer and return it;
  • If not, try to interpret the value as a date and return it.

Said like this, it is possibe to realize that you only need 3 methods: tryDate, tryInteger and tryDecimal. Each of those will do the necessary to try to interpret the String and return null if they can't. Basically, the code you currently have inside stringTo* should be merged into the part where you found the correct pattern in detect*. As an example:

private static Object tryDate(String dateString) {
    for (Map.Entry<String, String> entry : DATE_FORMAT_REGEXPS.entrySet()) {
        if (dateString.toLowerCase().matches(entry.getKey())) {
            SimpleDateFormat dateFormat = new SimpleDateFormat(entry.getValue());
            try {
                return dateFormat.parse(dateString);
            } catch (ParseException e) {
                // what to do in this case? Possibilites: throw an exception or return null
            }
        }
    }
    return null; // value is not a date
}

Note that I changed your code so that it loops directly around the entries instead of looping just with the keys. You should also decide what to do in case of a ParseException. You could rethrow a custom runtime exception, wrapping the original, or returning null to signal that the attempt failed.


The big issue is that there is some duplicated logic in this: each time we are detecting a pattern and, if it's not null, applying it to the value. If you were to add more detection algorithm, this method can become very quickly copy/pasted code and clumsy. We need to refactor that and make it generic.

Using Java 8, you can define that as a list of Function.

private static final List<Function<String, Object>> FUNCTIONS = 
    Arrays.asList(s -> tryDecimal(s), s -> tryInteger(s), s -> tryDate(s));

The goal of those functions is to convert a String into a Object. Here, we define 3 which are the 3 methods mentioned above. Then we can have

public static Object stringToDataType(String valueAsString) {
    return FUNCTIONS.stream()
                    .map(f -> f.apply(valueAsString))
                    .filter(Objects::nonNull)
                    .findFirst()
                    .orElse(valueAsString);
}

This creates a Stream pipeline over the functions. Each of them is applied to the given String, non-null elements are filtered and only the first one is kept. Effectively, this will select the first non-null element, so it will select the first mapping that matched. If all are null then we return a default value with orElse which is the given value as String.


If somehow you want to keep your current approach with two different methods (one for detecting the pattern and one for parsins), then what you want comes down to creating a custom class that will hold:

  • A function that returns the pattern detected given the value as String;
  • A function that returns the object given the value as String and a pattern.

Simply, it could be:

public final class Conversion {

    private final UnaryOperator<String> patternDetector;
    private final BiFunction<String, String, Object> toObjectFunction;

    public Conversion(UnaryOperator<String> patternDetector, BiFunction<String, String, Object> toObjectFunction) {
        this.patternDetector = patternDetector;
        this.toObjectFunction = toObjectFunction;
    }

    public UnaryOperator<String> getPatternDetector() {
        return patternDetector;
    }

    public BiFunction<String, String, Object> getToObjectFunction() {
        return toObjectFunction;
    }
}

Then we can make a static utility that applies a given Conversion to a given value:

private static Object stringToDataType(String valueAsString, Conversion conversion) {
    String pattern = conversion.getPatternDetector().apply(valueAsString);
    if (pattern != null) {
        return conversion.getToObjectFunction().apply(valueAsString, pattern);
    }
    return null;
}

and finally, we can refactor the main method:

private static final List<Conversion> CONVERSIONS = Arrays.asList(
    new Conversion(v -> detectDecimal(v), (v, p) -> stringToBigDecimal(v, p)),
    new Conversion(v -> detectInteger(v), (v, p) -> stringToBigInteger(v)),
    new Conversion(v -> detectDate(v), (v, p) -> stringToDate(v, p))
);

public static Object stringToDataType(String valueAsString) throws ParseException {
    return CONVERSIONS.stream()
                      .map(c -> stringToDataType(valueAsString, c))
                      .filter(Objects::nonNull)
                      .findFirst()
                      .orElse(valueAsString);
}

Now, if we want to add more conversion in the future, we just need to update this list that, arguably, should be made a constant instead of being initialized each time. The Stream pipeline will return the first non-null element and will short-circuit once it has been found. If all are null, there we return the default value which is the given value as string.

\$\endgroup\$
  • \$\begingroup\$ Could you please explain a little bit more what's happening in your second part of the answer? I still need to provide the current detectX and stringToX methods, right? \$\endgroup\$ – carlossierra Apr 28 '16 at 22:39
3
\$\begingroup\$

Actually, you can use enums for this and encapsulate the logic in them.

e.g.:

public enum DataType {

    STRING("[A-Za-z_$][A-Za-z0-9_$*]") {
        @Override
        public Object getValue(String input) {
            return input;
        }
    },

    INTEGER("(?:[1-9][0-9]*)|(?:0x[1-9A-Fa-f][0-9A-Fa-f]*)") {
        @Override
        public Object getValue(String input) {
            try {
                return Integer.parseInt(input);
            }
            catch (NumberFormatException e) {
                return 0;
            }
        }
    };

    private Pattern pattern;
    private DataType(String pattern) { 
        this.pattern = Pattern.compile(pattern);
    }

    public boolean isType(String input){
        return pattern.matcher(input).matches();
    }

    public abstract Object getValue(String input);

    public static DataType getType(String input){
        for(DataType type : values())
            if (type.isType(input))
                return type;
        return null;
    }
}

Then you can just use the static get method to handle everything. (I find that a method like that is really useful for enums):

DataType.getType(input).getValue(input);

This method is also much better than an if else chain in that if you need to define more data types to handle, all you need to do is add a simple enum entry to the enum class. (No dealing with adding more if else clauses and accidentally forgetting an else.)

This also has the additional benefit of allowing you to selectively convert types later on. If you need to convert something specifically, you can simply use the right enum:

DataType.STRING.getValue(input);
\$\endgroup\$
2
\$\begingroup\$

You can enhance the readability of your code with something like this:

SomeEnum someEnum = detectPattern(someValueAsString);
switch (someEnum) {
    case Decimal:
        System.out.println("De");
        // Do something
        break;
    case Integer:
        System.out.println("In");
        // Do something
        break;
    case Date:
        System.out.println("Da");
        // Do something
        break;
}

where you have only one function that detects the pattern and based on that detection, returns an Enum which you can switch to process further. Since, you did not post code for detect*, can not really tell about those implementations to generify things.

\$\endgroup\$
  • \$\begingroup\$ I just posted code to show the other methods. Also, I had implemented something similar to this at the beginning, but decided to change it because having an Enum as an intermediate step seemed unnecessary. My rationale is that the Enum couldn't be used as the pattern for the parse functions, could it? \$\endgroup\$ – carlossierra Apr 28 '16 at 22:18
2
\$\begingroup\$

On obvious observation is that the code would look better simply by eliminating the else clauses.

public static Object stringToDataType(String valueAsString) throws ParseException {
    String pattern;
    if (null != (pattern = detectDecimal(valueAsString))) {
        return stringToBigDecimal(valueAsString, pattern);
    }
    if (null != (pattern = detectInteger(valueAsString))) {
        return stringToBigInteger(valueAsString);
    }
    if (null != (pattern = detectDate(valueAsString))) {
        return stringToDate(valueAsString, pattern);
    }
    return valueAsString;
}

What could possibly cause a ParseException? Isn't the function supposed to fall back to returning a String in case the value cannot be interpreted as anything else? It seems tricky to define exactly the right regular expressions for each data type. Judging from your stringToDate() function, it seems that the ParseException would arise from a mismatch between what your regular expression thinks is valid and what the real constructor or parser actually accepts. I would consider that to be more of an assertion failure, with the root cause being a fragile design.

If you want to know what type of data is available next in a data stream, you can use methods in java.util.Scanner to do that. Note the advice in the cited answer: trying to do this type detection in a general-purpose CSV parser is probably a bad idea. Since this function returns Object, the caller would need to do some kind of instanceof check anyway. You might as well let the caller decide when to attempt to do the type conversion.

\$\endgroup\$
  • \$\begingroup\$ You are right about the ParseException. Regarding the Object type being returned, at first I didn't think it was a problem, because in my particular case the caller passes the result to an SmartGWT API that expects a Map<String, Object>, where each Object is required to be of the appropriate type. Then, SmartGWT handles the Objects internally to persist them into a DB). Anyway, the comments here made me realize that trying to detect the data type is a bad idea in the first place because of the possible overlaps, so I am rethinking my approach. \$\endgroup\$ – carlossierra Apr 29 '16 at 14:42

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.