Comment On Accounting for Complexity

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.

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...

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!

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?

the beauty of it is that the method is private within the class, and has an 'automation' boolean parameter

No, the beauty of it is that the 'allowSetAccount' variable is always set true should 'automation' be true (which, according to TFA, it always is)

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.

Oh, I get it... decision making about whether the property can be mutated should have been delegated to a general-purpose ACL class, right?

hatterson:

Anyway, should the compiler optimize that all out anyway?

Any decent optimizing compiler would.

snoofle:

Imagine the possibilities with all the other potential things to test for:

if (FILE_NOT_FOUND) 

   allowSetAccount = true;

else if (System.currentTimeMillis() > 12345)

   allowSetAccount = true;

else if (System.getProperties().getProperty("os.name").equals("Windows")

   allowSetAccount = true;

else if (!System.getProperties().getProperty("os.name").equals("Windows")

   allowSetAccount = true;

else if (42)

   allowSetAccount = true;

// This could go on for a while...

hmm.

You obviously forgot:

...

if (allowSetAccount)

    allowSetAccount = true;

else

    allowSetAccount = true;

...

if (have_something_to_say) {
comment();
} else {
comment();
}

Fixed?

private void SetAccount(RequisitionData.RequisitionItem requisitionItem, 

                        AccountData.Account account)

{

	requisitionItem.AccountID = account.ID;

        requisitionItem.AccountCode = account.Code;

}

Pigeon:

snoofle:

Imagine the possibilities with all the other potential things to test for:

if (FILE_NOT_FOUND) 

   allowSetAccount = true;

else if (System.currentTimeMillis() > 12345)

   allowSetAccount = true;

else if (System.getProperties().getProperty("os.name").equals("Windows")

   allowSetAccount = true;

else if (!System.getProperties().getProperty("os.name").equals("Windows")

   allowSetAccount = true;

else if (42)

   allowSetAccount = true;

// This could go on for a while...

hmm.

You obviously forgot:

...

if (allowSetAccount)

    allowSetAccount = true;

else

    allowSetAccount = true;

...

And of course:

if (Paula.isBrillant())

    allowSetAccount = true;

else // Unreachable code path

    allowSetAccount = true;

@Pigeon

Ok, I needed that today.

+1

The Nerve:

Fixed?

private void SetAccount(RequisitionData.RequisitionItem requisitionItem, 

                        AccountData.Account account)

{

	requisitionItem.AccountID = account.ID;

        requisitionItem.AccountCode = account.Code;

}

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)

The Nerve:

Fixed?

private void SetAccount(RequisitionData.RequisitionItem requisitionItem, 

                        AccountData.Account account)

{

	requisitionItem.AccountID = account.ID;

        requisitionItem.AccountCode = account.Code;

}

Does it need checks for parameters being null?

The FILE_NOT_FOUND is out there....

Buddy:

The Nerve:

Fixed?

private void SetAccount(RequisitionData.RequisitionItem requisitionItem, 

                        AccountData.Account account)

{

	requisitionItem.AccountID = account.ID;

        requisitionItem.AccountCode = account.Code;

}

Does it need checks for parameters being null?

It should be:

private void SetAccount(RequisitionData.RequisitionItem requisitionItem, 

                        AccountData.Account account)

{

    if (account != null

	requisitionItem.AccountID = account.ID;

        requisitionItem.AccountCode = account.Code;

    }

}

The other check for null is effectively ignored (barring side effects, operator overloading, lack of file system, etc.).

NullPointerException:

Buddy:

The Nerve:

Fixed?

private void SetAccount(RequisitionData.RequisitionItem requisitionItem, 

                        AccountData.Account account)

{

	requisitionItem.AccountID = account.ID;

        requisitionItem.AccountCode = account.Code;

}

Does it need checks for parameters being null?

It should be:

private void SetAccount(RequisitionData.RequisitionItem requisitionItem, 

                        AccountData.Account account)

{

    if (account != null) {

	requisitionItem.AccountID = account.ID;

        requisitionItem.AccountCode = account.Code;

    }

}

The other check for null is effectively ignored (barring side effects, operator overloading, lack of file system, etc.).

-_-

Geesh. I always thought that one of the goals of a large class hierarchy was to reduce decision making.

....barring side effects, operator overloading...

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....)

The Nerve:

Fixed?

private void SetAccount(RequisitionData.RequisitionItem requisitionItem, 

                        AccountData.Account account)

{

	requisitionItem.AccountID = account.ID;

        requisitionItem.AccountCode = account.Code;

}

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)

vcx:

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?

Arr! I love sea-based languages.

snoofle:

else if (42)
allowSetAccount = true;
// This could go on for a while...
[/code]

Error: Boolean expected.

vcx:

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?

Nobody said it was Java. It's obviously C#.

snoofle:

Imagine the possibilities with all the other potential things to test for:

if (FILE_NOT_FOUND) 

   allowSetAccount = true;

else if (System.currentTimeMillis() > 12345)

   allowSetAccount = true;

else if (System.getProperties().getProperty("os.name").equals("Windows")

   allowSetAccount = true;

else if (!System.getProperties().getProperty("os.name").equals("Windows")

   allowSetAccount = true;

else if (42)

   allowSetAccount = true;

// This could go on for a while...

for ( int i = 0; i < 128; ++i )
{
switch (i)
{
case 0:
if ( true )
allowSetAccount = true;
break;

....

case 127:
if ( true )
allowSetAccount = true;
break;

default:
if ( true )
allowSetAccount = true;
}
}

Pigeon:

hmm.

You obviously forgot:

...

if (allowSetAccount)

    allowSetAccount = true;

else

    allowSetAccount = true;

...

Ever heard of optimization?

if (!allowSetAccount)

    allowSetAccount = !allowSetAccount;

else

    allowSetAccount = allowSetAccount;

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.

hatterson:

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.

Those 2 explanations are certainly plausible. But based on code I've inherited in my career, as well as so many snippets we've seen on this site in the last 6 years, I think a more likely explanation is that the original programmer's brain was taking annual leave that day.

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!"

FuBar:

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!"

A bug isn't a WTF?

You don't think the sales reps are yelling "WTF?!" right now?

Indeed. I would go so far as having a look at the requirements.

Probably they were:

The accounts should be set:
- when automation is disabled,
- when automation is enabled and:
- the account code is null or the empty string,
- or when there is an account code.

Probably the analyst then expleted:

Then they should be set always!?

But the PHB replied:

No, no, look, the accounts should be set only:
- when automation is disabled,
- when automation is enabled and:
- the account code is null or the empty string,
- or when there is an account code.

and then added: "I'm a people person! I know how to deal with people! You take care of the computer!"

NullPointerException:

hatterson:

Anyway, should the compiler optimize that all out anyway?

Any decent optimizing compiler would.

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.

I wonder what their unit test looks like for this code?

Ben:

NullPointerException:

hatterson:

Anyway, should the compiler optimize that all out anyway?

Any decent optimizing compiler would.

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.

Liar. This isn't valid Java code.

x = false
if (...)
if (...)
if (...)
x = true
else
x = true
else
x = true
else
x = true

boog:

A bug isn't a WTF?

Nope.

x = false
if (...)
...if (...)
......if (...)
.........x = true
......else
.........x = true
...else
......x = true
else
...x = true

wtf:

boog:

A bug isn't a WTF?

Nope.

I should rephrase.

"A bug can't be a WTF?"

That's better. Continue.

Could this be machine-generated code??

dave

NullPointerException:

hatterson:

Anyway, should the compiler optimize that all out anyway?

Any decent optimizing compiler would.

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.

> As for what should be done when you try to use a reference to null;

Then I have to ask; in a language where primitives are not allowed to be null, why are references allowed to be null ?

DaveAronson:

if (have_something_to_say) {
comment();
} else {
comment();
}

mightLaugh = false;

if( !comment.referencesBertGlanston() {

  mightLaugh = true;

} else {

//  mightLaugh = true;

}

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.

TheCPUWizard:

....barring side effects, operator overloading...

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.

What??! I thought the glorious promise of the much touted object oriented paradigm was that other code can't mess with my stuff.

Jay:

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.

Not being a knowledgeable person about these stuff, but i would imagine that if the "bytecode" (IL code, ASM instructions, or whatever's the compiler's output) match, no branching is needed. ;-) Note, that's not saying that the condition should be left out entirely, just that it's evaluated (for possible side effects), and only its result is discarded. The optimization would save the jump (infinitesimal speed increase) and the redundant code (smaller code size).

E.g., IMO, "++x", "x++", "x=x+1", and "x=1+x" should all compile into "INC x", so they would all match.

Mike:

DaveAronson:

if (have_something_to_say) {
comment();
} else {
comment();
}

mightLaugh = false;

if( !comment.referencesBertGlanston() {

  mightLaugh = true;

} else {

//  mightLaugh = true;

}

In other words, you wouldn't even laugh at your own comment. Besides, I haven't seen a Bert Glanston comment yet this week.

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.

bene:

> As for what should be done when you try to use a reference to null;

Then I have to ask; in a language where primitives are not allowed to be null, why are references allowed to be null ?

because you might actually want to represent something that has no value (member not initialized or member explicitly unset). That's one thing I found annoying when working in a c# application that we have is that Dates are value types and can't be set to null, so if there is no maturity date on the transaction then you have to set it to a magic value which means null. Either that or you create your own NullableDate as a reference type just so that you can set it null.
Then there's also the trouble with value types in data sets - if you access a value that is unset you get an exception, so you have to wrap such access in:

if(!dataSetRow.isValueNull())

{

//use dataSetRow.Value

}

xP

SwampJedi:

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.

That's what you get when you have scientists coding.

"If this statement was correct, the following would happen. This statement is not correct, therefore it will not."