• AP (unregistered)

    public static string GetFrist(int id) { for (int i = 0; i < GetProductCodes().size(); i++){ // do nothing :) } return "frist!"; }

  • kktkkr (unregistered)

    You know you have a WTF when your utility functions behave like disk benchmarks.

  • (nodebb)

    The precise grade of WTF depends on what GetProductCodes().get(i) does if there isn't a product with that ID.

    Addendum 2018-10-17 07:16: EDIT: Not to mention the disconnect between "get a product with this ID" and "get the product at position ID in the list"...

  • Code Refactorer (unregistered)

    This is an example ofa typical code duplication we found en masse in our legacy code. Some programmers argue that it's faster to write and easier to read this way instead of using an additonal line "List productCodes = GetProductCodes()" above the loop (and then referring to that variable in the loop) . And they argue that it is not a performance issue because the compiler could optimize it anyway. They are very astonished when I tell them that the compiler is not allowed to optimize here and that the method it is not an easy getter but could do performance-intensive operations (like access a database) or being changed later to do so. And that GetProductCodes.size() is executed every time in the loop, and if someone changes the size (in a parallel thread) while still iterating, the loop could crash or skip an item! I already had this discussion with many developers, so it is a real WTF that exposes the habit of a quick-and-dirty developer or beginner. I suspect that the IDE-autocomplete somehow supports this kind.

  • Dumb Dude (unregistered)

    I have produced some really stupid code in my life, but this is the single most awe-inspiring example of programming idiocy I've ever seen. Even if you have no idea what GetXYZ() does, even if you have no concept of data(base) handling, even if some external condition or legacy code forces your hand to produce more bad code - a somewhat talented, learning or at least responsible-minded programmer just HAS to feel it in his guts that these three lines are the devil's work.

  • (nodebb)

    There is certainly a WTF here. It's definitely bad code.

    But it's still not quite reasonable to expect a new team member to know calling GetProductCodes has the side-effect of making the hard drive chatter away. Even if that person in Mads Kristensen himself.

    Javadoc or XMLdoc comments and a good IDE would help that person.

    ///

    load the codes from the product flat file

  • (nodebb)

    It calls the method ID * 2 times, once for size and once more for get.

  • (nodebb)

    I am thinking that the refactored code would be: (I think this was originally in Java, but I wrote this from a C++ perspective, as I am much more familiar with C++)

    public static ProductCodeModel GetProductCode(int id) {
    List<ProductCodeModel> ProductCodes = GetProductCodes();

    if (id < 0 || id >= ProductCodes.size() )
         return null;
    else
         return ProductCodes.get(id);
}

    Also, look at where this function is called. It might be more efficient to inline this code, and only call GetProductCodes once, especially if this function is called in a loop and you do nothing with the null return value.

  • King (unregistered)

    This is a beautiful example of Enterprise level code. Carry on the good work!

  • (nodebb) in reply to tahir_ahmadov

    It calls the method ID * 2 times, once for size and once more for get.

    Nope. The call to get is conditional and only done on the one that matches, so the number of calls is id+1.

    Addendum 2018-10-17 08:47: Argh, and I'm wrong, too. id+2

  • my name is missing (unregistered)

    The big O of this function is OMG.

  • Chronomium (unregistered)

    If this "ended up in source control" amongst a team full of people who recognized the subject is a pretender, then clearly the place's code review standards are worse than its hiring ones.

  • (nodebb) in reply to Chronomium

    How do you review code that is not in source control?

  • (nodebb)

    ...and his next project yeilded a Bubble sort using GetProductCode(int).

  • Acronym (unregistered) in reply to Steve_The_Cynic

    It could be a map that maps id -> productcode...

    In this case there is a bonus WTF: the code will stop working once lower ids get deleted, making some ids higher than the map size.

  • DK (unregistered) in reply to Watson

    Pre-commit reviews are a thing. You could use Review Board or Crucible, or you could send your patch to a mailing list.

  • Bob (unregistered)

    Blue screen joke. That was real wtf?

  • gumpy_gus (unregistered)

    Meh, I've seen worse. One programmer was tasked with writing a procedure to read the Nth line of a text file.

    The delivered something, and indeed it read the first through fifth text lines just. fine. The sixth line took two seconds to show up. The seventh took 20 seconds. We didn't wait long enough for the seventh to show up. A peek at the co\de showed why. pseudocode:

    proc readxththline(x): rewind(file)

     for i = 1 to x:
         readline(out)

 return out;

    proc readline() ch = '?' y = '' while ch <> '\n': readch(file, c) y += c

    return y

    well, actually, I think there was another rabbithole, where to read the n-th character it would rewind and read n characters too. So line O() a fourth power. Amazing code.

  • (nodebb)

    Well, it could've been worse. The so-called developer could just pick up random products until he got the one he wanted.

  • Chronomium (unregistered) in reply to Watson

    How do you review code that is not in source control?

    I read the article as implying "this appeared and now they have to deal with it". I suppose it could have meant "this arrived at the gate and it was angrily turned down".

    Or "this was pushed through review because Big Boss knew it had to be good since [candidate] wrote it". But if that had happened I'd assume it would have been part of the article.

  • sizer99 (google)

    This isn't Rails, but I see it a lot with Rails programmers - they just have no concern for performance at all and seem confused by the concept that code which runs without faulting and produces the right output can still be bad. I've rewritten their 10 hour overnight processes to run in less than a minute. This 'read the file N times' is a very common anti-design pattern, for instance, and trivial for me to optimize out. Or iterating through a gigantic sorted array without using a binary search, or iterating through a hashset/dictionary, etc. etc. 'Oh, but it's more clear that way'. Only if you're brilant! And I'm sure there are some performant RoR programmers out there, I just haven't met any.

  • Derf Skren (unregistered) in reply to Watson

    "How do you review code that is not in source control?"

    With pre-commit patches or a veritable plethora of other methods.

  • (nodebb) in reply to Derf Skren

    "Pre-commit patches"? So the programmer is supposed to get a copy of the source from the branch being worked on, run diff(1) on it and the modified copy, email everyone else with the results and if they're all happy with what they see the modifications get to be saved into the VCS? There must be a more robust method than that - there's no guarantee that what gets saved is what was used in the diff. I don't like that one. What are the other methods? I thought of everyone taking turns to shoulder-surf everyone else to see the code before it gets saved, but there's still no guarantee that what gets committed is what was reviewed.

  • Mongo (unregistered) in reply to Code Refactorer

    You really think adding a "List productCodes = GetProductCodes()" would decrease the WTFness of that code? Stop giving advice to other people right now, you're not qualified!

  • sizer99 (google) in reply to Watson

    Re: Pre-commit patches: You commit the change to the in-progress branch, which is what most people are working on. That kicks off a code review. If everyone like what they see, then that patch gets merged into the production branch. If not it gets reverted or fixed.

    If you're really fancy, like JIRA, you can do the code review entirely online with no icky human contact required. It shows relevant people the patch and they can comment on it and say 'fix this first' or sign off on it. If everyone signs off, closed, promote, done. Of course if anyone wants they can request a meatbag facehole discussion on it, but that's rare.

  • gnasher729 (unregistered) in reply to sizer99

    In what f***ed up mind is iterating through a dictionary to find an item with the right key "more readable" than just indexing with the key?

  • (nodebb) in reply to Watson

    "Pre-commit patches"? So the programmer is supposed to get a copy of the source from the branch being worked on, run diff(1) on it and the modified copy, email everyone else with the results and if they're all happy with what they see the modifications get to be saved into the VCS?

    That's essentially how processes like Phabricator/Maniphest work, although (a) the review itself is webby, and the email just says where to find it. There is then a fussy distinction about whether "not in source control" means "not in source control anywhere" or "in my local git repo but not in the central 'master' repo". Phabricator works best on the second rather than the first, obviously.

    And anyone who puts one thing up for code review and then (deliberately) commits something different should be chastised severely. I recommend strapping him to a wall "downstream" from the loud end of a GAU-8. (Yes, I am advocating murdering him for this level of malfeasance. It's not a totally serious suggestion.)

  • (nodebb) in reply to gnasher729

    In what f***ed up mind is iterating through a dictionary to find an item with the right key "more readable" than just indexing with the key?

    The sort that, when presented with a need to "search the dictionary for the item", writes code to do just that, rather than asking the dictionary to search itself. That way it's clear to any reader that the code is searching the dictionary... (Fix: change the required action to "get the item from the dictionary" rather than "search the dictionary for the item" or "find the item in the dictionary".)

  • (nodebb)

    I had an off-site/out-sourced/overseas contractor (I am a contractor too) that I had to call up at 4 AM in the morning (my time) to coordinate with.

    This contactor did something similar with an iteration over a list, except that each iteration called a SOAP service which in turn did a query on a database. Suffice to say, it was very slow. I should not have had to explain to the dev that this was not desirable (much less not optimum) but I did.

  • (nodebb)

    "...and if it's the last product code in the datafile, you have read the file N times."

    N: undefined variable

  • Sparky (unregistered)

    The big question is: how was the new hire related to the boss? Or what kind of “services” did the n00b perform for the boss.

  • nasch (unregistered)

    "Pre-commit patches: You commit the change to the in-progress branch,"

    That does not sound like a pre-commit patch...

Leave a comment on “A Load of ProductCodes”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article