Assertiveness

maja

Check this out:

public LogRecorder startLogRecording() {

if( recorder==null)

recorder = new LogRecorder();

else

assert recorder==null : "recorder was already started...";

return recorder;

}

(The space after the opening brace but not before the closing brace: sic!)

BTW, this is only used in a test.

Ben L.

Is the WTF that they wanted to be more verbose than assert false?

maja

Thanks for asking :-) The code is equivalent to:

public LogRecorder startLogRecording() {
     assert recorder==null : "recorder was already started...";
     recorder = new LogRecorder();
     return recorder;
}

That's if you think that asserts are useful at all. In this case I must concede that the above is shorter than an "if ... else throw new IllegalStateException(...)".

Granted, the duplicated check is not expensive, and it's not in a place where performance counts, but it hides the intent of the code, even though it might look as if it were emphasizing it. It took me a while to find out that the if is superfluous here. I came to call this "smoke and mirrors coding."

Also: checking such state information is a smell that starts me looking for the real condition. For example, it may be possible to instantiate the LogRecorder in the constructor of the using class, which would remove this method - and the possibility of an inconsistent state - entirely. That's what I came to call "making more feet so it's easier to hit."

Alas, that is not exactly the case here, because then the LogRecorder would be instantiated in production, when only being used in testing. Code that is only needed for testing but goes into production: Not a WTF as such, but another smell.

Ben L.

Your solution clobbers recorder when assertions are turned off. Also, I seriously hope this isn't used with threads.

Severity_One

@maja said:

The code is equivalent to:

public LogRecorder startLogRecording() {
     assert recorder==null : "recorder was already started...";
     recorder = new LogRecorder();
     return recorder;
}

No it isn't.

Barbaz

@maja said:

Thanks for asking :-) The code is equivalent to:

public LogRecorder startLogRecording() {
     assert recorder==null : "recorder was already started...";
     recorder = new LogRecorder();
     return recorder;
}

No, it's not. Assertions can be disabled, in which case the code is

public LogRecorder startLogRecording() {
     if (recorder == null) recorder = new LogRecorder();
     return recorder;
}

MiffTheFox

@maja said:

assert recorder==null : "recorder was already started...";

What the hell language is this. It looks like C#/Java if not for that strange assert statement.

I thought the same thing at first.
Turns out, they are valid Java assertions.

The second form of the assertion statement is:
assert Expression1 : Expression2 ;

eViLegion

The lesson here, is that you should be robustly deriving new code from old when you refactor,
or you're going to make statements about equivalence that make everyone here laugh at you.

And as we all know, nothing in the world is worse than being laughed at by TDWTF Side Barrers.
Except red trousers... those are worse.

dhromed

@eViLegion said:

Except red trousers... those are worse.

Cyan trousers.