Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It only takes a minute to sign up.
Sign up to join this community
This is a statement that I use a lot in my code base:
(LocalDateTime.now()).format(DateTimeFormatter.ofPattern("ssmmHHddMMMuu"))
Should I create a static method for this or is it overkill?
public static String getTodaysDateInFormat(String format){
return (LocalDateTime.now()).format(DateTimeFormatter.ofPattern(format));
}
In general I think it's OK to do so, because readability is improved, and DRY principle is encouraged.
But I'd make it a bit more generic not relying on LocalDateTime.now() and pass this as a parameter:
public static String getDateInFormat(DateTime d, String format) {
return d.format(DateTimeFormatter.ofPattern(format));
}
The call
String formattedDate = getDateInFormat(LocalDateTime.now(),"ssmmHHddMMMuu");
is still shorter, and better readable.
Last but not least try to give that function a name, that expresses more precisely what it does. Something like formatDateToString() maybe.
getDateInFormat really adds anything, the original code reads just as clear to me.
\$\endgroup\$
– Simon Forsberg
Dec 14 '15 at 19:44
formatDateToString can even be called formatDate, and oh, look! That's exactly how I read LocalDateTime.now().format(DEFAULT_FORMAT)!
\$\endgroup\$
– Simon Forsberg
Dec 14 '15 at 19:45
First of all, the parenthesis around LocalDateTime.now() are useless.
Now, let's see what we're really doing here:
return LocalDateTime.now().format(DateTimeFormatter.ofPattern(format));
We're creating a unique LocalDateTime each time, but I am assuming that you always use the same format, and therefore the DateTimeFormatter.ofPattern(format) object will always be the same.
I would recommend not creating a method for this and instead extract a constant for the format:
private static final DateTimeFormatter FORMAT = DateTimeFormatter.ofPattern("ssmmHHddMMMuu");
And then each time you need it, you just do
LocalDateTime.now().format(FORMAT);
A method named getTodaysDateInFormat provides exactly the same information as the above piece of code, therefore the method does not really provide any additional value.
I don't consider LocalDateTime.now().format(FORMAT) as duplicated code. Or rather: I'm considering just as much code duplication as a call to getTodaysDateInFormat().
It's not overkill. Don't repeat yourself. Duplicated logic is troublesome. This is not exactly trivial to write, if I found myself writing it twice, I would already extract it to a helper method.
In addition to reducing duplication (which is evil), consider the readability improvement too. The method name is nicely readable, without any thinking. The statement is not too difficult, but not nearly as readable as your nicely descriptive method name.
(LocalDateTime.now()).format(DateTimeFormatter.ofPattern("ssmmHHddMMMuu"))
The first problem I see in this line of code is that it lacks a big flashing comment explaining why you aren't using an ISO-8601 compliant format.
You should, of course, have this code in a single place. I don't think a static method is quite the right answer though. I'd prefer to see it in an object that helps explain what's going on. LittleEndianTimeEncoder would be better, but it's a bit too implementation specific -- too much what, not enough why (see previous paragraph).
The way you consume this data might help you to understand the why part, which would help with naming. Take a look at the comment explaining why you are using this unusual format -- somewhere in it should be a noun that motivates the format. For instance, if you are serializing time stamps this way because the Wizzobroke service requires it, then the class that does the formatting is probably a WizzobrokeMessageEncoder.
