- Feature Articles
- CodeSOD
- Error'd
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Imagine the possibilities with all the other potential things to test for:
Admin
It's probably the 9th (or so) version of this method, and the developers are scared to completely remove the logic (it MIGHT be needed in a future version, when allowSetAccount could be false!). But yes, as it is now, the whole method is entirely unnecessary. I suspect that the compiler might optimize it out of existence anyway.
Admin
Maybe I've worked on big systems like this too much, but this doesn't really seem all that bad to me.
Yes, there's the needless conditional chain of assignments of allowSetAccount to true which should be factored out, but if this method is really the worst this system has to offer...
Admin
No no no, this is just forward thinking. Sure the automation boolean is always true now, but what if that won't always be true in the future? The developer is just ahead of eveyone else of course!
Admin
you sure its Java? Java uses boolean, not "bool" and the naming conventions are slightly off (though given the application, this may be a red-herring). Methinks C# more likely?
Admin
Admin
There's 2 explanations for this.
1.) It's code leftover from a past version. Everyone updating it is scared to remove it because, if it was used in the past, it might be used again in the future.
2.) There's some design document out there that specifies possible future additions to the program that don't need to be implemented now, but it should be possible to add in the future. A programmer just decided to write the code in such a way as to make it very easy to add those 'features'
Anyway, should the compiler optimize that all out anyway?
Unnecessary? Absolutely. A WTF? meh, not really.
Addendum (2010-09-01 13:02): Edit: I actually meant to say "shouldn't the compiler optimize that all out anyway?" rather than "should"
Meant rhetorically rather than as an actual question.
Admin
Oh, I get it... decision making about whether the property can be mutated should have been delegated to a general-purpose ACL class, right?
Admin
Admin
hmm.
You obviously forgot:
Admin
if (have_something_to_say) { comment(); } else { comment(); }
Admin
Fixed?
Admin
And of course:
Admin
@Pigeon
Ok, I needed that today.
+1
Admin
That is assuming requisitionItem.IsAccountCodeNull() has no side effects (And the same for the various comparison operators - if they have been overloaded - in the original code)
Admin
Does it need checks for parameters being null?
Admin
The FILE_NOT_FOUND is out there....
Admin
Admin
Admin
Geesh. I always thought that one of the goals of a large class hierarchy was to reduce decision making.
Admin
And that is something which should never be discounted. There are good reasons to justify making the minimal changes required to achieve the goal. Without examining other code, it is impossible to tell if anything is "really" happening behind the scenes.
Also the compiler is unlikely to optimize this out of existance unless it too can evaluate all of the called code (and the code which that calls, and the code....)
Admin
Encountered an exception not handled by user code:
NullReferenceException: Object reference not set to an instance of an object
at SomeClass.SetAccount(RequisitionData.RequisitionItem, AccountData.Account)
Admin
Admin
Error: Boolean expected.
Admin
Admin
for ( int i = 0; i < 128; ++i ) { switch (i) { case 0: if ( true ) allowSetAccount = true; break;
....
}
Admin
Ever heard of optimization?
Admin
With regards to the source, did they look at the annotations/history in source control.
That would explain the history and thus perhaps why it is written that way, still unless IsAccountCodeNull() has side-effects, they certainly could reduce this logic.
Admin
Admin
I think this is just a simple bug, not a WTF. The first "else" should set to false, not true. It's perfectly legitimate to not allow an automated update process to blithely overwrite known good data with possibly bad data, though this is not the ideal way to prevent it. The sales reps in this company are probably still wondering why they're getting complaints from customers that packages are going to stale addresses; the customers are probably saying "you had it right for a while, why are you using my old address again??? This is the fourth time I've told you to change it!"
Admin
You don't think the sales reps are yelling "WTF?!" right now?
Admin
Indeed. I would go so far as having a look at the requirements.
Probably they were:
The accounts should be set:
Probably the analyst then expleted:
Then they should be set always!?
But the PHB replied:
No, no, look, the accounts should be set only:
and then added: "I'm a people person! I know how to deal with people! You take care of the computer!"
Admin
I did a test case for this. Using javac with -g:none, and running javap on the class file, even if all the branches return the same constant value, they're not optimized out.
Might try it with C in a little bit.
Admin
I wonder what their unit test looks like for this code?
Admin
Admin
x = false if (...) if (...) if (...) x = true else x = true else x = true else x = true
Admin
Nope.
Admin
x = false if (...) ...if (...) ......if (...) .........x = true ......else .........x = true ...else ......x = true else ...x = true
Admin
"A bug can't be a WTF?"
That's better. Continue.
Admin
Could this be machine-generated code??
dave
Admin
Hmm, I wonder about that.
It can't optimize out the call to isAccountCodeNull unless the optimizer is smart enough to figure out that this function has no side effects. In some cases that would be easy to figure out but in the general case? Wow.
Given that it must call that function, it must do the test on automation because it only calls isAccountCodeNull when automation is true.
As a human being looking at this code, I can easily see that the test on requisitionItem.AccountCode is superfluous, because I notice that the body of the IF is identical to the body of the ELSE. Are optimizing compilers smart enough to figure that out? Sure, in the simple case where the body is only one line that's easy. But what if the body is longer? How far does it check? Would it check for only character-for-character identical code or would it look for functionally identically, like "x++;" is the same as "x=x+1;", or calling a fuction with parameter "false" is the same as calling the function with a variable that has been set to false? Etc. I freely admit that I don't know if compilers are smart enough to do this sort of optimization. I'd be interested to know how far the state of the art on this has progressed.
So the only safe optimization that I see a compiler being likely to be able to figure out is to eliminate the "if (allowSetAccount)" around the last two statements. I can see that a compiler could figure out that the flag is always true by that point and thus eliminate the test.
I'd be interested to hear if someone who knows more about optimizing compilers can tell us just how much they can actually accomplish these days.
Admin
Then I have to ask; in a language where primitives are not allowed to be null, why are references allowed to be null ?
Admin
Admin
Not the tightest code ever, but if this is an example of the worst this code base has to offer, I'd say the OP should count his blessings.
Admin
Admin
E.g., IMO, "++x", "x++", "x=x+1", and "x=1+x" should all compile into "INC x", so they would all match.
Admin
Admin
I've had the honor of trying to maintain a 250k line FORTRAN77 codebase that was almost 30 years old. I came across several gems like this. These were almost always the result of new/changed requirements that were implemented without any thought of refactoring.
This from the same group of folks who routinely commented out code by wrapping it in a conditional that would always evaluate to false, with no documentation.
Admin
Admin
"If this statement was correct, the following would happen. This statement is not correct, therefore it will not."