Thursday, September 17, 2009

Better late then never



I won't tell you where I found it but it has been there for almost 6 years ;) Your day has come!

11 comments:

Tiran Kenja said...

Hehe, nice! Considering the option it could be null but not making sure it don't provoke a NPE.

Well played ;)

Scott Rosenbaum said...

Maybe I am just dumb, but wouldn't that code throw an NPE if config was NULL? Seems like the null test needs to sit in front of the assignment to fName.

Maarten Meijer said...

Wouldn't an ordinary check using FindBugs have spotted that immediately?

Joerg said...

ROFL

Jilles said...

There are several static code analyzers that catch this type of bugs. The findbugs people make the point regularly that their tool finds loads of issues in high profile OSS projects. Like Eclipse JDT.

I consider it downright irresponsible to not use findbugs on a Java project. Over the years it has helped me find unclosed streams, bad if conditions, potential (and real) deadlocks, flaky synchronization, npe situations, unnecessary checks, dead code, etc. It's totally unobtrusive when you have it integrated in your eclipse project to run on save.

Anonymous said...

well, I´m quite new to software development, but what´s the problem with this code??

Miles Parker said...

Anonymous said...
"well, I´m quite new to software development, but what´s the problem with this code??"

Hey, I think there's a job waiting for you at Microsoft. Kidding!!

Anonymous said...

Thanks heaps Miles, why don't you bag someone new out AND give no answer.

There's a job waiting for you at... Hmmm. A phone company call centre???

IBBoard said...

Anon, check which variable the null check is against and then think of what the line before does. Hint: Scott already said it ;)

Jesper said...

@Anonymous:

You need to play it out in your head like this: 1) What if config is null at the start of the code. 2) What if it is non-null?

Result:
1) If config is null, then the 'if' statement is never reached, since calling a method on a null reference throws an exception.

2) If config is not null then the
'if'-branch is not taken.

So, the code inside the if branch is "dead", meaning that it will never be executed.

The author likely intended
A) to check 'config' before calling (in which case the call should be moved inside the 'if'-branch), or
B) to check the name returned by config.getAttribute, in which case the 'config == null' should be changed into 'fName == null'.

And yes, FindBugs and similar tools will help find these kinds of bugs: But always take care to try and understand the warnings the tool provides, don't just fix it - that's how you learn from it.

We've all been beginners to programming once, but hiding behind 'Anonymous' will make you a more likely target for bullying -- it's much easier to taunt somebody you don't know and who doesn't want to be seen.

FWIW: I've been a programmer for more than half my life and I didn't spot the bug at first: I noticed the missing localisation of "Unknown" (most of Eclipse use language files and has no user-visible text inside the Java classes themselves), and only on reviewing it a second time spotted the dead code.

pookzilla said...

That looks like the kind of bullshit I'd write before 9am (when the coffee kicks in).