• (nodebb)

    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.

  • dpm (unregistered)

    The "hour > 24" is a bug; it should be "hour > 23", so there's that as well.

  • Alistair (unregistered)

    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.

  • if(!fristPost) throw new IOException(msg) (unregistered)

    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.

  • dpm (unregistered)

    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.

  • dpm (unregistered)

    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.)

  • Philip (unregistered)

    I really like the parameter "name" very much. It's only used for composing the exception message.

  • dpm (unregistered) in reply to if(!fristPost) throw new IOException(msg)

    This is pretty thin gruel for a WTF, honestly.

    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!"

  • dpm (unregistered) in reply to Philip

    It's only used for composing the exception message

    I actually appreciate it. The calling code could very well look like

    checkTimestamp(fldCreated.Value,  "created");
    checkTimestamp(fldModified.Value, "modified");
    checkTimestamp(fldExpires.Value,  "expires");
    checkTimestamp(fldDeleted.Value,  "deleted");
    

    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.

  • mihi (unregistered) in reply to dpm

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

  • if(!fristPost) throw new IOException(msg) (unregistered) in reply to dpm

    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.

  • JZ (unregistered)

    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"

  • shcode (unregistered)

    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

  • erik (unregistered) in reply to dpm

    Because F*k leapseconds.

  • Naomi (unregistered)

    Now, one of Java’s “interesting” ideas was adding checked exceptions to the language. If a method could throw an exception, it needs to announce what that exception is. This lets the compiler check and make sure that any exception which might be thrown is caught.

    Pet peeve time! Only some exception types are checked. Anything that extends RuntimeException is not (put it another way, all methods are treated as if they had an implicit throws RuntimeException). That's why you can throw an IllegalArgumentException or IllegalStateException from anywhere (and why you don't need to mark any method that uses the dot operator as throws NullPointerException).

    So there's actually a very good reason for checked exceptions - they're mathematically equivalent to the Result monad. Widget foo() throws BarException is very similar to Result<Widget, Bar> foo(), and you should use them in the same way. That's why FileNotFoundException is a checked exception (the file not existing is a legitimate case that you have to handle somewhere), but IllegalArgumentException is not (if you get one, in the vast majority of cases it's a bug); you could imagine returning a Result<File, FileNotFound>, but not a Result<Widget, ProgrammerDidAnOopsie>, right?

    It’s also a pain for developers.

    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.

  • I dunno LOL ¯\(°_o)/¯ (unregistered)

    All those throws make me want to throw IOException(up).

  • (nodebb) in reply to if(!fristPost) throw new IOException(msg)

    This is pretty thin gruel for a WTF, honestly.

    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.

  • Solw Purpose Of Visit (unregistered)

    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.

  • dpm (unregistered) in reply to mihi

    you'd be surprised how many systems will pass [...]

    I would not. After 36 years of coding, it's hard to imagine anything surprising me.

  • Solw Purpose Of Visit (unregistered) in reply to dpm

    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.)

  • Anonymous') OR 1=1; DROP TABLE wtf; -- (unregistered)

    No surprise at all, but this code also doesn't support leap seconds, where the seconds value is 60, e.g. '20161231235960Z'.

  • ooOOooGa (unregistered)

    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#.

  • dpm (unregistered) in reply to Solw Purpose Of Visit

    "I know, I'll use 1582 as a lower boundary."

    Made me laugh. I bow in your general direction.

  • dpm (unregistered)

    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

  • Big Chungus (unregistered) in reply to dpm

    Except if it's daylight savings time, right? Then won't you have up to 25 hours?

  • Lothar (unregistered)

    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).

  • adhdeveloper (unregistered)

    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.

  • dpm (unregistered) in reply to Lothar

    where only IOException is catched

    . . . in which case, TRWTF is not explicitly commenting such in this method.

          // WARNING!  FOR LEGACY REASONS, WE MUST THROW IO EXCEPTION!!  DO NOT "FIX"!!
    

    Seriously, the lack of comments can be as bad as horrible code.

  • Foo AKA Fooo (unregistered) in reply to dpm

    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.

  • Foo AKA Fooo (unregistered) in reply to dpm

    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.

  • dpm (unregistered) in reply to Foo AKA Fooo

    but that's better than using 1752

    I'm waiting for the VMS programmers to pop up with "17 NOV 1858" as the lower bound. VAXMAN?

  • Say what (unregistered)

    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."

  • (nodebb) in reply to ooOOooGa

    Personally I like checked exceptions of Java.

    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.

  • MiserableOldGit (unregistered)

    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.

  • löchleindeluxe (unregistered) in reply to Big Chungus

    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. ;-)

  • 🤷 (unregistered)

    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.

  • 🤷 (unregistered) in reply to MiserableOldGit

    "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.

  • (nodebb) in reply to dpm

    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. ;-)

  • Naomi (unregistered) in reply to 🤷

    No matter how old, it doesn't excuse that a "checkTimestamp" returns void, instead of bool.

    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.)

  • 🤷 (unregistered)

    so why does extracting that into a method suddenly make it a WTF?

    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

  • Sole Purpose Of Visit (unregistered) in reply to nerd4sale

    As in, "I'm the Vaxman … and you're working for non-one but me?"

  • markm (unregistered)

    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.

  • MiserableOldGit (unregistered) in reply to 🤷
    No matter how old, it doesn't excuse that a "checkTimestamp" returns void, instead of bool.

    Fair point, the only excuse for that is if you are intentionally being an arse!

  • Foo AKA Fooo (unregistered) in reply to 🤷

    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.

  • Lothar (unregistered) in reply to 🤷

    Maybe it's just me, but if I read something like "checkTimestamp" I expect it to return a bool.

    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.

  • Lothar (unregistered) in reply to Foo AKA Fooo

    Or how would you call this method? "CheckTimestampOrThrow"?

    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.

  • Banagher (unregistered) in reply to markm

    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.

  • gnasher729 (unregistered) in reply to 🤷

    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"?

  • Foo AKA Fooo (unregistered) in reply to Lothar

    (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.

Leave a comment on “Going on an Exceptional Date”

Log In or post as a guest

Replying to comment #:

« Return to Article