5
\$\begingroup\$

Is this return style acceptable? Would the code be more readable if I assigned the MonthDay.parse() return value to a variable and returned that variable on the last line?

Also, is it a bad idea to both log and rethrow inside a catch block?

/**
 * Parses showing date from the header text.
 * 
 * @throws PageStructureException if the date string does not conform to the expected format
 */
public MonthDay parseShowingDate( String date ) throws PageStructureException {
    Preconditions.checkNotNull( date );

    DateTimeFormatter matDTF = DateTimeFormat.forPattern( DATE_FORMAT );
    String dayAndMonth = removeDayOfWeek( date );
    try {
        return MonthDay.parse( dayAndMonth, matDTF );
    } catch ( IllegalArgumentException iae ) {
        logger.error( "Could not parse MAT date {}, expected format [{}].", date, DATE_FORMAT );
        throw new PageStructureException( iae );
    }
}
\$\endgroup\$
6
\$\begingroup\$

I believe it's OK. This way, it is immediately clear from the code that the method returns a value only when no exception is thrown during parsing. If you used a variable to store the result, it becomes less clear. And, the reader has to track where the variable is initialized/modified/returned.

Logging an exception when it's rethrown is a bit confusing, because it is possible the caller expect the possibility of receiving an exception and deals with it. But sometimes a programmer needs it so that it's possible to examine what's happening even if the caller catches the exception. In such a case, I'd suggest using only debug or trace logging levels.

\$\endgroup\$
4
\$\begingroup\$

It is a very good understandable write.

You can also write the return method in a parameters, it is optimal (look in Effective Java all subtle reasons):

try {
    return MonthDay.parse( removeDayOfWeek(date), DateTimeFormat.forPattern(DATE_FORMAT));
} catch ( IllegalArgumentException iae ) {
    logger.error( "Could not parse MAT date {}, expected format [{}].", date, DATE_FORMAT );
    throw new PageStructureException( iae );
}

I've not included here Preconditions.checkNotNull( date ), because it is not really a parameters, but I'll do it if it was my job,

\$\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.