trjhtr

I'm a beginner i have an assignment to write a Date class (as part of a bigger project). in my question i focus on the constructor. here's some background: the given guidelines are that the date is not expected to be valid and the following instance variables expect this input range: day- integer 1-31 month- integer 1-12 year- integer 4 digits year.

now, if an invalid day/month/year or invalid date (such as 31.2.2010) is entered, the object will be created with the date of 1.1.2000.

this is the code I've come up with and it does compile and seem to work fine.

public class Date

 private int _day;
 private int _month;
 private int _year;


 public Date (int day, int month, int year) 
 
 switch (month)
 
 case 1: 
 case 3:
 case 5:
 case 7:
 case 8:
 case 10:
 case 12: if ((day>0 && day<32) && (year>999 && year<10000))
 
 _day=day;
 _month=month;
 _year=year; 
 
 else
 
 _day=1;
 _month=1;
 _year=2000;
 
 break;

 case 4:
 case 6:
 case 9:
 case 11: if ((day>0 && day<31) && (year>999 && year<10000))
 
 _day=day;
 _month=month;
 _year=year; 
 
 else
 
 _day=1;
 _month=1;
 _year=2000;
 
 break;

 case 2: if (leap(year))
 
 if ((day>0 && day<30) && (year>999 && year<10000))
 
 _day=day;
 _month=month;
 _year=year; 
 
 else
 
 _day=1;
 _month=1;
 _year=2000;
 
 break;
 

 else
 
 if ((day>0 && day<29) && (year>999 && year<10000))
 
 _day=day;
 _month=month;
 _year=year; 
 
 else
 
 _day=1;
 _month=1;
 _year=2000;
 
 break;
 
 
 

 /** check if leap year */
 private boolean leap (int y) 
 

here are my questions:

1. Is it fine to put all that code in the constructor? will it greatly affect the processing time or cause an error? is there an alternative if its a problem?




2. Is any part of the code could be considered a bad practice? such as the switches and ifs? I'm not feeling to confident with this build despite it working fine...

sorry for indentation inconvenience and such, 1st post :(

First of all, your code does not validate the month. If the month parameter is set to anything outside the range of 1–12, then all three fields _day, _month and _year will be initialized to their default values 0.

Now, the reason you are not convinced of your code might be that it contains a lot of duplicate code, namely the validation of the year and the assignment of the three fields of your Date class. You could start to rectify this issue by putting the code to initialize the Date instance to the default value of 1.1.2000 in one place only, instead of repeating it for every possible case of an invalid date. A way to do this would be to create a static helper method that validates a given input date, and if this method returns false, i.e. if the input date is invalid, then your Date is initialized to 1.1.2000.

private static boolean isValidDate(int day, int month, int year) 
 if (year < 1000 

This will probably not compile if you add it to your code, because in your code, leap(int) is not static and can therefore not be referenced from a static context. However, since leap(int) does not depend on any of the three instance variables of Date, it would be more appropriate if leap(int) were static as well. By the way, I don't think "leap" is a very good method name, because it doesn't really describe what the method does. A more informative name might be "isLeapYear".

Now, with this helper method, the actual initialization of the Date object will be a piece of cake:

public Date(int day, int month, int year) 
 if (isValidDate(day, month, year)) 
 _day = day;
 _month = month;
 _year = year;
 else 
 _day = 1;
 _month = 1;
 _year = 2000;
 

Finally, if you don't intend to change the values of _day, _month and _year, you can make these fields final, so Date will be immutable, meaning that, once a Date instance is created, it will never change, which can make a lot of things easier simply because you don't ever have to worry about the possibility of a Date object being modified.

Getting your class compiling and working is an achievement in itself. So don't let my comments discourage you, but see them as hints for the next learning steps.

Processing time

Don't worry about that until you really experience performance problems. Even expert programmers often fail when trying to predict the performance of their code. If you run into performance issues, then learn to use analysis tools like "profilers". But most probably, you won't need them for the next few years.

Code in constructor

It's good to do validity checks in constructors. But you're right, the constructor's readability suffers from its length. The key to readability is introducing good abstractions instead of writing lengthy code, as you already did with the leap() method. Writing a monthLength(int year, int month) method and using that in the constructor, you can get rid of most switch/if statements.

Invalid dates

You chose to create a 01.01.2000 date in case of invalid date elements. Professionals will instead throw an IllegalArgumentException.

As a Java beginner, it's quite likely that you haven't learnt about the concept of exceptions yet, but it's an important thing, an elegant way to tell someone who wants to construct e.g. a 31.2.2010 date that you can't give him that date as it doesn't exist. As a caller of a constructor or method, I always want to be informed about success or failure, and exceptions are a good way to do that.

Is it fine to put all that code in the constructor?

A constructor should only assign not do any work except very basic validations like checking inputs fornull. The kind of complex validation you do should be done outside of the constructor immediately after the data has been entered into your program.

is there an alternative if its a problem?

The alternative is to create a factory method that first does the validation and either returns a (valid) object or throws an IllegalArgumentException as suggested by Ralf Kleberhoff.

This factory method could be a static method within your class. But static methods have downsides of their own. Therefore the factory method usually lives in a factory class and is not static.

underscore prefix

First of all, I do not like to use an underscore as prefix. That was made back in the day, when IDE didn't visually represent member variables as member variables. Same goes for static variables or constants. As long as you don't write your code in vi, there's no reason to do so.

guards

Your 'guards' are spread out in the constructor (the day=1, month=1,year=2000 parts). Make sure the variables are correct first, so you can be sure the rest of the code works.

Beside that, the user has no feedback, if they pass rubbish, they just end up at 01.01.2000. This is very error prone. Validate first, and throw an exception if the parameters are wrong.

And actually, those are not really guards, but default values.

code duplication (don't repeat yourself)

I count year>999 && year<10000 three times, the assignment of the 'default values' four times, the assignment of the actual parameters four times.

The year>999 ... part can easily be refactored to a separate method isYearValid or something similiar.

The default values can be assigned directly when declaring the variables. But to be honest, the type shouldn't have default values in the first place (imo).

naming / comments

The method leap has the comment /** check if leap year. First of all, that's a JavaDoc of a private method. Try to name your stuff, so nobody needs a JavaDoc or a comment anyway. If I'm coding within the type, and my code completion shows me leap(y: int), I imagine someone jumping y meters high (and high, because y is the vertical axis). Usually, methods which return a boolean, have the is prefix. But isLeap(y: int) is not enough, because a date can't jump. So go with isLeapYear(year: int).

A small and maybe unnecessary detail: In most date API's, the day is usually named "dayOfMonth", to avoid confusion with "dayOfYear" or "dayOfWeek".

Check how others implemented it

You're far form the first who implemented a date. And actually, there's a lot that can go wrong. You might want to check LocalDate on grepcode.com. The LocalDate of the OpenJDK is over 2000 lines long. Most of it is, of course, additional functionality. But everything else might be interesting.

Don't reinvent the wheel

To be honest: I don't see any reason not to use already written code. The LocalDate is fine, does everything you want and even more. And it is more 'complicated' (better: comprehensive) than one would think. I'd be somewhat disappointed if I had to write and especially maintain code, which is shipped by the jdk.

Okay, to be honest v2: Working with java.util.Date before JDK 1.8 (I think?) was a huge pain. People used that fancy jsr-addon for 1.7 or JodaTime. And to migrate that stuff - especially when your software is supporting several timezones within one VM and one backend - wasn't very economic (and quite scary). But they did a really good job in 1.8, imo.