- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
I've seen this sort of pattern a lot: a void "verifyFrist" function that throws an exception if the input is bad and does nothing if it's good. Callers would start with a handful of these assertion-like things at the top of the method, and then continue as if all input is good. If it wasn't then the exception gets caught somewhere up the call stack.
Admin
The "hour > 24" is a bug; it should be "hour > 23", so there's that as well.
Admin
The built in methods do not allow minutes after 24 hours.
It would be more efficient to check day>28 before checking the months, and to skip the checks for 31 day months if the check for 30 day months has passed.
Also delay parsing the parts until immediately before the relevant checks. This would avoid unnecessary parsing if previous parts fail.
Admin
Methods called 'check' or 'verify' or similar are allowed to be written as 'if not valid, throw'. That saves you from having to write
if(!checkTimestamp(...)) { throw new IllegalArgumentException("bad timestamp"); }
... everywhere. That validation error probably wants to go all the way out to the UI/service layer/whatever to generate a 'bad input' response, so an exception is correct. And it also means your checkTimestamp method can throw the exception with a useful message because it knows exactly what is wrong. (Which isn't happening here, although at least the message echoes the input so you can see what's wrong yourself.)
This is pretty thin gruel for a WTF, honestly. Yes, IOException is the wrong exception to be thrown here, but that's quite minor. And yeah you should be able to use the built in parsing for this, but then you're just throwing a ParseException anyway.
Admin
I also admire the "xxx < 0" tests when the code has already established that every character being passed to parseInt() is a digit. Since none of them can possibly be a '-', none of the resulting integers can possibly be negative.
Admin
Allowing "year" to be 3 or 3,141,592 is . . . generous. Granted, there are no well-defined hard limits for that component as there are for the other five, there should still be some boundaries. Since an assumption that the Gregorian calendar is being used seems safe, the lower bound should be at least 1582, yes? And I'll throw out the suggestion that 3000 as an upper bound will exceed the useful lifetime of whatever software is using this method.
(I've been writing date/time code since 1985, so it's rather a sore point with me.)
Admin
I really like the parameter "name" very much. It's only used for composing the exception message.
Admin
True, I've seen worse, but one of my pet peeves is code (excepting authorization, of course) which knows the reason for a failure but refuses to pass that information back. "Figure it out yourself, stupid user!"
Admin
I actually appreciate it. The calling code could very well look like
so at least it informs which field failed to parse. There are much better ways to do so, of course, but I acknowledge that the parameter was well-meant.
Admin
When the timestamps are used as ranges, you'd be surprised how many systems will pass 00000101, 00010101, 29991231 or 99991231 here (meaning "ever since" or "forever") and expect that others believe these dates are "valid". You can of course hard code these values, or just allow any year...
Admin
Exactly. I have this exact pattern in some validation code in our product (not for dates, but same idea). If I do
checkTimestamp(fldCreated.Value, "created"); checkTimestamp(fldModified.Value, "modified");
then the user or service consumer will see a message that says "The created timestamp is invalid" or "The modified timestamp is invalid". That is useful information.
Admin
It's tempting to ban the word "check," for the same reasons we might ban meaningless fluff like "helper" or "utility." My legacy is full of "checkXyz" where it actually means either "tryGetXyz" or "xyzExists"
Admin
but if the method were a bool, then the name would be stupid. "checkTimestamp returned true, what does that mean?"
you'd want isValidTimestamp then
Admin
Because F*k leapseconds.
Admin
Pet peeve time! Only some exception types are checked. Anything that extends
RuntimeExceptionis not (put it another way, all methods are treated as if they had an implicitthrows RuntimeException). That's why you can throw anIllegalArgumentExceptionorIllegalStateExceptionfrom anywhere (and why you don't need to mark any method that uses the dot operator asthrows NullPointerException).So there's actually a very good reason for checked exceptions - they're mathematically equivalent to the Result monad.
Widget foo() throws BarExceptionis very similar toResult<Widget, Bar> foo(), and you should use them in the same way. That's whyFileNotFoundExceptionis a checked exception (the file not existing is a legitimate case that you have to handle somewhere), butIllegalArgumentExceptionis not (if you get one, in the vast majority of cases it's a bug); you could imagine returning aResult<File, FileNotFound>, but not aResult<Widget, ProgrammerDidAnOopsie>, right?Well, it's a pain when you have to implement an interface that doesn't throw the same exception type, but I more often see it come up when someone thinks handling the happy path should be sufficient. If checked exceptions force you to think about what happens in unusual-but-valid circumstances, I'll call that a win. :)
I guess what I'm trying to say is, checked exceptions are isomorphic to a really useful functional programming construct, without the specialized syntax FP tends to need.
Admin
All those throws make me want to throw IOException(up).
Admin
I don't know. It's a 60 line format checker that never gives any indication of what's wrong with the format. A goodly-sized method that pretty much ignores half its arguments and doesn't do what it was written to do is a decent WTF.
As I think more, there is a good chance that the existence of this method makes the program worse. A malformatted timestamp might have thrown a reasonable exception on use, so calling this before use might serve only to hide information.
Admin
I see Remy likes "structured" exceptions but doesn't like "checked" exceptions. Or possibly he does, when used correctly -- hard to tell.
But to play Devil's Advocate here, I don't necessarily see the overall approach as a huge WTF. (Apart from not using libraries, of course.)
At least, with an exception, the caller has to deal with failures. No options, no quarter given. Returning a bool is just asking for trouble. Of course, returning a Maybe or an Option or your wrapped bool of choice would be better, but still.
The OP "approach" could be improved by a simple lambda that takes a specific error string and adds it to the basic "msg", but that's just a trivial refactoring. If this were the quotidien garbage I have to deal with, I'd be happy with that. Easy to fix, easy to reason about, no need to track every single sodding call-site and figure out where the phantom bool went.
Admin
I would not. After 36 years of coding, it's hard to imagine anything surprising me.
Admin
Ah, well, the Gregorian calendar. Some people have a date problem, and they think "I know, I'll use 1582 as a lower boundary."
Now they have an I18N problem.
(Which of course you know, and which of course is a travesty of JWZ, and which of course argues for using libraries.)
Admin
No surprise at all, but this code also doesn't support leap seconds, where the seconds value is 60, e.g. '20161231235960Z'.
Admin
Personally I like checked exceptions of Java. It forces developers to think about problem cases from day 1. Sure you can write 'good path only' code, but it won't even compile. You certainly won't be releasing it and having it blow up in production unexpectedly. It is painful, but it is a good pain.
Compare that to C#, which also has an exception list as part of the function signature. However, you definitely don't ever, ever want to use it. Because as soon as you list one exception in that list, then if anything else in your function ever throws any other unlisted type of exception, the entire application crashes with an uncatchable error. Even if something higher up in the call stack would have caught the exception being thrown. So now that is just a delayed foot-gun ticking silently away and waiting to blow up in production someday. That is probably the biggest WTF of C#.
Admin
Made me laugh. I bow in your general direction.
Admin
In late 1985/early 1986 my boss ordered me to add I18N support to the company's applications, handing me a copy of Inside Macintosh and telling me to follow every decision Apple had made. I wrote the code and went insane testing it, because I could not get the day of week to work correctly. When I finally checked a paper calendar, I set the book on fire because their example of "Wednesday 1 Feb 1985" was wrong; that was a Friday (they had meant 1984). Lesson learned.
https://spinsidemacintosh.neocities.org/im202-figs-27-1.png
Admin
Except if it's daylight savings time, right? Then won't you have up to 25 hours?
Admin
Using IOException might not be a WTF depending on where that method is used. If you're putting that check e.g. into an existing procedure where only IOException is catched, you're making sure that an invalid timestamp is handled in the same ways like a premature end of the stream. By using a different Exception (or an IllegalArgumentException) you might be forced to change other parts of the code (if at all possible).
Admin
For some reason I can't shake the feeling that this code was decompiled from bytecode, and then hand modified. I feel like we are actually looking at code from the Codethulu project.
Admin
. . . in which case, TRWTF is not explicitly commenting such in this method.
Seriously, the lack of comments can be as bad as horrible code.
Admin
I hope you were fired for that. You were explicitly told to follow every decision Apple had made. Apple decided that it was a Wednesday, and you did not follow that! Think different, you know.
Admin
What do you gain by limiting the year to 3000? If formatting might be an issue, then 9999 at least has a reason to it. And then you take it out with the Long Now Foundation ...
Re other comments about 1582: That's a definite lower bound for the Gregorian calendar. In some countries this will allow too much, but that's better than using 1752 and prohibiting some valid years in some countries.
Admin
I'm waiting for the VMS programmers to pop up with "17 NOV 1858" as the lower bound. VAXMAN?
Admin
It's a bit lost among the rest of the insanity, but look at the first line. If timestamp is null ... just return. Now, sure, maybe that would be OK if, say, you're intent on storing this in a database somewhere. Otherwise, it's equivalent to "We've been passed a landmine. We've detected that it's a landmine. Shall we tell anyone else that it's a landmine? Nah. Let's wait for someone down the chain to dereference it. This is fine."
Admin
They are a failed experiment.
Think about this... you write code that takes a delegate as an argument and calls it. How do you know what it throws? OK, maybe you specify what it throws... how do people write their code but only throw your exceptions?
Checked exceptions work in theory in the simple cases. They don't even work in theory for more sophisticated code. In practice, you can't force idiots to do anything. They either put everything in the throws clause that the compiler tells them to, or they swallow everything. That's why C# didn't copy the feature and has no plans ever to do so.
Admin
It's a bit shitty, but how shitty it is depends on how old it is. Java's date handling was crap until fairly recently, and we all know iffy code just gets rolled forward until it really fails.
This smells like something from the old days that only recently popped up for scrutiny, and thus, a bit unfair on whoever hacked it together.
Admin
Nope, you go from 03:59:59 to 03:00:00 and corrupt the backup-in-progress by starting another backup process on top of it. ;-)
Admin
I don't get why so many developers think they need to reinvent the wheel (or at least the calendar) when they implement some date/time checking functionality. I've seen so many methods and functions that all try to tackle the problem and all fail because of leap years or because of not all months having the same length or because of the start of a new year... for heaven's sake, there's a 99.99999% chance that the framework you are using already has a "DateTime" library (or something similar) that will handle any date checks for you. And if it hasn't you might want to ask yourself if you really want to continue using that framework.
Admin
"This smells like something from the old days that only recently popped up for scrutiny, and thus, a bit unfair on whoever hacked it together."
No matter how old, it doesn't excuse that a "checkTimestamp" returns void, instead of bool.
Admin
Well, using 17 NOV 1858 as a lower bound makes sense. Anything before that is BVE (Before VMS Age), and that cannot, by definition, be relevant. ;-)
Admin
It's really not something that even needs to be excused.
if (!someComplexValidationLogic()) { throw new SomeException(); }is a bog-standard guard clause, so why does extracting that into a method suddenly make it a WTF?(Okay, I mean, the implementation of the guard clause is overcomplicated and wrong, and the choice of exception is totally wrong, but that's a separate issue.)
Admin
Maybe it's just me, but if I read something like "checkTimestamp" I expect it to return a bool. I really like it when method names do what they advertise. Maybe it's remnant from my first job that had code that lead me to submit this: https://thedailywtf.com/articles/recycled-code
Admin
As in, "I'm the Vaxman … and you're working for non-one but me?"
Admin
This function must have been coded for a very specific application (timestamps starting with 4 digit year, no separators, and ending with 'Z'), so maybe the biggest WTF is that the function prototype or headers give no warning that its not a generally applicable checker. In most cases, would have coded it as parseTimestamp(), to return a system time or some other standardized format, with the side effect of acting as a checker by throwing an exception for a bad timestamp.
@Say what: I assume that the programmer thought throwing an exception here for a null timestamp is not required because Java will throw an exception wherever the null timestamp is dereferenced. That allows the timestamp to be optional. OTOH, if you assume that passing this check ensured a valid timestamp, your code will crash somewhere else with even less information about why.
If the timestamp might be optional, could you throw a null-pointer exception in this case, and have the calling program ignore that when a null timestamp is OK? And obviously, the exception thrown when there is a string, but it's not a valid timestamp, should be something like Bad Parameter.
Using the same msg for every failure is not too bad, since it does show the offending timestamp string. However, since the timestamp includes no separators, only someone very familiar with the expected format will be easily able to spot the problem.
Admin
Fair point, the only excuse for that is if you are intentionally being an arse!
Admin
But this method does what it advertises: It checks the Timestamp. "Check" is a verb, and in exception-based environments I expect methods named with a verb to do what the verb says or throw. "DeleteFoo", delete it or throw an exception. "UpdateBlah", same. Same with "check".
If you want a return value, the method should be named with a predicate, such as "isValidTimestamp" (as suggested by shcode above).
So actually, grammatically, this is more correct that what you're used to. Well, you might prefer a different verb such as "Validate" or "Verify", though they mean almost the same.
Or how would you call this method? "CheckTimestampOrThrow"? I hope not -- consequently you'd then have to add "OrThrow" to all method names that can potentially throw. That would actually be a nice WTF, combining all the beloved advantages of Hungarian notation and checked exceptions.
And saying that such a method should not exist is also a bad idea, since it leads to code duplication in the callers, see Naomi's comment.
Admin
Depends how long you've worked with Java. In the past, methods that return a boolean where expected to start with "is" (the naming scheme was introduced with the Bean Framework), so I'd expect above method to be called "isTimestampValid" if it returned a boolean.
Admin
Today, you might use Optional, call the method getCheckedTimestampString that returns it and use orElse(defaultTS) or orElseThrow(() -> throw new WhateverException("invalid timestamp")) in dependence of the context you're using the method. Between this and the code shown in this article are at least 20 years of development of the Java Language, so solutions of that time and what you might do nowerdays can differ significantly.
Admin
Probably military. The 'Z' stands for 'Zulu time', the milops way of saying UTC/GMT 0, and Zulu timestamps tend to be some variation of YYYYMMDD HH:MM:SS Z.
Admin
For a function named "checkTimeStamp" to return Bool would be worse, because there is no indication what "Yes" and "No" mean. Does yes mean "Yes, I found something that needs fixing", or does it mean "Yes, everything is fine"?
Admin
(I know it's late, but your comment wasn't moderated for a long time.)
Now, you turned one problem into two problems (and all that without a regex :):
It's still code duplication in the callers (again, see Naomi's comment), even if wrapped in more abstraction (which doesn't make it much shorter either).
It blurs, or even abuses, the semantics of Optional. An invalid time stamp, is not an empty optional, it's invalid. You might use an empty optional in return to an empty string, but not for an invalidly formatted string. The latter should throw an exception (of course, a correct one, unlike here), and throwing should usually be done close to the check, to avoid duplication and to be able to give a detailed reason.