2
\$\begingroup\$

I read through and implemented the advice I received yesterday.

I would, once again, really like some input on how this can be improved. e.g. encapsulation, interfaces, abstract classes etc..

Right now, it's one class.

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

class DateValidator {

    private static DateValidator dateValidator;
    private static DateFormats dateFormats;

    static {
        dateValidator = new DateValidator();
    }

    private enum DateFormats {
        DDMMYYYY, MMDDYYYY, YYYYMMDD;

        private static final Map<String, Integer> setupDateFormat(DateFormats dateFormats, String date) {

                Map<String, Integer> map = new HashMap<String, Integer>();

                String value = date.replaceAll("[^0-9]","");

                switch (dateFormats) {
                    case DDMMYYYY: {
                        try {
                            map.put("day", Integer.parseInt(value.substring(0, 2)));
                            map.put("month", Integer.parseInt(value.substring(2, 4)));
                            map.put("year", Integer.parseInt(value.substring(4, 8)));
                        } catch (Exception e) {
                            System.out.print(e);
                        }
                        break;
                    }

                    case MMDDYYYY: {
                        try {
                            map.put("month", Integer.parseInt(value.substring(0, 2)));
                            map.put("day", Integer.parseInt(value.substring(2, 4)));
                            map.put("year", Integer.parseInt(value.substring(4, 8)));
                        } catch (Exception e) {
                            System.out.print(e);
                        }
                        break;
                    }

                    case YYYYMMDD: {
                        try {
                            map.put("year", Integer.parseInt(value.substring(0, 4)));
                            map.put("month", Integer.parseInt(value.substring(4, 6)));
                            map.put("day", Integer.parseInt(value.substring(6, 8)));
                        } catch (Exception e) {
                            System.out.print(e);
                        }
                        break;
                    }

                    default:
                        System.out.println("Wrong date-format...\n Choose either DDMMYYY / MMDDYYYY / YYYYMMDD");

                }

                return map;
            }

    }

    private enum Months {
        JANUARY(1, 31), FEBRUARY(2, 28), MARCH(3, 31), APRIL(4, 30), MAY(5, 31), JUNE(6, 30), 
        JULY(7, 31), AUGUST(8, 31), SEPTEMBER(9, 30), OCTOBER(10, 31), NOVEMBER(11, 30), DECEMBER(12, 31);

        private final int monthNumber;
        private final int numberOfDaysInMonth;

        private static final Map<Integer, Integer> map;

        private Months(int monthNumber, int numberOfDaysInMonth) {
            this.monthNumber = monthNumber;
            this.numberOfDaysInMonth = numberOfDaysInMonth;
        }

        static {
            map = new HashMap<Integer, Integer>();

            for (Months m : Months.values()) {
                map.put(m.monthNumber, m.numberOfDaysInMonth);
            }
        }

        private static Map<Integer, Integer> getMap() {
            return map;
        }

    }   

    public static final DateValidator setDDMMYYYY() {
        dateFormats = DateFormats.DDMMYYYY;
        return dateValidator;
    }

    public static final DateValidator setMMDDYYYY() {
        dateFormats = DateFormats.MMDDYYYY;
        return dateValidator;
    }

    public static final DateValidator setYYYYMMDD() {
        dateFormats = DateFormats.YYYYMMDD;
        return dateValidator;
    }

    //EMPTY NON-CALLABLE CONSTRUCTOR
    private DateValidator() {}

    private static boolean isLeapYear(int y) {
        return (y % 4 == 0) && ((y % 100 != 0) || (y % 400 == 0));  }

    public static boolean checkDay(String date) {
        try {
            Map<String, Integer> map = new HashMap<String, Integer>();

            map = DateFormats.setupDateFormat(dateFormats, date);
            int day = map.get("day");
            int month = map.get("month");
            int year = map.get("year");

            int daysInMonth = (month == 2 && isLeapYear(year)) ? 29 : Months.getMap().get(month);
            return day <= daysInMonth && day >= 1;
        } catch (Exception e) {
            System.out.print("You need to call setDDMMYYYY(), setMMDDYYYY(), setYYYYMMDD() FIRST\n");
            return false;
        }
    }   
}

class Main {
    public static void main(String[] args) {
        System.out.print(DateValidator.setDDMMYYYY().checkDay("16101990"));
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ You should really not return static fields. Currently you give back a singleton dateValidator. However, the functioning of that validator changes when you call another set method. That means that different parts of code can influence each other. Try and stay away from static fields, and if you do need them, keep them immutable. Calling something setXxx and then returning a value is not a good naming convention. Call it retrieveX or getInstance instead. \$\endgroup\$ – Maarten Bodewes Apr 3 '15 at 0:41
  • 1
    \$\begingroup\$ In Map<String, Integer> map the variable map is a reference. In your code you first create an empty map and assign it to map upon creation (within checkDay). Immediately afterwards you assign it a new map, throwing away the previous reference, meaning that the empty map will have to be garbage-collected. You should of course not create the empty map for nothing. Again naming: map is not a descriptive name, and I would not expect a method called setupDataFormat to return a map. \$\endgroup\$ – Maarten Bodewes Apr 3 '15 at 0:48
  • 1
    \$\begingroup\$ @MaartenBodewes That looks like good feedback - why don't you post it in an answer rather than as two long comments? \$\endgroup\$ – Duncan Jones Apr 3 '15 at 6:32
2
\$\begingroup\$

This is the most terrible part:

} catch (Exception e) {
     System.out.print(e);
}

Just let it be. Do you expect an exception? If so, avoid it. Otherwise, let it be. One day a bigger program of your starts malfunctioning, after some while you'll find that it prints null or some other ¨useful" messages to the console and after many days you may find where and maybe even why.

e.printStackTrace would be waaaaay better, but it's still a non-sense. Exception exist in order to be propagated to a place where they can be handled. I mean "handled", not "swallowed".


A similar stuff:

default:
    System.out.println("Wrong date-format...\n Choose either DDMMYYY / MMDDYYYY / YYYYMMDD");

It's an enum, and there is no other value. Some companies (e.g. Google) have a rule that a default clause must be always present. I can't really follow it as I prefer a compiler warning concerning the missing case. Anyway, a default clause printing something is wrong for about the same reason as above. Will the program work? No way. So throw an exception.


The usage should look like

DateValidator.DDMMYYYY.validate("16101990");

Make DateValidator to an enum, remove the setters, create no objects in this call as you don't need them.


I'd start like this (the comments are just for this CR):

public enum DateValidator {
    DDMMYYYY(4, 2, 0),
    MMDDYYYY(4, 0, 2),
    YYYYMMDD(0, 4, 6),
    ;

    private DateValidator(int yearIndex, int monthIndex, int dayIndex) {
        ...
    }

    public boolean validate(String date) {
        // throwing an NPE on null input is usually best
        // currently, all formats have length 8
        // (this may change later, but no need to care now)
        if (date.length != 8) return false;
        // http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/base/CharMatcher.html
        if (!MATCHER.matchesAllOf(date)) return false;
        // This can't throw as everything has been checked.
        int year = Integer.parseInt(date.substring(yearIndex, yearIndex+4));
        ...
    }

    private static final CharMatcher MATCHER = CharMatcher.range('0', '9');

    private final int yearIndex;
    private final int monthIndex;
    private final int dayIndex;
}
\$\endgroup\$

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.