The Daily WTF: Curious Perversions in Information Technology

2021-01-14 Reply Admin

Oh well. So now a WTF bar was lowered down to "a local scope variable shadows a variable from a parent scope"?

Mr. TA · 2021-01-14 Reply Admin

Whom are we kidding. They meant to search by some other filter value and totally messed it up. Because this code returns incorrect results, somewhere else there's a whole bunch of WTF bandaid code to work around the problem.

thosrtanner · 2021-01-14 Reply Admin

err. no it isn't that at all. This is a for loop which is coded so that it always drops out on the first iteration. Nothing to do with variable scope.

2021-01-14 Reply Admin

Another possibility - it's supposed to be comparing product id to the previous product id to find duplicates.

2021-01-14 Reply Admin

Ah. Found the developer guilty for this code. Also, what parent scope are you talking about? BOTH variables are declared inside the for-loop, with the second variable being absolutely useless. In fact, you could replace the entire code inside the for-loop with "if(true) break;". Do you now see why this is a WTF?

2021-01-14 Reply Admin

could replace the entire code inside the for-loop with "if(true) break;"

Correction:

     if(true) {
        productName = productList.get(i).getProductName();
        break;
    }

Now it's correct.

2021-01-14 Reply Admin

What they mean is that the currentProductID variable probably also exists outside of the code, e.g. as a parameter of the method inside which the code is. This is also my suspicion as to what happened here. If they meant the OTHER currentProductID when definind toMatchProductID, the loop wouldn't break on the first iteration.

Or, perhaps, the other variable was named something other than currentProductID but similar enough that the developer mixed them up when selecting an autocomplete suggestion.

DocMonster · 2021-01-14 Reply Admin

If I had to guess I would say that somebody used the wrong variable or change the name of a variable or something like that. Since it's reasonable if you were getting the current product ID outside of that loop.

2021-01-14 Reply Admin

What they mean is that the currentProductID variable probably also exists outside of the code, e.g. as a parameter of the method inside which the code is. This is also my suspicion as to what happened here. If they meant the OTHER currentProductID when definind toMatchProductID, the loop wouldn't break on the first iteration.

That still doesn't explain why that "second" toMatchProductID gets created in the first place. Without that variable, you might well be right. But that variable has no reason to be there at all, shadowing or no shadowing.

2021-01-14 Reply Admin

But that variable has no reason to be there at all, shadowing or no shadowing.

Exactly.

    for (int i = 0; i < productList.size(); i++) {
        if (currentProductID.equals(String.valueOf(productList.get(i).getProductId()))) {
            productName = productList.get(i).getProductName();
            break;
        }
    }

Don't know if this is "best practice" or not, I am not a Java Developer. But now we are getting somewhere at least. No need to create two string variables inside the loop. It may be beneficial for debugging purposes to create one variable inside the loop, but two just doesn't. Especially if it's just a copy of another one.

2021-01-14 Reply Admin

Another possibility - it's supposed to be comparing product id to the previous product id to find duplicates.

Yeah I reckon it was meant to do that or maybe originally did something like that and got "neutralised" by a developer who wanted the functionality off but couldn't be arsed to crawl through all the places it was called from and clean it up right.

Stupid, but dull.

2021-01-14 Reply Admin

The null check is reasonable (I'll assume this predates Java's Optional type), but the isEmpty check is arguably superfluous, since we enter a for-loop based on size()- an empty list would just bypass the for loop. Still, that's all harmless.

I see this one all over the place in my team's code as well, but it's not necessarily so harmless in C#, at least according to Resharper. Due to the way LINQ works, something like "if (foo != null && foo.Any()) { foo.Select(...) }" could potentially trigger multiple queries to the DB unless you've already enumerated your IEnumerable to a concrete type. At the very least, it leaves annoying squiggly lines all over the code.

But then, rather than fix the code design, people just jam ToList() calls everywhere to create lots of intermediate lists, reducing LINQ's ability to use deferred execution, which is one of its biggest benefits.

2021-01-14 Reply Admin

That still doesn't explain why that "second" toMatchProductID gets created in the first place.

People have different programming styles - some like to create lots of intermediate local variables that others would find unnecessary. Or maybe they expected the logic to calculate toMatchProductID to be more complicated and then didn't bother to clean up when it turned out not to be, who knows.

That's not really the point though - no-one's claiming this is good code (after all, it doesn't even work correctly), just that it's fairly easy to see how it (probably) came about, and it arguably isn't worthy of a front page article all by itself.

Mr. TA · 2021-01-14 Reply Admin

That is a bad reading of the situation. By definition, the filtering value, or a they call it, toMatchProductID, has to be defined outside of the loop. We should know what we're looking for before we start looking. And we shouldn't change what we're looking for as we're looking, either, because that creates a very likely scenario that we missed it previously. Even if they did assign some value to it which wasn't the current iterator value, it would still be a WTF.

Then they assign the current value to it, making the whole loop doubly stupid and pointless.

"People have different programming styles" - particularly the style of writing terrible code which can't work in any circumstances.

2021-01-14 Reply Admin

"People have different programming styles" - particularly the style of writing terrible code which can't work in any circumstances.

... and yet you insist that the same person who wrote that¹ couldn't possibly have just created a redundant local variable?

¹ More importantly, who didn't (properly or at all) test before committing - the mistake itself is fairly easy to make, as we've established, but it should have been caught pretty quickly.

dkf · 2021-01-14 Reply Admin

the style of writing terrible code which can't work in any circumstances.

There's also the style of writing terrible code that works, but ever… so… badly…

cellocgw · 2021-01-14 Reply Admin

Speaking of " isNull || isEmpty" code, let me poll the audience. Many languages, including MATLAB as an example, return an empty object when a search is empty. like

 foo = strfind(my_string, 'foo')

If the string is found, I get a number showing the position. If not found, I get nothing. Now, seeing as MATLAB is 1-indexed, I'd rather get a 0 (numeric) returned when the search finds nothing. That way I always have a numeric value to pass along tosomething else. But no, I get an empty object, which lots of functions will barf on, whereas these same function will read a zero and happily do nothing but not crash.

So which is preferred? empty or a numeric value?

2021-01-14 Reply Admin

Anyone want to guess if it's the outsourcing contract or their employee's quarterly goals that include "lines of code" somewhere that really shouldn't?

2021-01-14 Reply Admin

So which is preferred? empty or a numeric value?

After thinking about it for 2 seconds, I think I'll prefer a numeric value, as long as it means (and only means) "not found", and not "here, I return a 0 to you, which might be the index of what you were looking for, but it might also mean that there was an error in the function you just called, I don't know. KTHXBYE!" like in PHP.

Steve_The_Cynic · 2021-01-14 Reply Admin

Sort of. In PHP, it returns zero for "found at the beginning" and FALSE for "not found", but of course 0 == FALSE is true, so you end up having to use charming circumlocutions with === or you add a little string at the beginning and hope that it plus the real string don't unexpectedly ... well, an example is probably best:

   search_for = "foo"
   search_in = "oofoo"

   if find_substring("f" . search_in, search_for) == FALSE

This will fail because find_substring will return "found at the beginning" which is zero which is == FALSE, but the string really does contain "foo", and not at the beginning.

2021-01-14 Reply Admin

Well, we'd have to see the repository tree. But I doubt this is a case of shadowing (because, comparison variable inside loop).

There's always the possibility of terrible C&P, followed by senseless renaming and injection of a scoped variable, but my guess is:

foreach (var prod in XXX)
{
    var currentProdID = prod.ID;
    foreach (var prod2 in YYY)
    {
        if (prod2.ID == currentProdID)
            return currentProdID;
    }
}

// Otherwise, return some default.

Written in C# because I'm in the middle of a day's work and can't be bothered to translate into Java, but that might be where this started.

Then you try to remove the outer loop and find that the compiler complains that there is no currentProdID. So, what do you do?

You create one, inside the inner loop, which is now the only loop. Compiler stops complaining. Job done. (Very badly indeed.)

Mr. TA · 2021-01-14 Reply Admin

Redundant local variable? For what? Did you read the source code? What are you even talking about? I have the distinct impression that you're just trolling everybody.

2021-01-14 Reply Admin

... thereby breaking the whole functionality without even any diagnostic (assuming no shadowing warnings, which seems fair to assume).

By now we heard it probably wasn't a case of shadowing, but even if it was, this would seem pretty WTFy to me.

2021-01-14 Reply Admin

There was something else going on here, someone made a change to remove that part, and we got left with this. Seen it 100 times.

2021-01-14 Reply Admin

Redundant local variable? For what?

A redundant local variable, toMatchProductID, that was intended to contain (effectively) the same thing as another variable, currentProductID, that already existed outside the loop, except that due to the shadowing, it ended up containing the same thing as the currentProductID defined inside the loop instead.

Did you read the source code?

Did you read the discussion thread?

What are you even talking about?

OK, let's take this slowly. Some of us think that the code was probably supposed to look something like this:

public String getProductName(String currentProductID) {
    if (productList != null && !productList.isEmpty()) {
        for (int i = 0; i < productList.size(); i++) {
            String whatever = String.valueOf(productList.get(i).getProductId());
            String toMatchProductID = String.valueOf(currentProductID);
            if (whatever.equals(toMatchProductID)) {
                productName = productList.get(i).getProductName();
                break;
            }
        }
    }
}

Except the developer had a brain fart and called the "whatever" variable the same thing as the existing variable, thereby shadowing it and making toMatchProductID contain the wrong thing. Some of you think that's impossible, because why would toMatchProductID be defined inside the loop if it isn't supposed to vary between iterations? The answer to that is simply "bad programmers write bad code", and shuffling data between variables without actually doing much of anything is a pretty common form of that in my experience.

I have the distinct impression that you're just trolling everybody.

I have the distinct impression that your brain is a rock.

2021-01-14 Reply Admin

... thereby breaking the whole functionality without even any diagnostic (assuming no shadowing warnings, which seems fair to assume).
By now we heard it probably wasn't a case of shadowing, but even if it was, this would seem pretty WTFy to me.

Sure, but not really worth an article all by itself IMO (and IFrist'sO too apparently). Maybe if it had made it into production and somehow gone unnoticed for months that would justify it, but the article doesn't touch on that at all - it might just as easily have been caught in code review as soon as the outsourced developer delivered it.

Mr. TA · 2021-01-14 Reply Admin

OK I guess that's possible, but there's absolutely evidence that's what happened. If you go by the naming, current probably refers to iterator value. So your explanation is less likely than the more obvious one, the toMatch was supposed to be outside the loop and have some meaningful value, but got moved in for whatever reason.

In any case, we're taking shots in the dark here. That's why I was confused by your comment - this whole conversion is kind of pointless because we're way outside of having any reasonable way of knowing.

Please don't take it personally.

Addendum 2021-01-14 14:33: *no evidence

2021-01-14 Reply Admin

Could the intent be to find the first product ID that does not have a leading zero? Then the only WTF is how they are checking for a leading zero.

2021-01-14 Reply Admin

If you go by the naming, current probably refers to iterator value.

True - I'm starting to think that the "outer" currentProductID is more likely to have been from some other loop rather than a function parameter.

In any case, we're taking shots in the dark here. That's why I was confused by your comment - this whole conversion is kind of pointless because we're way outside of having any reasonable way of knowing.

That's fair. The shadowing explanation seems more likely to me (the smallest distance between a kinda-sloppy-but-working version and what we have), but only the submitter can really know.

Please don't take it personally.

:-)

Mr. TA · 2021-01-14 Reply Admin

Now that I think about it, the outer loop idea sounds plausible. Alas, some things are destined to stay in the dark forever.

2021-01-14 Reply Admin

My guess is that toMatchProductID was supposed to be defined outside, probably an argument passed to the function/method. Then someone mistrusted the string matching and introduced the clone of currentProductId as a quick hack to verify it. It works, indeed, – let's call it a day!

Bonus points, if the outside toMatchProductID was discarded later, as it was clearly redundant (as proven by tests).

2021-01-14 Reply Admin

Find a fellow programmer of computers. Publish his mistakes. Laugh behind his back. What? You think people can be evil or something?

2021-01-15 Reply Admin

When I make a mistake I admit it, apologise for it, and move on. What, pray, do you do, sir?

2021-01-15 Reply Admin

Mockery is an essential part of learning and therefore this site represents a vital comunity service. If you are newer mocked, newer spat on, if you newer have your code reviewed and told every single thing you got wrong in a way that makes you ashamed for doing so than how will you ever stop making those mistakes? You won't. You'll just continue living your happy life thinking you are the best software engineer in the world when in reality you have trouble scripting up a HTML only website.

Or, as we old folk like to say. Spare the rod, spoil the child.

2021-01-15 Reply Admin

In PHP, it returns zero for "found at the beginning" and FALSE for "not found", but of course 0 == FALSE is true,

You are right, of course. I got confused by the 0 == FALSE bit when I wrote the comment.

2021-01-15 Reply Admin

Or, we can use those example to question our own code and avoid mistakes that other people made.

Watson · 2021-01-15 Reply Admin

PHP now has a shiny new function to tell if one string is contained in another. It doesn't tell you where in the string it is — it's a straight boolean — but if all you care is whether it's there or not it'll do the job. If you do want the position you're back to 0 == false !== 0.

2021-01-15 Reply Admin

Those of us trained and an experienced in engineering are taught (and understand) early on that there is far more to be learned from dissecting and examining failure than simply adoring the lack of it.

Yeah, this place can be a bit sarcastic, but we are as much for slinging mud at each other for misunderstanding constraints/mistakes than running down whatever idiot creates the stuff that generates the articles. It's called being grown up about it.

There's nothing more dangerous than a coder who thinks he does everything perfectly and then gets all defensive when questioned, or worse still hides his mistakes to maintain the illusion.

2021-01-18 Reply Admin

I feel like I've seen this or something similar, at a large printing/paper/phonebook company...

Jeremy Pereira · 2021-01-25 Reply Admin

That still doesn't explain why that "second" toMatchProductID gets created in the first place. Without that variable, you might well be right. But that variable has no reason to be there at all, shadowing or no shadowing.

There's nothing in the code that tells you that a product ID is a string. If it's a class that doesn't override equals() but does override toString() sensibly, this would be a way to compare product ids.

A Match Made In…

Leave a comment on “A Match Made In…”