Can you replace the hardcoded date format with the one placed in Constants.groovy?

You can also use the SimpleDateFormat from this file, it's already declared here

Good point @alannadolny

Using SimpleDateFormat back in Grails 1.3.9 / Java 7 was a no-brainer since it was all we really had. I think we attempted to use Joda Time at some point to avoid some of the badness of the Java 7 date libraries. In Java 8 I think the datetime and formatting libraries are a bit better, so we could potentially use those. Wrapping whatever we do around a utility / service might be a good idea so we can codify our convention in one place.

So here are my recommendations.

• Add Alan's recommendation to our current coding convention (using SimpleDateFormat, Constants, configuration) to make this clear to all developers.
• If we're calling this assignDefaultPreferenceType() in a loop, we should try to find a way to share the date formatter across iterations of the loop (static variable, pass in as an argument) as this could be expensive.
• Consider adding a wrapper (utility, service) to abstract these methods so we can update the implementation without having to refactor a lot of code.
• I would also recommend using the interface / abstract class on the assignment side.
  DateFormat dateFormat = new SimpleDateFormat("MM/dd/yyyy")
• Create a discovery ticket to figure out best practices for date formatting in Java 8 and beyond.

None of these are urgent recommendations, so no need to do them here in this PR and I'll defer to @ewaterman @awalkowiak to decide whether any of these gets done at all.

@alannadolny @jmiranda It was only a copy-paste of already existing code, hence I didn't pay much attention to that, but obviously I can change it to the existing constant.

Yeah I think we'd benefit from a mini discovery to standardize how we format our dates and then create some kind of date util class as Justin mentioned that abstracts away the specific implementation in case we need to change it at some point.

For this ticket though since it's not new functionality, I think we stick with Alan's recommendation.

won't it be cleaner to check if the preference type doesn't exist at the begging of the method? Doing this change you don't have to place so much logic in the if (preferenceType) so probably it should be easier to read. If you won't do this change it looks good to me.

could you elaborate on what would be cleaner? in my opinion doing if (!preferenceType) at the beginning wouldn't change much, because the logic of assigning preferences would have to stay the same, so the only change would be, that this logic wouldn't be nested inside the if statement, unless I miss something 🤔

this isn't an expiration date though, right? That constant just happens to have the same value that we want. You could create a new feature-specific constant (PREFERENCE_TYPE_VALIDITY_DATE_FORMAT) and use that but I think I'd instead create a new generic constant based on the format that you're using.

Maybe MONTH_DAY_YEAR_DATE_FORMAT = "MM/dd/yyyy"

Anyways, this is risking re-opening the date formatting discussion so I'll defer to whatever you think is best.

I was supposed to use an existing constant. I would say technically it might be considered as the expiration date, as the validity date is somewhat a synonym, isn't it? 🤔