patternjavaMinor
Date validation in Java
Viewed 0 times
javavalidationdate
Problem
I'm learning Java, and would really appreciate any tips on how to improve this code.
It's just a simple date validation class.
It's just a simple date validation class.
import java.util.Map;
import java.util.HashMap;
class DateValidator {
private int day;
private int month;
private int year;
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 Map map;
private Months(int monthMnumber, int numberOfDays) {
this.MonthNumber = monthMnumber;
this.numberOfDaysInMonth = numberOfDays;
}
static {
map = new HashMap();
for (Months m : Months.values()) {
map.put(m.MonthNumber, m.numberOfDaysInMonth);
}
}
public static Map getMap() {
return map;
}
}
DateValidator(int d, int m, int y) {
this.day = d;
this.month = m;
this.year = y;
}
private static boolean leapYear(int y) {
return ((y % 4 == 0) && (y % 100 != 0) || (y % 400 == 0));
}
public boolean checkDay() {
if (this.month == 2 && this.leapYear(this.year)) {
Months.getMap().put(this.month, 29);
}
return (this.day <= Months.getMap().get(this.month));
}
}
class Main {
public static void main(String[] args) {
DateValidator d = new DateValidator(31, 10, 90);
System.out.print(d.checkDay());
}
}Solution
A few observations in no particular order:
-
I would consider redesigning the class so that it presents a static method:
-
If you decide to stick with the current layout, then the fields
-
A minor point, but your constructor may be more readable if you use
-
Because your
-
In general, you use
-
In your
-
You've created an enum for your month data, however this is used solely to populate a map. The rest of your code just uses the map. As a result, I would suggest you remove the enum and just stick with a map, unless you plan to extend this class in future and use the enum for other purposes. If you stick with using an enum, the map can be declared final.
-
It would be easier to read your
-
In your
-
(Very minor...) Typically one would call your method
-
I would consider redesigning the class so that it presents a static method:
checkDay(day, month, year). There doesn't seem to be much benefit in requiring callers to create an instance of the class, bound to a particular date. If you do this, ensure you make your constructor private so that the class cannot be instantiated.-
If you decide to stick with the current layout, then the fields
day, month and year could be declared as final, since you don't adjust them at all within the class.-
A minor point, but your constructor may be more readable if you use
day, month and year rather than d, m and y.-
Because your
leapYear method is static, you should call it in a static way:public boolean checkDay() {
if (this.month == 2 && leapYear(this.year)) { // <----- here
Months.getMap().put(this.month, 29);
}-
In general, you use
this. more than is necessary. I guess it's a style issue, but generally code will only use this. when it's necessary to distinguish between local variables of the same name and class fields.-
In your
Months enum, you have a field named MonthNumber, but it should be monthNumber to match normal Java style.-
You've created an enum for your month data, however this is used solely to populate a map. The rest of your code just uses the map. As a result, I would suggest you remove the enum and just stick with a map, unless you plan to extend this class in future and use the enum for other purposes. If you stick with using an enum, the map can be declared final.
-
It would be easier to read your
leapYear method if you added some parentheses, rather than relying solely on operator precedence. You don't need the outer parentheses however:return (y % 4 == 0) && ((y % 100 != 0) || (y % 400 == 0));-
In your
checkDay() method, I would avoid putting data into your map. Certainly you don't want to pollute your map if you plan to move to a static checkDay() method as discussed above. I would do something like this instead:public boolean checkDay() {
int daysInMonth =
month == 2 && leapYear(year) ? 29 : Months.getMap().get(month);
return day <= daysInMonth;
}-
(Very minor...) Typically one would call your method
isLeapYear rather than simply leapYear.Code Snippets
public boolean checkDay() {
if (this.month == 2 && leapYear(this.year)) { // <----- here
Months.getMap().put(this.month, 29);
}return (y % 4 == 0) && ((y % 100 != 0) || (y % 400 == 0));public boolean checkDay() {
int daysInMonth =
month == 2 && leapYear(year) ? 29 : Months.getMap().get(month);
return day <= daysInMonth;
}Context
StackExchange Code Review Q#85579, answer score: 3
Revisions (0)
No revisions yet.