2
\$\begingroup\$

I am creating a file uploader, and one function of the uploader will parse the file name to make sure that it is in the correct format.

This is the proper format of the file name: FILENAME_USERNAME_YYYYMMDD.csv, where FILENAME and USERNAME are any chars of any length.

Specifically, I must extract the date from the file name and compare it to a date given in the .csv file content. (comparing to a date: MM/DD/YYYY).

Below is the way that I have approached the problem: but if the user happens to input a file with a random, arbitrary file name, it's very possible for the method to throw a null pointer exception.. Hoping for a suggestion for a more elegant solution.

public boolean compareDate(File file, String cellDate)
{
    String date;
    String tempDate;
    String year, month, day;

    int startIndex = file.getName().lastIndexOf("_");
    int lastIndex = file.getName().lastIndexOf(".");

            if (startIndex > 0 && lastIndex > 0)
            {
                tempDate = file.getName().substring(startIndex+1, lastIndex);

                if (tempDate.length() < 8) // 8 == chars in date
                    return false;
            }

            year = tempDate.substring(0,4);
            month = tempDate.substring(4,6);
            day = tempDate.substring(6,8);

            date = month + "/" + day + "/" + year;

            if (date.equals(cellDate) || date.contains(cellDate)) // (ex. 01/25/2013 == 1/25/2013 
                return true;
            else
                return false;
}
\$\endgroup\$
5
\$\begingroup\$

My version:

public class CodeReview_27975 {
    // match everything between the last "_" and "." [explanation in the notes!]
    private final static Pattern FILENAME_DATE_PATTERN = Pattern.compile(".*_(.*?)\\..*");

    // apply the pattern to extract date from file name
    private String extractDateFromFilename(String filename) {
        Matcher m = FILENAME_DATE_PATTERN.matcher(filename);
        if (!m.matches()) {
            throw new IllegalArgumentException(
                    "Filename doesn't match expected format: " + filename);
        }

        return m.group(1);
    }

    private boolean compareDate(String fileName, String cellDateString) {
        SimpleDateFormat fileDateParser = new SimpleDateFormat("yyyyMMdd");
        SimpleDateFormat cellDateParser = new SimpleDateFormat("MM/dd/yyyy");

        Date d1, d2;
        try {
            d1 = fileDateParser.parse(extractDateFromFilename(fileName));
            d2 = cellDateParser.parse(cellDateString);
        } catch (ParseException e) {
            throw new IllegalArgumentException(
                    "One of the input dates failed to be parsed:" + fileName
                            + ", " + cellDateString);
        }

        return d1.equals(d2);
    }

    // this is the only public method, but it delegates to private utility functions
    public boolean compareDate(File file, String cellDate) {
        return compareDate(file.getName(), cellDate);
    }

    public static void main(String[] args) {
        CodeReview_27975 app = new CodeReview_27975();
        // easy to test
        System.out.println(app.compareDate("file_user_20130125.csv", "1/25/2013"));
        System.out.println(app.compareDate("file_user_20130125.csv", "01/25/2013"));
        System.out.println(app.compareDate("file_user_20130101.csv", "1/1/2013"));
    }
}
  • split the single method into smaller, easier to test, chunks.
  • each function deals with a single concern (most of the code deals with strings, no need to pass the File object further down the stack). This makes it easier to test
  • regular expressions are useful when dealing with patterns in strings. Extracting date from file is much more robust this way (you'll most likely getting the NullPointerException for non-standard input; the current expression doesn't solve this problem but at least it reports it correctly, making it easier to create a fix)
  • comparing dates as strings is just wrong; your equals-or-contains trick would fail when comparing 1/1/2013 and 01/01/2013. Creating a Date object is a small price to pay for correctness.
  • the class could be visually longer but I believe it's easier to understand this way; most of the new code comes from exception handling - if you decided to simply make the caller handle checked exceptions, you could remove most of that.

Also, convention for compare functions is to return an integer: <0 if first argument is smaller, 0 if both are equal and >1 if the first argument if bigger. This would make the method more flexible and potentially useful for the caller. Changing the name to areDatesSame or something like that would be another option.


edit: ".*_(.*?)\\..*" pattern dissected:

.*    any character, zero or more of them at the start
_     an underscore
(.*?) capture a group (after underscore, before the dot)
\.    dot character (has to be escaped into "\\." if inside a String)
.*    any character, any number of them at the end

Regular expressions are a broad subject but knowing the basics (and where/how to look for more detailed information) will always pay off. You can start by having a look at the Java Tutorial on Regex

The expression could be changed to only accept exactly 8 digits in the group (rather than ANY string between "_" and "."), making it a bit more complicated but more specific.

Visualisation from debuggex.com:

Regular expression image

\$\endgroup\$
  • \$\begingroup\$ This is great, exactly what I was hoping for. I'm not skilled at all in regex, although I knew the solution would involve it. "Pattern.compile(".*_(.*?)\\..*")" If it's not too much trouble, can you explain the various symbols in that statement? \$\endgroup\$ – stevendao Jun 30 '13 at 22:32
  • \$\begingroup\$ Great, thank you! Yes, I know realize how important and efficient it is to learn proper regex. \$\endgroup\$ – stevendao Jun 30 '13 at 22:54
  • 1
    \$\begingroup\$ Added some explanation, hopefully it will clear things up... And as a bonus: the 8-digits group regex I mentioned would look something like this: ".*_(\\d{8})\\..*" (Edit/test live on Debuggex) \$\endgroup\$ – kryger Jun 30 '13 at 22:54

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.