4
\$\begingroup\$

I have a couple of general purpose LocalDateTime parser methods I wrote to handle Dates flowing between a number of legacy XML based processing pipelines over to a modern Spring Boot, Mongo applications. Because both the legacy and modern applications are set in stone, I'm stuck with a narrow conversion of legacy String dates to LocalDateTime. That said I wanted to present it for review and perhaps improve the code. The various legacy String Dates we have are :

    // 200701     - 6  yyyyMM
    // 032007     - 6  MMyyyy
    // 2007-01    - 7  yyyy-MM
    // 03-2007    - 7  MM-yyyy
    // 03012007   - 8  MMddyyyy
    // 20070301   - 8  yyyyMMdd
    // 03-01-2007 - 10 MM-dd-yyyy
    // 2007-03-01 - 10 MM-dd-yyyy

And the methods I came up with are : The primary method is parseDate(), I just pass in the legacy String Date and basically have it figure out the length and then try and convert it to LocalDateTime. It works and all the JUnit tests pass.

public LocalDateTime    parseDate(String dateString) {
    checkArgument(!Strings.isNullOrEmpty(dateString), "Date [" + dateString + "] is empty of NULL!");
    LOG.debug("parseDate.................................Date [{}]", dateString);
    LocalDateTime   ldateResults = null;
    dateString = dateString.replace("/","-");

    switch (dateString.length()) 
    {
        case 6:
            try { ldateResults = getLocalDateTimeFromString ( dateString, "MMyyyy" ); }
            catch ( Exception ltwoXcp ) {
                LOG.warn("Date [{}] failed with format [MMyyyy], trying another", dateString);
                try { ldateResults = getLocalDateTimeFromString ( dateString, "yyyyMM" ); }
                catch ( Exception loneXcp ) {
                    LOG.warn("Date [{}] failed with format [yyyyMM], trying another", dateString);
                }
            }
            break;
        case 7:
            if ( dateString.indexOf("-") > 3 ) {
                try { ldateResults = getLocalDateTimeFromString ( dateString, "yyyy-MM" ); }
                catch ( Exception loneXcp ) {
                    LOG.warn("Date [{}] failed with format [yyyy-MM], trying another", dateString);
                }
                try { ldateResults = getLocalDateTimeFromString ( dateString, "MM-yyyy" ); }
                catch ( Exception ltwoXcp ) {
                    LOG.warn("Date [{}] failed with format [MM-yyyy], trying another", dateString);
                }
            }
            break;
        case 8:
            try { ldateResults = getLocalDateTimeFromString ( dateString, "MMddyyyy" ); }
            catch ( Exception loneXcp ) {
                LOG.warn("Date [{}] failed with format [MMddyyyy], trying another", dateString);
                try { ldateResults = getLocalDateTimeFromString ( dateString, "yyyyMMdd" ); }
                catch ( Exception ltwoXcp ) {
                    LOG.warn("Date [{}] failed with format [yyyyMMdd], trying another", dateString);
                }
            }
            break;
        case 10:
            try { ldateResults = getLocalDateTimeFromString ( dateString, "MM-dd-yyyy" ); }
            catch ( Exception loneXcp ) {
                LOG.warn("Date [{}] failed with format [MM-dd-yyyy], trying another", dateString);
                try { ldateResults = getLocalDateTimeFromString ( dateString, "yyyy-MM-dd" ); }
                catch ( Exception ltwoXcp ) {
                    LOG.warn("Date [{}] failed with format [yyyy-MM-dd], trying another", dateString);
                }
            }
            break;
    }
    
    return ldateResults;
    
}
public LocalDateTime getLocalDateTimeFromString(String dateString, String dateFormat) {
    DateTimeFormatter dateFormatter = DateTimeFormatter.ofPattern(dateFormat);
    if ( dateFormat.length() == 6 ) {
        LocalDate localDate = YearMonth.parse(dateString, dateFormatter).atDay(1);
        return localDate.atStartOfDay();
    }
    return LocalDate.parse(dateString, dateFormatter).atStartOfDay();
}
public String getStringFromLocalDateTime(LocalDateTime date, String dateFormat) {
    DateTimeFormatter formatter = DateTimeFormatter.ofPattern(dateFormat);
    return formatter.format(date);
}

Here are some JUnit tests that run :

@Test
public void parseDateReturnsCorrectyyyyMM() {
    String dateString="200703";
    LocalDateTime ldtValue = null;
    try {
        ldtValue = dateUtils.parseDate(dateString);
    } catch (Exception e) {
        e.printStackTrace();
        fail(e.getMessage());
    }
    if ( ldtValue != null ) {
        dateString = dateUtils.getStringFromLocalDateTime(ldtValue,"MM/dd/yyyy");
    }
    
    assertEquals("03/01/2007", dateString);
}

@Test
public void parseDateReturnsCorrectMMyyyy() {
    String dateString="032007";
    LocalDateTime ldtValue = null;
    try {
        ldtValue = dateUtils.parseDate(dateString);
    } catch (Exception e) {
        e.printStackTrace();
        fail(e.getMessage());
    }
    if ( ldtValue != null ) {
        dateString = dateUtils.getStringFromLocalDateTime(ldtValue,"MM/dd/yyyy");
    }
    
    assertEquals("03/01/2007", dateString);
}

@Test
public void parseDateReturnsCorrectyyyyMMdd() {
    String dateString="20070301";
    LocalDateTime ldtValue = null;
    try {
        ldtValue = dateUtils.parseDate(dateString);
    } catch (Exception e) {
        e.printStackTrace();
        fail(e.getMessage());
    }
    if ( ldtValue != null ) {
        dateString = dateUtils.getStringFromLocalDateTime(ldtValue,"MM/dd/yyyy");
    }
    
    assertEquals("03/01/2007", dateString);
}

@Test
public void parseDateReturnsCorrectMMddyyyy() {
    String dateString="03012007";
    LocalDateTime ldtValue = null;
    try {
        ldtValue = dateUtils.parseDate(dateString);
    } catch (Exception e) {
        e.printStackTrace();
        fail(e.getMessage());
    }
    if ( ldtValue != null ) {
        dateString = dateUtils.getStringFromLocalDateTime(ldtValue,"MM/dd/yyyy");
    }
    
    assertEquals("03/01/2007", dateString);
}

@Test
public void parseDateReturnsCorrectMM_dd_yyyy() {
    String dateString="03-01-2007";
    LocalDateTime ldtValue = null;
    try {
        ldtValue = dateUtils.parseDate(dateString);
    } catch (Exception e) {
        e.printStackTrace();
        fail(e.getMessage());
    }
    if ( ldtValue != null ) {
        dateString = dateUtils.getStringFromLocalDateTime(ldtValue,"MM/dd/yyyy");
    }
    
    assertEquals("03/01/2007", dateString);
}

@Test
public void parseDateReturnsCorrectyyyy_MM_dd() {
    String dateString="2007-03-01";
    LocalDateTime ldtValue = null;
    try {
        ldtValue = dateUtils.parseDate(dateString);
    } catch (Exception e) {
        e.printStackTrace();
        fail(e.getMessage());
    }
    if ( ldtValue != null ) {
        dateString = dateUtils.getStringFromLocalDateTime(ldtValue,"MM/dd/yyyy");
    }
    
    assertEquals("03/01/2007", dateString);
}

Tia adym

\$\endgroup\$
2
  • \$\begingroup\$ Are you sure this works correctly? I believe I have spotted two potential bugs: In case 7 the if should probably only surround the first try block and in getLocalDateTimeFromString the if should probably cover the lengths 6and 7. \$\endgroup\$
    – RoToRa
    Commented Jan 22, 2021 at 23:48
  • \$\begingroup\$ Yup, good eye...working on a rewrite using the cached formatters...since these parseDate methods reside in a Spring Boot bean, made sense to use the cache method. \$\endgroup\$ Commented Jan 23, 2021 at 16:23

2 Answers 2

5
\$\begingroup\$

"A mess" is the first thing that comes to my mind. Why don't you just use a loop over the various formats?

If you want to introduce a special guard (like the string length), create a parser objcet, which implements an appropriate check.

Something along the lines of

interface DateParser {
    boolean isApplicable(String in);
    LocalDateTime parse(String in);
}

Then create a bunch of implementations and try them all out.

You could even consider implementing the various possibilities in an enum:

enum DateParser {
    LEGACY_1("yyyyMM"),
    LEGACY_2("MMyyyy"),
    ...;
    
    private final int length;
    private final DateTimeFormatter formatter;
    
    private DateParser(String pattern) {
        this.length = pattern.length;
        // note: format created only *once*, not every time
        this.formatter = DateTimeFormatter.ofPattern(pattern);
    }
    
    public boolean isApplicable(String in) {
        return in.length == length;
    }
    
    public LocalDateTime tryParse(String in) {
         ... do whatever you need to do using formatter ...
    }
}

Then, just loop over the enum constants and call the methods as appropriate.

\$\endgroup\$
4
  • \$\begingroup\$ LEGACY_1 I'd rather call them YEAR_MONTH, MONTH_YEAR, DAY_MONTH_YEAR and so forth. \$\endgroup\$
    – Bobby
    Commented Jan 23, 2021 at 12:42
  • \$\begingroup\$ Thank you for the input! Btw, I agree it was a mess. \$\endgroup\$ Commented Jan 23, 2021 at 16:40
  • \$\begingroup\$ @Bobby I thought about a naming scheme like this, but it basically exposes an implementation detail via the name. In reality, I'd rather go for the source of the format in the enumaration constant names like MONGO, EXCEL, OLD_MAINFRAME_CRP and so on. \$\endgroup\$
    – mtj
    Commented Jan 24, 2021 at 7:18
  • \$\begingroup\$ I see, though I'd not consider the format an implementation detail in this case, but if there is a more specific name available, one should go for that, true. \$\endgroup\$
    – Bobby
    Commented Jan 24, 2021 at 8:57
5
\$\begingroup\$
public LocalDateTime    parseDate(String dateString) {

Your formatting choices are partly odd, if in doubt, stick to the IDE default formatting (or the formatting of the project.


checkArgument(!Strings.isNullOrEmpty(dateString), "Date [" + dateString + "] is empty of NULL!");

Split it into two checks, one for null and one for empty, that does make it easier to debug later.


LOG.debug("parseDate.................................Date [{}]", dateString);

Don't make me read a long line in the log if a short one is completely sufficient:

LOG.debug("MyUtilClass.parseDate(\"{}\")", dateString);

LocalDateTime   ldateResults = null;

Don't shorten variable names just because you can, it makes the code harder to read and harder to maintain. Case in point is "ltwoXcp", what's that supposed to even mean?


dateString = dateString.replace("/","-");

Do not assign to parameters, it makes the code harder to debug. Treat all parameters as final (you can also make all parameters final, but in my opinion that is too much noise for a very simple rule to follow).


try { ldateResults = getLocalDateTimeFromString ( dateString, "MMyyyy" ); }
catch ( Exception ltwoXcp ) {

Same here regarding formatting. With default Java-style formatting, the code would look like this:

try {
    ldateResults = getLocalDateTimeFromString ( dateString, "MMyyyy" );
} catch (Exception ltwoXcp) {

Again, whether or not that is easier to read is up to the reader. But overall you seem to go for tenser code, which does not improve readability or maintainability as you would believe.


LOG.warn("Date [{}] failed with format [MMyyyy], trying another", dateString);

Logging every failed parse attempt seems excessive.


public LocalDateTime getLocalDateTimeFromString(String dateString, String dateFormat) {
    DateTimeFormatter dateFormatter = DateTimeFormatter.ofPattern(dateFormat);
    if ( dateFormat.length() == 6 ) {
        LocalDate localDate = YearMonth.parse(dateString, dateFormatter).atDay(1);
        return localDate.atStartOfDay();
    }
    return LocalDate.parse(dateString, dateFormatter).atStartOfDay();
}

This seems quite costly overall. You're creating a new formatter every time you try to parse a string, and then an exception is thrown for every failed attempt.


    try {
        ldtValue = dateUtils.parseDate(dateString);
    } catch (Exception e) {
        e.printStackTrace();
        fail(e.getMessage());
    }

Just declare throws Exception on the test method and let the Exception go its natural course.


    if ( ldtValue != null ) {
        dateString = dateUtils.getStringFromLocalDateTime(ldtValue,"MM/dd/yyyy");
    }
    
    assertEquals("03/01/2007", dateString);

I'd rather compare against a specific LocalDateTime rather than against a String, as that might expose unexpected behavior in the parser method.


You return null, so the calling method should have an output like "Failed to parse <201202302340243> into a valid LocalDateTime." if your parser returns null.


Overall your logic is too complex, but not complex enough. You're making a few assumptions about what dates you will receive and how to handle them, which might be fair to do but can lead to problems. For example, a malformed date might still get parsed, "132012" will be parsed even though it was an user input error.

First, I'd cache the formatters:

public static final Map<String, DateTimeFormatter> FORMATTERS;

static {
    Map<String, DateTimeFormatter> formatters = new HashMap<>();
    formatters.put("MMyyyy", new DateTimeFormatter.ofPattern("MMyyyy"));
    formatters.put("yyyyMM", new DateTimeFormatter.ofPattern("yyyyMM"));
    formatters.put("yyyy-MM", new DateTimeFormatter.ofPattern("yyyy-MM"));
    formatters.put("MM-yyyy", new DateTimeFormatter.ofPattern("MM-yyyy"));
    formatters.put("MMddyyyy", new DateTimeFormatter.ofPattern("MMddyyyy"));
    formatters.put("yyyyMMdd", new DateTimeFormatter.ofPattern("yyyyMMdd"));
    formatters.put("MM-dd-yyyy", new DateTimeFormatter.ofPattern("MM-dd-yyyy"));
    formatters.put("yyyy-MM-dd", new DateTimeFormatter.ofPattern("yyyy-MM-dd"));
    FORMATTERS = Collections.unmodifiableMap(formatters);
}

That allows you yo easily get the cached instance based on the pattern you want to use.

Next, as we don't care that much about the DateTimeParseException, we will simply ignore it in an extra method:

private LocalDateTime parse(String dateString, String datePattern) {
    DateTimeFormatter formatter = FORMATTERS.get(datePattern);
    
    if (formatter == null) {
        throw new IllegalStateException("No formatter for pattern <" + datePattern + "> has been created.");
    }
    
    TemporalAccessor parsedResult = null;
    
    try {
        parsedResult = formatter.parse(dateString);
    } catch (DateTimeFormatterException ex) {
        // Ignore the exception, we don't care for it.
        return null;
    }
    
    if (!parsedResult.isSupported(ChronoField.DAYE_OF_MONTH)) {
        return YearMonth.from(parsedResult).atDay(1).atStartOfDay();
    } else {
        return LocalDate.of(parsedResult).atStartOfDay();
    }
}

Now what we have done here is that we have ignored the Exception at a single point. Additionally, we've decoupled the presence of the day of the month in the parsed value from the length.

Now let's return to your main logic:

LocalDateTime localDateTime = null;

switch (dateString.length()) {
    case 6:
        localDateTime = parse(dateString, "MMyyyy");
        
        if (localDateTime == null) {
            localDateTime = parse(dateString, "yyyyMM");
        }
        break;
    
    case 7:
        localDateTime = parse(dateString, "yyyy-MM");
        
        if (localDateTime == null) {
            localDateTime = parse(dateString, "MM-yyyy");
        }
        break;
    
    case 8:
        localDateTime = parse(dateString, "MMddyyyy");
        
        if (localDateTime == null) {
            localDateTime = parse(dateString, "yyyyMMdd");
        }
        break;
    
    case 10:
        localDateTime = parse(dateString, "MM-dd-yyyy");
        
        if (localDateTime == null) {
            localDateTime = parse(dateString, "yyyy-MM-dd");
        }
        break;
}

return localDateTime;

With these simple changes, we see a pattern emerging. Yes, I dropped the check whether the hyphen is contained or not, it does not help to have it in one of the branches in none of the others, the parser will tell use whether it is valid or not.

From the looks of it, it seems like we'll be able to shorten our logic considerably even more. We change our parse method to accept a given DateTimeFormatter and instead of the datePattern:

for (Entry<String, DateTimeFormatter> entry : FORMATTERS.entrySet()) {
    String format = entry.getKey();
    DateTimeFormatter formatter = entry.getValue();
    
    if (format.length() == dateString.length()) {
        LocalDateTime result = parse(dateString, formatter);
        
        if (result != null) {
            return result;
        }
    }
}

Of course, that assumes that always formats are being used that match the length of the result, but as far as I can see that's a safe assumption.

Last but not least, you can add safety checks to parse to make sure that the year 1100 is not misinterpreted as month, but it is very likely that you will never encounter such a date.

Regarding your unit tests, they are insufficient in my opinion, you can make them a lot better with more examples. I'd suggest do add your won assertion method which allows you to write the tests quicker.

private void assertLocalDateTime(int expectedYear, int expectedMonth, LocalDateTime actualLocalDateTime) {
    assertLocalDateTime(expectedYear, expectedMonth, 1, actualLocalDateTime);
}

private void assertLocalDateTime(int expectedYear, int expectedMonth, int expectedDayOfMonth, LocalDateTime actualLocalDateTime) {
    Assertions.assertNotNull(actualLocalDateTime);
    Assertions.assertEquals(expectedYear, actualLocalDateTime.getYear());
    Assertions.assertEquals(expectedMonth, actualLocalDateTime.getMonth());
    Assertions.assertEquals(expectedDayOfMonth, actualLocalDateTime.getDayOfMonth());

    // TODO Assert midnight.
}

With that your unit tests will look like this:

@Test
public void parseDateMonthYear() {
    assertLocalDateTime(2010, 1, Parser.parseDate("012012"));
    assertLocalDateTime(2010, 12, Parser.parseDate("122012"));
}

@Test
public void parseDateYearMonth() {
    assertLocalDateTime(2010, 1, Parser.parseDate("201201"));
    assertLocalDateTime(2010, 12, Parser.parseDate("201212"));
}

And so on...

\$\endgroup\$
3
  • \$\begingroup\$ Thank you for the input, tried using the TemporalAccessor as you suggested, but it seemed to fail on simpler dates, 032007, 200703, 03122007. Anyway, I did get it working as you outlined with a few modifications around TemporalAccessor...Not sure, am I supposed to post the final solution? \$\endgroup\$ Commented Jan 23, 2021 at 16:39
  • \$\begingroup\$ It failed because TemporalAccessor does only contain certain fields, for example for "032007" it only contains the month and year, and from that you can't build a LocalDate. That's why I took the detour in that case over YearMonth. And I had a typo in the logic there, it of course use YearMonth if no day is available. \$\endgroup\$
    – Bobby
    Commented Jan 23, 2021 at 16:44
  • \$\begingroup\$ Normally we don't post the revised code, as it adds nothing to the review process...and we're here for reviewing things (also, we might need to review your revised solution again, and so on...). \$\endgroup\$
    – Bobby
    Commented Jan 23, 2021 at 16:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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