• P (unregistered)

    TRWTF is there are no checks if the keys are numbers.

  • Little Bobby Tables (unregistered)

    This is one case where I'd be tempted to suggest: it ain't broke so don't fix it. The only immediate direct fragility I can see is the hardwired "validators".

    It clearly works -- it hasn't been amended since it was initially checked in (which says something about the care taken by the original developer), so thank whatever superstitious avatars you revere that you don't have a bug to fix here.

  • RLB (unregistered)

    They have a diff log. Luxury.

  • -to- (unregistered)

    And then you wonder why JavaScript code conventions call for only two spaces by level of indentation.

  • MiserableOldGit (unregistered)

    That looks familiar, I reckon it might be auto-generated by something, if only because I never saw poorly conceived Javascript so consistently spaced and indented.

  • Premature optimization ? (unregistered)

    Either the author optimized too much or too little : – all the "helper.isNullOrUndefined" calls could be grouped at the beginning, which would remove 2 levels of indentation and improve performance. – or stop being clever with for/break loops and rearrange your data structure to use "this.fields[fieldName]" instead of looking for "field.name == fieldName".

  • (nodebb) in reply to Little Bobby Tables

    Just because it works, doesn't mean it isn't broken. It just gets to wait for the fires to be put out first, then you can figure out how to make this thing run more efficiently.

  • Junkfoodjunkie (unregistered)

    Why the fuck wouldn't you just use a framework that allows you to access by the field's name-attribute or something similar...?

  • (nodebb)

    TRWTF is Git.

  • Barf4Eva (unregistered) in reply to Nutster

    Both of you speaking some major truths here. Had some crazy SQL in this procedure nobody had touched in 9 years... It mostly worked, such that everyone thought it was perfectly fine. Actually, had a bug in it I found when I spent the extra time to understand some of the more arcane and undocumented details. In hindsight, I think it was the likely reason for a number of infrequent manual requests to correct certain data points over the years.

    But expecting to rewrite hundreds of thousands of lines of SQL at this company... I think not. If it works, don't fix it. But if you happen to be in that neck of the woods and aren't too certain, it won't hurt to check and remove any assumptions.

    One must choose their battles wisely.

  • ooOOooGa (unregistered)

    I once worked on some code that had an entire family of hand-rolled version checking functions that each validated that the version number in question was equal to or greater than a fixed version number. So one function was checking 'is_11_0_or_later' and such like that.

    Well, these functions all did manual string parsing. And the 11.0 checking function only checked that the length of the string before the first 'dot' character was two digits and that the second digit was not zero.

    We were currently on version 13.1 and I had to argue rather strenuously that while this code isn't currently broken, it is scheduled to break horribly in version 20.0.

  • sizer99 (google) in reply to Little Bobby Tables

    It's obviously broken because it's unmaintainable.

    Now, does it work? Well, badly written code like this that hasn't been ripped out and replaced usually works 99.99% of the time. And after looking at it devs are willing to shrug and pretend the other 1/10000 is an unexplained glitch. Must be alpha particles. One other 'fix' I've seen is to massage the results of it with boilerplate code everywhere you call it. Anything to avoid putting your hands in the latrine.

  • (nodebb)

    'Curly braces'? Hmmm. In Dutch (as well as also in English) they are called 'accolades'. My dictionary, besides this, gives either 'brace' or 'curly bracket', but not that.

    O, well, this could do with 7 fewer pairs of them anyway, they are single statement blocks...

  • xtal256 (unregistered) in reply to -to-

    "And then you wonder why JavaScript code conventions call for only two spaces by level of indentation."

    Two? I thought it was four? Why not just use tabs and configure your editor's tab width to be what ever you want? Everybody wins.

  • (nodebb)

    In English I've never heard "accolade" used in that context (though I can see how an accolade can involve "embracing"). Unicode calls a '{' a LEFT CURLY BRACKET, as distinct from '𝄔', a MUSICAL SYMBOL BRACE. Me, I've always gone (parentheses) [brackets] {braces}.

  • (nodebb) in reply to Watson

    My bad, I did not try to imply that 'accolade' would be 'the' word to use here On rereading my comment, it could be mistakenly read like that.

    I agree that 'brace' or (either round, square or curly) 'bracket' in this context would be a much better and way more widely used term.

    I just meant to mention that the word accolade exists in both languages and that in Dutch, it happens to be the word for it.

  • RLB (unregistered) in reply to xtal256

    Because then people will (not may, will) mix tabs and spaces, which is The Devil.

  • WTFGuy (unregistered) in reply to Premature optimization ?

    @Premature optimization ?: Ref ... use "this.fields[fieldName]" instead of looking for "field.name == fieldName". ...

    There are a disturbing number of languages and libraries of collection / map / list classes that throw an exception when you attempt to index/lookup into the collection with a key value that's not present in the collection, is null, or (in dynamically typed languages) is the wrong datatype. I'm going to bet this dev came from an environment where at least some of that was true at least sometimes.

    So the "better safe than sorry" approach is using for loops. If the collections are guaranteed to be fairly short (tens, not hundreds of entries), the performance difference is negligible and the reduced thinking (and corner case testing) by the original dev is a worthwhile savings.

    Of course the extra WTF? thinking triggered for the subsequent maintainers is a drawback.

  • T.T.O. (unregistered) in reply to RLB

    Yup, people will mix tabs and spaces if you use tabs, but not if you use spaces. Totally makes sense.

  • James (unregistered) in reply to JiP

    I was confused about that as accolade has a different meaning that is more commonly used. However, Wikipedia informed me that they are called accolades in music. TIL

  • (nodebb) in reply to Premature optimization ?

    As the sixth comment suggests, grouping all the helper checks toward the beginning of the function then flattening the remaining loops helps massively with readability: https://gist.github.com/MrDOS/9973a0d9ba22bd6799922c1b4ad39890 It's still a red flag that there's such an impedance mismatch between their data structures and how they're accessing the data, but TRWTF is the stupid nesting.

  • Some Ed (unregistered) in reply to Little Bobby Tables

    I once was tasked with maintaining some illegible but presumed flawless code, as well as putting out many unrelated fires from issues in nearby code.

    One extended code review later, I replaced about 2200 lines of OMG with about 40 lines of nearly legible, smack in the middle of what they said I didn't need to worry about, and 90+% of my new work problems went away. (An experienced coder could read my 40 lines OK, but they really didn't come close to my current standard.)

Leave a comment on “Break my Validation”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article