• Prime Mover (unregistered)

    "It’s not… bad."

    Reminds me of the joke:

    "Why did Michael Jackson call one of his albums 'Bad'?"

    "Because he couldn't spell 'Appalling'."

  • Moogle (unregistered)

    I've seen that style of for-loop before. It's not so unusual, and it's useful because in some languages it scopes the working array to the loop itself, which is nice. There's no WTF there.

    The body of the loop, on the other hand ... people have been executed in the streets for less. (if you're in the USA, there's some pretty good evidence of this right now; if you're elsewhere, consider it to be hilarious hyperbole 😊)

  • WTFGuy (unregistered)

    Disclaimer: IANA Java guy. But I did Google up a couple of Java tidbits before posting this.

    If this is ever called with the numResults parameter = 0 we'll get some interesting effects. No way to know what GetResults(0) will do: return null, an empty array, or throw? Assuming no throw ...

    We know what the rest of the loop will do: nothing, it won't be iterated even once.

    Then, having first initialized sb to null above the loop, they call sb.ToString() in the return statement. Smells like a NullPointerException to me.

    The fact they chose to defer allocating the StringBuffer until the first pass through the loop sorta-implies they were trying to avoid allocating it unnecessarily. if true, that means they were anticipating the possibility of zero passes through the loop. For which they wrote a sure-fire crash.

    I also like using names i & j, both commonly used in for loops as indexer values, to mean the actual business data being manipulated and returned. This whole thing is almost designed to be misunderstood.

    Bottom line: Remy hit all the high points of their twisted / half-baked thinking. But there's more.

  • Foo AKA Fooo (unregistered) in reply to Moogle

    I agree the loop is not the worst thing here. Scoping might be an advantage, though in this case not so much, since the loop basically is the whole function body. So I'd pull out the initialization, and use a while-loop for the rest (decrement-and-check is alright, though), so not a huge difference.

    But the body of the loop, yeah! Incidentally, recent GCC versions have warnings for both issues, misleading indentation and identical code in if and else parts. So we're getting somewhere. That is, if programmers (especially this kind of programmers) would actually enable and heed such warnings ...

  • Foo AKA Fooo (unregistered) in reply to WTFGuy

    a) You might be giving them too much credit. I think they just (mis)used null as an indicator of first item (so no comma), and probably didn't actually consider the 0 case.

    b) Especially since i and j are actually not needed (in particular if you eliminate the nop-if). What's wrong with "sb.append (result[numResults] + 1);"? I say that's much more readable than distributing the same "logic" over 3 lines, with other code interspersed.

  • AverageCoder (unregistered)

    I'm surprised that everyone has missed that we are appending the indexer (j) NOT (getresults[j]) to the string buffer. So we are making a list of the indexer for an array... so why did we get an array in the first place?

  • Krister (unregistered)

    Bah! Why no ternification? Since sb.append handily returns sb, we could write as follows:

    sb = (sb == null ? new StringBuffer() : sb.append(",") ).append(j);

    Don't like ternaries? How about:

    try { sb.append(","); } catch(NullPointerException e) { sb = new StringBuffer(); } finally { sb.append(j); }

  • Naomi (unregistered) 
    var results = getResults(numResults);
return IntStream
    .range(0, results.length)
    .map(i -> results[results.length - i] + 1)
    .mapToObj(Integer::toString)
    .collect(Collectors.joining(","));

    The fact they chose to defer allocating the StringBuffer until the first pass through the loop sorta-implies they were trying to avoid allocating it unnecessarily.

    If that's the case, then one of the many RWTFs is that they've overcomplicated their logic to avoid a single object allocation, but didn't think to use a StringBuilder - StringBuffers are thread-safe, with all the overhead that implies. (It's technically possible that this code predates Java 5, but I don't think that's very likely.)

  • Twither (unregistered)

    What bothers me most is "int result[]". Was this written by a recovering C programmer? Why does Java even permit this syntax?

  • (nodebb)

    The fact they chose to defer allocating the StringBuffer until the first pass through the loop sorta-implies they were trying to avoid allocating it unnecessarily.

    It doesn't imply that at all. They're using the allocation as a boolean for "is this the first time" to avoid a leading comma in the final result.

    Confession: I've done that too.

  • Krister (unregistered) in reply to Twither

    That's how you declare a primitive array type.

  • Anon (unregistered)

    The "goes to" operator is free-style programming, not WTF

  • looper (unregistered)

    It's an optimized code. You could write: foo(); while (++bar < 10) {...} But then you notice that you can save at least 3 characters by writing: for (foo(); bar < 10; ++bar) {...} And finally save at least 3 more characters by doing this: for (foo(); ++bar < 10;) {...} So it's 6 characters saved in just one loop, imagine the disk space reclaimed if you did this everywhere!

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

    The author missed the perfect chance to use the "down-to" operator '-->':

    for (...; numResults --> 0; )
  • Sole Purpose of Visit (unregistered)

    And this doesn't get held for moderation?

    Needs moar functional cowbells.

  • ooOOooGa (unregistered)

    So someone hooked up a text generator to the comment section. But it is all of the actual commenters that are having their posts held for moderation. The text generator gets through just fine.

    That's TRWTF right there.

  • Enotsola (unregistered)

    That indenting is the kind of trick I've seen teachers use to catch people off guard on tests.

  • cavaj (unregistered)

    Honestly, I rather get the feeling this is some decompiled-and-then-maybe-mucked-with code.

  • Lothar (unregistered) in reply to Twither

    Java allows that because its designers decided to allow that ;-) This syntax allows you to declare arrays and primitive types in one line:

    int myInt, myIntArray[], myOtherInt;

    YMMV but I'm not a fan of that and prefer one declaration per line where this feature is completely unnecessary.

  • Merus (unregistered)

    I mean, I involuntarily said "what the fuck" when I realised what was going on, so I guess I am a satisfied customer and do not need to be told what TRWTF is

  • WTFGuy (unregistered)

    @Averagecoder ref

    I'm surprised that everyone has missed that we are appending the indexer (j) NOT (getresults[j]) to the string buffer. So we are making a list of the indexer for an array... so why did we get an array in the first place?

    If you look carefully, j is NOT the indexer. Neither is i. As I pointed out in my earlier post, on each iteration, i set to the value of the relevant array entry. Then that value is incremented into j. Then j is stored to the accumulating StringBuffer.

    Sadly, you've lived up to your screen name. Which is why TDWTF will never run out of material.

    @Foo AKA Fooo This may be a Java idiom (I'm a C# guy), but folding the for(...) statement inc/decrement and check clauses into one feel real weird / misleading to me. Yet you say it's fine.

  • Foo AKA Fooo (unregistered) in reply to WTFGuy

    In a for-loop it may feel strange (though it's really just that, a feeling -- technically, there's no problem with it), but I said to turn it into a while-loop, where it feels more normal. (It's basically just using the "--" operator for its purpose, unless your style bans using the value of that operator entirely, i.e. only allows using it as if it were a statement without a value.)

  • Naomi (unregistered) in reply to WTFGuy

    @Foo AKA Fooo This may be a Java idiom (I'm a C# guy), but folding the for(...) statement inc/decrement and check clauses into one feel real weird / misleading to me. Yet you say it's fine.

    It's a fairly common idiom (regardless of language - okay, I guess not in Python!) for iterating backwards:

    // Frob the widgets in reverse order.
for (var i = widgets.size(); --i >= 0;) {
    widgets.get(i).frob();
}

    There are other ways to do it, of course:

    for (var i = widgets.size(); i > 0; --i) {
    widgets.get(i - 1).frob();
}

if (widgets.size() > 0) for (var i = widgets.size(); i >= 0; --i) {
    widgets.get(i).frob();
}

for (var i = 0; i < widgets.size(); ++i) {
    widgets.get(widgets.size() - i - 1).frob();
}

    Which of them is "least misleading" comes down to taste and your team's conventions, I think.

  • WTFGuy (unregistered)

    @Foo AKA Fooo: Thank you. I agree that as the control clause of a while loop it (the combined mutate indexer and range test operation) makes perfect sense and fits perfectly with the feature set of the while clause.

    ISTM the whole point of the for clause is that it has 3 separate sub-clauses: initialize, test, & mutate. Yes, each of them is separately optional and in fact all 3 together are optional. The fact you (any you) can cleverly combine 2 of the subclauses into 1 clause, and omit the other doesn't mean you should. Smells smelly to me. As you suggest, if one likes combining the two, then ISTM while is the more natural choice of verb for one's chosen logic.

    But compared to the rest of this example, that's fly shit in the pepper.

    @Naomi: Thank you! Now I'm seeing the advantage to the combined mutate and test for reverse traversal = indexer decrement operations and why that's different for forward traversal = index increment operations.

    Namely that in any index-origin zero system (i.e. almost everything sane nowadays) the size() operator (by whatever name) returns a value beyond the last item's index. So you're stuck tucking an extra -1 in there somehow. This idiom sneaks the offsetting decrement into the test the first time though so you don't need to code around it in each iteration. And that issue doesn't occur in forward/incrementing traversal.

    AHA!!

    It's obvious once you point it out with good examples. And yes, given those other uglier ways of handling it, I'm a convert.

  • Mattie (unregistered)

    "Look more closely. Look for the curly brackets. Oh, you don’t see any?"

    I make this mistake fairly often now that python has wormed its way into my mix of programming languages.

  • (nodebb) in reply to Naomi

    Last one every time for me.

    It's not really an issue in Java or Javacript, but in C there is a type called size_t which is a type alias used to specify the size of an array and, as such, is guaranteed to be able to index any array. It's usually defined as unsigned int or unsigned long, the main thing being it is an unsigned integer. When I'm iterating an array, it's almost muscle memory to type

    for (size_t i = 0 ; i < array_size ; ++i)

    which means, when I need to go downwards, I type

    for (size_t i = array_size - 1 ; i >= 0  ; --i)

    Hmmmm, my unit test has been running for four hours. I must go and find out why it hasn't finished yet.

    Oh, and another thing. in a code review, I would reject anything that combined the test and the increment/decrement operator in a for loop.

  • (nodebb) in reply to WTFGuy

    And yes, given those other uglier ways of handling it

    Ugliness is in the eye of the beholder. To me, it's far uglier to deliberately put a side effect into something that's meant to be a boolean expression than to have to subtract 1 from a number.

  • Sole Purpose of Visit (unregistered)

    "Imagine you have some Java code which needs to take an array of integers and iterate across them in reverse, to concatenate a string. Oh, and you need to add one to each item as you do this."

    OK, I'm game. Generally, I think that offering a solution to a WTF is missing the point, but in this case it's worth it, I think. In C#:

    #include System.Linq; // etc

int [1000] input = Whatever();
var reverseInput = input.Reverse();
var incrementals = reverseInput.Select(item => (item + 1).ToString());
var retval = incrementals.Join(',', incrementals);

    I haen't tested this, but basically it's the pattern you want to use. No string builders. No counting to make sure you put the commas in the right places. No mutables. Just simple, understandable, iteration.

    Does it need to be in C#? Hell, no. I'm no language bigot. Python would be almost identical. Java would use Streams.

    Useless morons use for-loops. On occasion they come in handy. In this case, they don't. All I want for Christmas is for programmers to understand the concept of list operations.

  • Sole Purpose of Visit (unregistered) in reply to Sole Purpose of Visit

    Well, slightly buggered (no need to define the size of the array, and the Join is wrong), but really, if you can't write some piece of crap like I just did and then spend five minutes debugging it in an IDE -- you should be fired immediately.

  • (nodebb) in reply to Sole Purpose of Visit

    I haen't tested this, but basically it's the pattern you want to use. No string builders. No counting to make sure you put the commas in the right places. No mutables. Just simple, understandable, iteration.

    ... at the cost of copying the list around in memory three times, instead of once.

  • Chris (unregistered)

    I don't think the decrement as part of the condition clause is a huge sin, and can even make it more readable as shown above. But I agree with WTFGuy that in general we should use things the way they are intended to be used where possible. Being clever and using something just because it works is how a lot of bugs and unmaintainable code begin. A lot of problems are easily solved by using things the way they're intended.

  • (nodebb)

    Yup, proof that the JVM is truly "Universal"... I'm categorically stating beyond reasonable doubt that this is extra terrestrial code. Maybe that far-fetched virus from Independence Day is more plausible than initially thought.

  • (nodebb)

    Yup, proof that the JVM is truly "Universal"... I'm categorically stating beyond reasonable doubt that this is extra terrestrial code. Maybe that far-fetched virus from Independence Day is more plausible than initially thought.

  • (nodebb)

    Yup, proof that the JVM is truly "Universal"... I'm categorically stating beyond reasonable doubt that this is extra terrestrial code. Maybe that far-fetched virus from Independence Day is more plausible than initially thought.

  • (nodebb)

    Yup, proof that the JVM is truly "Universal"... I'm categorically stating beyond reasonable doubt that this is extra terrestrial code. Maybe that far-fetched virus from Independence Day is more plausible than initially thought.

    Addendum 2020-08-24 05:19: Uh I'm sorry, my browser had a bit of an aneurysm brought on by a dodgy internet connection. Please disregard the multiple posts.

  • (nodebb) in reply to Sole Purpose of Visit

    Useless morons use for-loops. On occasion they come in handy. In this case, they don't. All I want for Christmas is for programmers to understand the concept of list operations.

    One of our guys just came to me last week with a suggestion of going through an application and replacing all of the list based operations with for loops. He explained that the for loops executed in about one fiftieth of the time as the alternative.

    I looked into his claim. He showed me a benchmark he ran where a particular loop went from taking 156 ticks to taking 3 ticks - true to his claim. But, he didn't notice that this was an operation that runs once when someone clicks a button. So, although it may fifty times as fast, all the user will really notice is a 10 microsecond reduction in the response time of the application. He was unfazed and still recommended the change.

  • Chris (unregistered) in reply to Jaime

    Wow. And that's one scenario. There are others where the for loops are at best no better. E.g. the LINQ Except() function returns the items in one list that isn't in a second list, and is much more efficient than the obvious naive solution where you use nested loops. The obvious solution is O(NxM), for list lengths N and M, but the LINQ function is O(N+M), because it makes a hash set of the second list.

  • MiserableOldGit (unregistered)
    A lot of problems are easily solved by using things the way they're intended.

    Otherwise stated as being "really clever" is often a pretty dumb idea.

    That sort of thing usually acretes too ... someone uses the wrong approach, so it doesn't quite work properly, so a nifty workaround is added, which solves part of the problem and/or creates another unwanted side-effect, so more is added and so we go on until the festering unstable pile of poo is ready for release.

    I'm all for "clever" and "nifty", but when you find yourself revisiting the procedure because it does weird stuff you need to be honest with yourself that you weren't!

  • Craig (unregistered) in reply to stoborrobots

    Since it's Linq, it's probably not copying the list. Linq leans heavily on deferred execution, so the sequence isn't likely to be processed until the call to Join.

  • nasch (unregistered) in reply to stoborrobots

    "... at the cost of copying the list around in memory three times, instead of once."

    Even if it does, I wouldn't worry about it unless I saw some specific reason to. Like it is an array of binary file data, or it's intended to run on a severely constrained system. An array of numbers or smallish strings on a normal server? Not worth even spending the time to think about it, unless a performance issue is found.

Leave a comment on “A Backwards For”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article