0

I wrote this class that returns the fiscal year:

public class Fiscal {

private Calendar calendar;

public FiscalDate(String date) {
    try {
        DateFormat formatter = new SimpleDateFormat("yyyy-MM-dd");
        Date formattedDate = formatter.parse(date);
        this.calendarDate = Calendar.getInstance();
        this.calendarDate.setTime(formattedDate);
    }
    catch (ParseException e) {
        System.out.print(e);
    }
}
}

I'm using it like the following:

String test = new Fiscal("2020-03-31").display();

Everything works fine, I'm just wondering if there is anything wrong with my try and catch in the constructor? Any ways I can improve this class?

noobdev
  • 21
  • 5
  • 6
    I would **not** catch any exceptions. Let the caller handle that. Otherwise, do you return an uninitialized object? – 001 Mar 06 '20 at 19:31
  • Why should there by anything wrong? – dan1st Mar 06 '20 at 19:32
  • unfortunately since the java.util datetime stuff is all obsolete, best thing to do is throw most of this away. but you're right that constructor shouldn't have this code in there, let some other code handle the conversion and pass the result into the constructor. – Nathan Hughes Mar 06 '20 at 19:32
  • please refer this: https://stackoverflow.com/questions/16796076/try-catch-in-constructor-recommended-practice – Brooklyn99 Mar 06 '20 at 19:34
  • @JohnnyMopp you're saying format the string into a date before calling the FiscalDate class? that way I don't need to try catch? – noobdev Mar 06 '20 at 19:34
  • also see https://stackoverflow.com/questions/23623516/flaw-constructor-does-real-work – Nathan Hughes Mar 06 '20 at 19:35
  • @NathanHughes so remove the FiscalDate(String date) constructor and format the string to a date object before calling the FiscalDate class? – noobdev Mar 06 '20 at 19:36
  • yes, i would have a separate helper function deal with converting the string to a date. – Nathan Hughes Mar 06 '20 at 19:36
  • 1
    JohhnyMopp is correct. Don’t catch any exceptions. With that `catch` in place, if the string argument is invalid, the constructor will create a FiscalDate instance with a null Calendar. Is that what you want? Do you want FiscalDate objects in the program whose getFiscalYear method will fail with mysterious NullPointerExceptions? – VGR Mar 06 '20 at 19:38
  • 1
    Except other advices, also mark the `calendarDate` field as `final`. If you did it at first, compiler wouldn't allow you to produce exception-swallowing catch block. – Tomáš Záluský Mar 06 '20 at 19:41
  • I recommend you don’t use `calendar` and `SimpleDateFormat`. Those classes are poorly designed and long outdated, the latter in particular notoriously troublesome. Instead use `LocalDate` and `DateTimeFormatter`, both from [java.time, the modern Java date and time API](https://docs.oracle.com/javase/tutorial/datetime/). – Ole V.V. Mar 08 '20 at 07:29

3 Answers3

3

Yeah so like @Johnny Mopp said in the comments you shouldn't catch the exception silently, instead throw the exception and let the person who is implementing that class choose how to handle it.

public FiscalDate(String date) throws ParseException {
    DateFormat formatter = new SimpleDateFormat("yyyy-MM-dd");
    Date formattedDate = formatter.parse(date);
    calendarDate = Calendar.getInstance();
    calendarDate.setTime(formattedDate);
}
try {
    FiscalDate date = new FiscalDate("my date string");

    // some code that utilizes date
} catch (ParseException exception) {
    // darn, something went wrong, time to handle it!
}
Jason
  • 5,154
  • 2
  • 12
  • 22
  • 1
    I would propose to initialize the class with a `Date` to begin with, to leave parsing and model entirely separate. – daniu Mar 06 '20 at 20:06
2

I would argue against any code in the constructor. You could follow the guidelines in this article: https://www.yegor256.com/2015/05/07/ctors-must-be-code-free.html

Basically initializing your object with the string representation and parsing only at #displayFiscalYear. Any additional behavior can be achieved through decorators, like caching to avoid repeated computes.

It might seem over complicated, but IMHO it's the best approach. If this doesn't convince you though, I would go with a factory approach. Compute the parsing in a factory method and the constructor is directly initialized with a Calendar object.

P.S. I would also recommend to move to java 8's java.time if you can.

leo277
  • 439
  • 2
  • 7
  • Quote from your link: *It’s a bad idea for one reason: It prevents composition of objects and makes them un-extensible.* In the case in the question I see no harm done to composition and extension. I’d primarlily like to see a `FiscalDate(LocalDate)` constructor for callers that have already got a `LocalDate` (recommended for dates), but adding a further convenience constructor for those who’ve got a string is nice. Keeping your date as a string in the object is bad, on the other hand. – Ole V.V. Mar 08 '20 at 07:34
0
  1. Use LocalDate from java.time, the modern Java date and time API, for a date.
  2. Catching an exception in the constructor is generally bad. You may do it if you can still initialize the object to a fully valid and meaningful state or you want to wrap the exception into a different exception that you throw to the caller.

java.time

public class FiscalDate {

    private LocalDate calendarDate;

}

A LocalDate is a date in the proleptic Gregorian calendar without time of day. A Calendar is a date and time of day with time zone, week numbering scheme and more, theoretically in any calendar system. The Calendar class is also poorly designed and long outdated. So I recommend LocalDate.

Constructors

Now which constructors do we want? I’d find it natural to have a constructor that accepts a LocalDate:

    public FiscalDate(LocalDate calendarDate) {
        this.calendarDate = calendarDate;
    }

If you foresee that the caller will sometimes have the date as a string rather than a LocalDate, providing a convenience constructor that accepts a string is nice:

    /** @throws DateTimeParseException if dateString is not a valid date string in format yyyy-MM-dd */
    public FiscalDate(String dateString) {
        this(LocalDate.parse(dateString));
    }

The this() call calls the constructor from before. It’s similar to a super() call, only instead of calling a constructor in the superclass it calls a constructor in the same class. LocalDate.parse() throws a DateTimeParseException if the string cannot be parsed into a valid date. It’s an unchecked exception, so we need not declare that the constructor may throw it, but it’s nice for the caller to know, so we put it in the Javadoc comment.

I believe that deciding what to do in case of an invalid string date should be the job of the caller, not this class. So throwing the exception is appropriate (as it is or wrapped in a more appropriate exception type). If you were to catch the exception in the constructor, you would need to initialize the object, and I can’t see how you would be able to do that in any meaningful way.

Link

Ole V.V.
  • 81,772
  • 15
  • 137
  • 161