setDirty( true )

  • ... 2006-11-29 00:21
    <p>wow, good thing that isn&#39;t in the military!</p>
  • felix 2006-11-29 00:25
    <p>I&#39;m thinking and thinking and can&#39;t figure out the logic in that piece of code. Am I alone in this?</p>
  • ammoQ 2006-11-29 00:47
    [quote user=&quot;felix&quot;]<p>I&#39;m thinking and thinking and can&#39;t figure out the logic in that piece of code. Am I alone in this?</p><p>[/quote]</p><p>This method sets isRecordLocked; if the new value is different from the old value, it also sets isDirty to true to mark the object as &quot;dirty&quot;.<br /></p>
  • Some Guy 2006-11-29 00:54
    I think this was machine generated code.
  • jaspax 2006-11-29 01:15
    <p>Perhaps I&#39;m dumb, but I can&#39;t find a way to put this into a very succinct one-liner. If you don&#39;t need to worry about isRecordLocked being null, then you can just do:</p><p>&nbsp;
    if (!isRecordLocked.equals(newValue)) context.setDirty(true); 
    </p><p>There&#39;s various ways to handle the incoming value that ensure that it never generates a NullPointerException, but none of them are necessarily more obvious than what&#39;s given here.<br /></p><p>&nbsp;If it were up to me, I&#39;d just change Boolean to boolean and write &quot;if (isRecordLocked != new Value) { ... }&quot;.&nbsp; It would take about ten minutes with the &quot;Refactor&quot; function in Eclipse.<br /></p>
  • KNY 2006-11-29 01:17
    <p>[quote user=&quot;Anonymous&quot;]I think this was machine generated code.[/quote]</p><p>Machines everywhere are insulted by this post.&nbsp;</p>
  • whitty 2006-11-29 01:28
    public void setIsRecordLocked(Boolean newValue) {<br />&nbsp;&nbsp;&nbsp; if (isRecordLocked != newValue)<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; context.setDirty(true);<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />&nbsp;&nbsp;&nbsp; }<br />}<br /><br />The code isn&#39;t that bad in the scheme of WTFs.&nbsp; At least it explicitly covers the right bases.<br /><br />Although why a language would need the Boolean class which is essentially a tri-state (true, false, null) rather than a native bool is beyond me.<br />
  • Divy 2006-11-29 01:36
    <p>[quote user=&quot;Anonymous&quot;]public void setIsRecordLocked(Boolean newValue) {<br />&nbsp;&nbsp;&nbsp; if (isRecordLocked != newValue)<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; context.setDirty(true);<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />&nbsp;&nbsp;&nbsp; }<br />}<br /><br />[/quote]</p><p>&nbsp;</p><p>what abt just </p><p>&nbsp;context.setDirty(true);<br />&nbsp;isRecordLocked = newValue;</p><p>&nbsp;</p><p>assuming the assumption that a null can&#39;t be equal to non-null is true :)<br />&nbsp;</p>
  • Erwan 2006-11-29 01:44
    [quote user=&quot;jaspax&quot;]<p>Perhaps I&#39;m dumb, but I can&#39;t find a way to put this into a very succinct one-liner. If you don&#39;t need to worry about isRecordLocked being null, then you can just do:</p><p>&nbsp;
    if (!isRecordLocked.equals(newValue)) context.setDirty(true); 
    </p><p>There&#39;s various ways to handle the incoming value that ensure that it never generates a NullPointerException, but none of them are necessarily more obvious than what&#39;s given here.<br /></p><p>&nbsp;If it were up to me, I&#39;d just change Boolean to boolean and write &quot;if (isRecordLocked != new Value) { ... }&quot;.&nbsp; It would take about ten minutes with the &quot;Refactor&quot; function in Eclipse.<br /></p><p>[/quote]</p><p>&nbsp;</p><p>You would then loose a big feature of this code : it uses 3-valued booleans (true, false and null).</p><br />
  • Mr Unanimous 2006-11-29 01:44
    <p>This should do the same thing, right? The potential for changing isRecordLocked to null seems bad, but who knows, maybe it&#39;s supposed to for some reason. :)</p><p>What&#39;s the difference between boolean and Boolean?</p><p>&nbsp;</p><p>public void setIsRecordLocked(Boolean newValue) {<br />&nbsp;&nbsp;&nbsp; // Update state if required.<br />&nbsp;&nbsp;&nbsp; if (isRecordLocked != null &amp;&amp; newValue != null &amp;&amp; !isRecordLocked.equals(newValue)) {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; context.setDirty(true);<br />&nbsp;&nbsp;&nbsp; }<br /><br />&nbsp;&nbsp;&nbsp; // Change the value.<br />&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />}<br />&nbsp;</p>
  • bruzie 2006-11-29 01:49
    <p>Having done several contracts for different NZ Government departments, I can make a reasonable guess of the companies involved. It&#39;s probably one of these:</p><ul><li>Inland Revenue/EDS</li><li>Accident Compensation Corporation/Unisys</li></ul><p>My experiences when working at government department clients have always been consistent when dealing with their outsourced IT partners - they&#39;ve been atrocious. Five days to get a log on, one day to get a password reset (what am I supposed to do in the meantime?). Unisys is worse - we had a Java enterprisey project and the Unisys Java Team Leader (Australian) was supposed to give us the EJB to work from - but went skiing instead, then was caught out in several lies trying to cover it up.</p><p>Grrrr.<br /></p>
  • lplatypus 2006-11-29 01:55
    <p>Anonymous wrote:</p><p style="margin-left: 40px">if (isRecordLocked != newValue)</p><p>No! In Java, Boolean is a class, not a simple type like bool, so:</p><ol><li>The != operator tests whether the two object references refer to the same object; it does not check whether the two object references refer to objects with the same value.</li><li>Object references might be null, which you don&#39;t account for.</li></ol><p>Someone has already beaten me to suggest the following replacement:<br /></p><p style="margin-left: 40px">if (isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))</p><p>&nbsp;</p>
  • Steve 2006-11-29 02:19
    <p>&gt;&gt; if (isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))</p><p>Is this right?&nbsp; I thought boolean short-circuiting will mean this is totally different from the original.&nbsp;&nbsp; For clarity (not brevity) I would be tempted to transfer the nulls to false&#39;s via a temporary to make the logic more obvious.&nbsp;&nbsp; Yes it&#39;s a waste.<br /></p><p>&nbsp;</p><p>&nbsp;</p>
  • Jon Haugsand 2006-11-29 02:36
    [quote user=&quot;Anonymous&quot;]<p style="margin-left: 40px">if (isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))</p><p>&nbsp;[/quote]</p><p>&nbsp;Really?&nbsp; I cannot get this to work.&nbsp; E.g. if both are null, should the dirty flag really be set? <br />&nbsp;What we need for clearity, is a small abstraction, something that should exist in the Object class:</p><p>&nbsp;public static boolean equals(Object o1, Object o2) {<br />&nbsp; return (o1==null ? o2==null: o1.equals(o2));<br />}</p><p>&nbsp;</p><p>Then we just need to say:</p><p>&nbsp;&nbsp; if (! Object.equals(isRecordLocked, newValue) ) context.setDirty(true);</p><p>&nbsp;</p><p>&nbsp;</p>
  • Jamie 2006-11-29 02:39
    <p>NoooooooOOOOooooooooooooOoooooooooooooooooOOOOOOOOooooOOOOOOOOoooOOOOOOOO!</p><p>&nbsp;</p><p>I thought maybe, just maybe, New Zealand might be some kind of safe haven from WTFs (this is the first one I have seen on here). *sigh* I could see it being IRD, or maybe studylink. All our government has done with new technology is move the queues from physical locations to phones and the internet. It still takes weeks to get ANYTHING done. Knowledge wave my ass.</p><p>&nbsp;</p><p>I need to start preparing myself for crud like this I suppose. &nbsp;</p>
  • Steve 2006-11-29 02:41
    <p>Actually I just figured, that that&#39;s wrong too because in the case of isLocked==false and newValue==null, which would result in setDirty(true), my new code wouldn&#39;t work.</p><p>Unless I&#39;m missing something fundamental there is no easy way to do this to get the same answer as the original unless Boolean.valueOf() was used to create the booleans.&nbsp;&nbsp; In which case the newValue == isLocked can be used because all the TRUE/FALSE values will be the same object.&nbsp;&nbsp; I&#39;m actually starting to prefer the WTF solution ...<br />&nbsp;</p>
  • AaronStJ 2006-11-29 02:51
    <p>
    public void setIsRecordLocked(Boolean newValue) {</p><p>&nbsp;&nbsp;&nbsp; context.setDirty(isRecordLocked != newValue);</p><p>&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;&nbsp;</p><p>}&nbsp;</p>
  • Goplat 2006-11-29 02:58
    <p>If you compare Boolean (or any reference type, actually) with != you are comparing the references, not the object contents like the original function does.<br /></p><p>This is a great opportunity to use the trinary conditional operator :) Here&#39;s the simplest equivalent code I can come up with:<br /></p><p>if (isRecordLocked == null ? newValue != null : !isRecordLocked.equals(newValue))<br />&nbsp;&nbsp;&nbsp; context.setDirty(true);<br /><br />It appears that .equals(null) always returns false on a Boolean, so I removed the redundant newValue == null check.<br /></p>
  • mcosta 2006-11-29 03:01
    All you are missing a thing. If both are not null and different two calls to setDirty() are made. That's why he says " Do not try to use this code as a pick up line".
  • TK 2006-11-29 03:04
    <p>I think the original WTF is fairly good code. All the the versions suggested in above comments a wrong in some way or other. Using Boolean instead of Java&#39;s built in type boolean creates this kind of hoop to jump through. </p><p>There are advantages of using Boolean, for example you can store them in data structures that take generic &quot;Object&quot;. Or they use it just for the enterpriseyness and to please some OO fanatic. I wonder if they use Integer instead of int too...<br /></p>
  • Jon Haugsand 2006-11-29 03:05
    [quote user=&quot;Anonymous&quot;]<p>
    public void setIsRecordLocked(Boolean newValue) {</p><p>&nbsp;&nbsp;&nbsp; context.setDirty(isRecordLocked != newValue);</p><p>&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;&nbsp;</p><p>}&nbsp;</p><p>[/quote]</p><p>&nbsp;Which won&#39;t work for two reasons.&nbsp; First is that you should not reset the dirty flag if it is already set to true.&nbsp; The second is that isRecordLocked is always different from newValue if one of them is generated by using &quot;new Boolean(whatever)&quot;.</p><p>&nbsp;<br />The real problem with the code is actually not the incomprehensive code, but the three value logic of it.&nbsp; Assuming here that there is no real semantic of a Boolean null, the programmer should assume that null is not possible, and use aspect oriented programming or some plain assert statements in the beginning of the function:</p><p>&nbsp;<br />assert(newValue != null &amp;&amp; isRecordLocked != null);<br />if (! newValue.equals(isRecordLocked)) context.setDirty(true);</p><p>Even better is perhaps to assume that both isRecordNull and newValue is made through Boolean.valueOf or Boolean.{TRUE,FALSE}:</p><p>&nbsp;</p><p>assert(newValue==Boolean.TRUE || newValue==Boolean.FALSE);</p><p>assert(isRecordLocked==Boolean.TRUE||isRecordLocked==Boolean.FALSE);</p><p>if (newValue != isRecordLocked) context.setDirty(true);</p><p>&nbsp;</p><p>This, however, would probably break &quot;working&quot; code.</p><p><br /><br />&nbsp;</p>
  • TK 2006-11-29 03:09
    <p>[quote user=&quot;Anonymous&quot;]All you are missing a thing. If both are not null and different two calls to setDirty() are made. That&#39;s why he says &quot; Do not try to use this code as a pick up line&quot;.[/quote]</p><p>No, the second one is else if.&nbsp;</p>
  • Smurf 2006-11-29 03:10
    [quote user=&quot;Anonymous&quot;]
    <p>
    public void setIsRecordLocked(Boolean newValue) {</p>
    <p>&nbsp;&nbsp;&nbsp; context.setDirty(isRecordLocked != newValue);</p>
    <p>&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;&nbsp;</p>
    <p>}&nbsp;</p>
    <p>[/quote]</p>
    <p>Umm, no. The assumption is that these are objects and that you need to call .equals to find out if the content is the same, instead of != which just checks if it&#39;s the same object. You might also be unable to pass a NULL object to .equals().<br />
    </p>
    <p>If that is so, then the original code is in fact correct. An equivalent one-liner test might be</p>
    <p><code>if (old ? new ? !old.equals(new) : TRUE : new != NULL)</code></p>
    <p>which is somewhat more readable.&nbsp;</p>
  • Einsidler 2006-11-29 03:21
    <p>[quote user=&quot;Anonymous&quot;]public void setIsRecordLocked(Boolean newValue) {<br />&nbsp;&nbsp;&nbsp; if (isRecordLocked != newValue)<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; context.setDirty(true);<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />&nbsp;&nbsp;&nbsp; }<br />}<br /><br />The code isn&#39;t that bad in the scheme of WTFs.&nbsp; At least it explicitly covers the right bases.<br /><br />Although why a language would need the Boolean class which is essentially a tri-state (true, false, null) rather than a native bool is beyond me.<br />[/quote]</p><p>Maybe they wanted to add a FileNotFound state<br /> </p>
  • berrs 2006-11-29 03:23
    <p>Maybe it&#39;s because I just woke up, but I cannot get the WTF in this. Ok, it takes a while to grasp but I see nothing wrong. If the new value is different from the old value, setDirty(true). So? But I would like to see that one-liner.<br /></p><p>&nbsp;</p>
  • Remco Gerlich 2006-11-29 03:55
    <p>Let&#39;s give it a try too...</p><p>&nbsp;</p><p>public void setIsRecordLocked(Boolean newValue) {</p><p>&nbsp;&nbsp;&nbsp; if (newValue == null ? (isRecordLocked != null) : !newValue.equals(isRecordLocked)) {</p><p>&nbsp;&nbsp; &nbsp;&nbsp; setDirty(true);</p><p>&nbsp;&nbsp; }<br />&nbsp;</p><p>&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;&nbsp;</p><p>} </p><p>&nbsp;</p><p>But there&#39;s not much wrong with the original. I much prefer the static Object.equals that someone proposed, given that that doesn&#39;t exist, you need to have it in a library in your project somewhere. It&#39;s the only readable way.<br />&nbsp;</p>
  • jesperdj 2006-11-29 03:56
    <p>Hmmm. This is not really a very spectacular WTF. I wouldn&#39;t say &quot;WTF?!&quot; at all if I&#39;d find something like this.<br /></p><p>[quote user=&quot;Anonymous&quot;]</p><p>&nbsp;what abt just </p><p>&nbsp;context.setDirty(true);<br />&nbsp;isRecordLocked = newValue;</p><p>[/quote]</p><p>That&#39;s wrong because setDirty() is not called in the original code when both isRecordLocked and isDirty are null. So your code does something different than the original code.<br /></p>
  • dnnrly 2006-11-29 04:07
    <p>The WTF is that it looks wrong, but when you get down to it I reckon it&#39;s right. It&#39;s just one of those times where you have to step away from the code and let someone smarter than you fiddle with it.</p><p>As far as I can see, it checks that neither of the Boolean objects are null before comparing the values and also comparing the &#39;nullness&#39; too. I wouldn&#39;t like to fiddle with this until I knew what context.setDirty(...) did.<br /></p>
  • Ottokar 2006-11-29 04:08
    Hello,

    a tristate Boolean makes sense. For example if you have to deal with databases. DBs can store booleans with a NULL value.

    Regards
  • DagSverre 2006-11-29 04:17
    <p>Can it be true? The original code is better than most of the comments!</p><p>This is the funniest in days - everyone trying to be smart and do it better, and almost every one trying to post a better solution getting it wrong. The original code is actually the best attempt I&#39;ve seen yet (but I did skim it so I probably missed a clear-thinking soul here and there). Well, ok, it is probably a WTF to accept null as a value in this circumstance, but given that behaviour (treat null as a value, and changing to/from null should set the dirty flag) the original code beats most of the following attempts.<br /></p><p>The real WTF is variants of<br /></p><p>&nbsp;context.setDirty(isRecordLocked.equals(newValue))<br /><br />(ignoring null issues)</p>
  • Wiggles 2006-11-29 04:23
    [quote user=&quot;Goplat&quot;] <p>If you compare Boolean (or any reference type, actually) with != you are comparing the references, not the object contents like the original function does.<br /></p><p>[/quote]</p><p>&nbsp;This makes the assumption that recordLocked is a Boolean.&nbsp; If it&#39;s of the primitive type boolean and Java 1.5 is being used then autoboxing will come into effect and&nbsp;simple object equality == would work fine.</p>
  • sprx 2006-11-29 04:42
    [quote user=&quot;Goplat&quot;]<p>This is a great opportunity to use the trinary conditional operator :) Here&#39;s the simplest equivalent code I can come up with:<br /></p><p>if (isRecordLocked == null ? newValue != null : !isRecordLocked.equals(newValue))<br />&nbsp;&nbsp;&nbsp; context.setDirty(true);<br /></p><p>[/quote]</p><p>&nbsp;</p><p>Best solution I&#39;ve seen so far. Most other code pieces will result in a NullPointerException.</p><p>Because </p><p>if(isLocked != null || isLocked.doSomething()) is equal to if(null != null || null.doSomething())&nbsp;</p><p>&nbsp;if isLocked is null of course.</p><p>&nbsp;</p><p>CAPTCHA = paula&nbsp;</p>
  • Slacker 2006-11-29 04:42
    That's so stupid it's almost funny
  • Earl Purple 2006-11-29 04:45
    <p>The original code is not so terrible except that it would be useful to have a function to compare tri-state booleans. So how about I write one</p><p>static public boolean equalsTristateBoolean( Boolean first, Boolean second )<br />{<br />&nbsp;&nbsp; return (( first == null &amp;&amp; second == null )) || ( first != null &amp;&amp; second != null &amp;&amp; first.equals( second ) );<br />}</p><p>Put the above in ourUtil (our utilities) Now the original code could be something like:</p><p>public void setRecordLocked( Boolean newValue )<br />{&nbsp;<br />&nbsp;&nbsp;&nbsp; if ( ! ourUtil.equalsTristateBoolean( isRecordLocked, newValue ) )<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; context.setDirty( true );<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />&nbsp;&nbsp;&nbsp; }<br />}</p><p>&nbsp;</p>
  • Bart 2006-11-29 04:49
    [quote user=&quot;Wiggles&quot;][quote user=&quot;Goplat&quot;] <p>If you compare Boolean (or any reference type, actually) with != you are comparing the references, not the object contents like the original function does.<br /></p><p>[/quote]</p><p>&nbsp;This makes the assumption that recordLocked is a Boolean.&nbsp; If it&#39;s of the primitive type boolean and Java 1.5 is being used then autoboxing will come into effect and&nbsp;simple object equality == would work fine.</p><p>[/quote]</p><p>&nbsp;Unfortunately, you can&#39;t autounbox a null Boolean.&nbsp; The result will be a NullPointerException.</p><p>&nbsp;<br />The original code is messy because the &quot;flags&quot; are three state variables that probably should have been primitive types rather than Objects.&nbsp; Most likely the developer is using null as an uninitialized indicator rather than maintaining two separate flags for the value and the &quot;is initialized&quot; state.</p><p>You could rewrite it as:<br /><br />if( newValue == null &amp;&amp; isRecordLocked == null )<br />{<br />&nbsp; return;<br />}<br /></p><p>if( newValue == null || isRecordLocked == null || !newValue.equals(isRecordLocked))<br />{<br />&nbsp; setDirty(true);<br />&nbsp; isRecordLocked = newValue;<br />}<br /></p><p><br />This is logically equivalent to the original but maybe a little more readable.&nbsp; For completeness, a horrible but functional way to write it:</p><p><br />boolean is_unchanged = false;<br /></p><p>try<br />{<br />&nbsp; is_unchanged = (newValue == isRecordLocked || newValue.equals(isRecordLocked))<br />}<br />catch (NullPointerException e) {}</p><p>if( !is_unchanged )<br />{<br />&nbsp; setDirty(true);<br />&nbsp; isRecordLocked = newValue;<br />}<br /><br /></p>
  • nts 2006-11-29 04:49
    <p>if ( ( isRecordLocked == newValue ) || (isRecordLocked != null &amp;&amp; isRecordLocked.equals(newValue) ) ) {</p><p>&nbsp;&nbsp;&nbsp; //the same - do nothing&nbsp;</p><p>} else {</p><p>&nbsp;&nbsp;&nbsp; context.setDirty(true);</p><p>&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br /></p><p>}&nbsp;</p>
  • Ottokar 2006-11-29 05:13
    Well,

    what is that funny?
  • Ottokar 2006-11-29 05:18
    [quote user=&quot;Anonymous&quot;]<p><br />if( newValue == null &amp;&amp; isRecordLocked == null )<br />{<br />&nbsp; return;<br />}<br /></p><p>if( newValue == null || isRecordLocked == null || !newValue.equals(isRecordLocked))<br />{<br />&nbsp; setDirty(true);<br />&nbsp; isRecordLocked = newValue;<br />}<br /></p><p>[/quote]</p><p>&nbsp;</p><p>Well, </p><p>&nbsp;</p><p>that looks as the best solution I have seen so far.</p><p>&nbsp;What about this:</p><p>&nbsp;</p><p>&nbsp;</p><p>if( newValue ==&nbsp; isRecordLocked ) {<br />&nbsp; return;<br />}<br /></p><p>if( newValue == null || isRecordLocked == null )<br />{<br />&nbsp; setDirty(true);<br />}<br /></p><p>isRecordLocked = newValue;<br /><br /></p>
  • Anonymous 2006-11-29 05:30
    Alex could keep this site running indefinitely by generating the day's WTF from the previous day's comments.
  • Asd 2006-11-29 05:32
    <p>This forum really is a haven for complete idiots. Only about three of the commenters actually understood what this code was doing (and I suspect the submitter didn&#39;t either). When they are going to so much effort to support tri-value booleans they probably have a reason for it. This code is slightly ugly but is not broken, unlike most of the suggested replacements.</p><p>For anyone using Java who needs to compare possibly null objects may I suggest Jakarta commons-lang which include the Object.equals methods in a previous comment as ObjectUtils.equals (along which a bunch of other useful stuff which probably should be in Java - and some complete crap of course).</p><p>&nbsp;</p><p>My solution would be to change the Context class:</p><p>class Context {</p><p>&nbsp;public void setDirtyIfDifferent(Object obj1, Object obj2) {</p><p>&nbsp;&nbsp;&nbsp; setDirty(!ObjectUtils.equals(obj1, obj2));<br /></p><p>}&nbsp;</p><p>}</p><p>&nbsp;then the original method could be:<br /></p><pre>public void setIsRecordLocked(Boolean newValue) {<br /> context.setDirtyIfDifferent(newValue, isRecordLocked);<br /><br /> // Change the value.<br /> isRecordLocked = newValue;<br />}</pre>
  • Asd 2006-11-29 05:34
    [quote user=&quot;Anonymous&quot;]<p>This forum really is a haven for complete idiots. Only about three of the commenters actually understood what this code was doing (and I suspect the submitter didn&#39;t either). When they are going to so much effort to support tri-value booleans they probably have a reason for it. This code is slightly ugly but is not broken, unlike most of the suggested replacements.</p><p>For anyone using Java who needs to compare possibly null objects may I suggest Jakarta commons-lang which include the Object.equals methods in a previous comment as ObjectUtils.equals (along which a bunch of other useful stuff which probably should be in Java - and some complete crap of course).</p><p>&nbsp;</p><p>My solution would be to change the Context class:</p><p>class Context {</p><p>&nbsp;public void setDirtyIfDifferent(Object obj1, Object obj2) {</p><p>&nbsp;&nbsp;&nbsp; setDirty(!ObjectUtils.equals(obj1, obj2));<br /></p><p>}&nbsp;</p><p>}</p><p>&nbsp;then the original method could be:<br /></p><pre>public void setIsRecordLocked(Boolean newValue) {<br /> context.setDirtyIfDifferent(newValue, isRecordLocked);<br /><br /> // Change the value.<br /> isRecordLocked = newValue;<br />}</pre><p>[/quote]</p><p>D&#39;oh. I am one of the idiots. Of course the setDirtyIfDifferent method should be:</p><p>if(!ObjectUtils.equals(obj1,obj2)) {</p><p>&nbsp;setDirty(true);</p><p>}&nbsp;</p>
  • Ottokar 2006-11-29 05:35
    <p>[quote user=&quot;Anonymous&quot;]Alex could keep this site running indefinitely by generating the day&#39;s WTF from the previous day&#39;s comments.[/quote]</p><p>&nbsp;I Agree. :)<br />&nbsp;</p>
  • cheesy 2006-11-29 05:43
    <div>Here&#39;s a way to test it...</div><div><br class="khtml-block-placeholder" /></div><div>public class test {</div><div>&nbsp; &nbsp; private static Boolean isRecordLocked;</div><div>&nbsp; &nbsp; private static Boolean choices1[] = {null, new Boolean(true), new Boolean(false)};</div><div>&nbsp; &nbsp; private static Boolean choices2[] = {null, new Boolean(true), new Boolean(false)};</div><div>&nbsp; &nbsp; </div><div>&nbsp; &nbsp; public static void main(String[] args) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; for (int func = 0; func &lt; 4; func++) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; for (int i = 0; i &lt; 3; i++) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; for (int j = 0; j &lt; 3; j++) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; System.out.print(&quot;old=&quot; + choices1[i] + &quot;, new=&quot; + choices2[j] + &quot;, &quot;);</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; isRecordLocked = choices1[i];</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; switch (func) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; case 0: setIsRecordLocked(choices2[j]); break;</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; case 1: setIsRecordLocked1(choices2[j]); break;</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; case 2: setIsRecordLocked2(choices2[j]); break;</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; System.out.println();</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }&nbsp; &nbsp; </div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; System.out.println();</div><div>&nbsp; &nbsp; &nbsp; &nbsp; }</div><div>&nbsp; &nbsp; }</div><div>&nbsp; &nbsp; </div><div>&nbsp; &nbsp; public static void setIsRecordLocked(Boolean newValue) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; // Update state if required.</div><div>&nbsp; &nbsp; &nbsp; &nbsp; if (isRecordLocked != null) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; if (newValue == null || !isRecordLocked.equals(newValue)) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; System.out.print(&quot;setDirty!&quot;);</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }</div><div>&nbsp; &nbsp; &nbsp; &nbsp; }</div><div>&nbsp; &nbsp; &nbsp; &nbsp; else if (newValue != null) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; System.out.print(&quot;setDirty!&quot;);</div><div>&nbsp; &nbsp; &nbsp; &nbsp; }</div><div><br class="khtml-block-placeholder" /></div><div>&nbsp; &nbsp; &nbsp; &nbsp; // Change the value.</div><div>&nbsp; &nbsp; &nbsp; &nbsp; isRecordLocked = newValue;</div><div>&nbsp; &nbsp; }</div><div><br class="khtml-block-placeholder" /></div><div>&nbsp; &nbsp; public static void setIsRecordLocked1(Boolean newValue) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; if (isRecordLocked != null) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; if (!isRecordLocked.equals(newValue)) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; System.out.print(&quot;setDirty!&quot;);</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; }</div><div>&nbsp; &nbsp; &nbsp; &nbsp; }</div><div>&nbsp; &nbsp; &nbsp; &nbsp; else if (newValue != null) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; System.out.print(&quot;setDirty!&quot;);</div><div>&nbsp; &nbsp; &nbsp; &nbsp; }</div><div>&nbsp; &nbsp; &nbsp; &nbsp; isRecordLocked = newValue;</div><div>&nbsp; &nbsp; }</div><div><br class="khtml-block-placeholder" /></div><div>&nbsp; &nbsp; public static void setIsRecordLocked2(Boolean newValue) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; if (isRecordLocked != newValue) {</div><div>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; System.out.print(&quot;setDirty!&quot;);</div><div>&nbsp; &nbsp; &nbsp; &nbsp; }</div><div>&nbsp; &nbsp; &nbsp; &nbsp; isRecordLocked = newValue;</div><div>&nbsp; &nbsp; }</div><div>}</div><div><br class="khtml-block-placeholder" /></div><div><br class="khtml-block-placeholder" /></div><div>The 1st is the original. The 3rd one shows you can&#39;t do a simple &quot;!=&quot;.</div><div><br class="khtml-block-placeholder" /></div><div>The 2nd one is a slightly simplified version of the original.&nbsp;The only difference is the &quot;newValue == null&quot; check is redundant, since if newValue IS null it will NOT be equal to isRecordLocked, because we already checked that isRecordLocked is definitely NOT null.</div><div><br class="khtml-block-placeholder" /></div><div>This is the best I can do... if anyone has a simpler function that passes the test (using Boolean types, not boolean primitives), let me know. If not, as far as I&#39;m concerned this code is perfectly fine except that one little redundant &quot;newValue == null&quot; check.</div><div><br class="khtml-block-placeholder" /></div><div>(This is all assuming the original code was in Java and wasn&#39;t translated from another language...)</div>
  • Chiraz 2006-11-29 05:43
    <p>There are a couple of utility classes that would certainly help in this case. Check out HashCodeUtil and EqualsUtil on Google and you get the idea.</p><p>The main reason for these &quot;one-liners&quot; to go through specific utility classes is that you don&#39;t want any nullpointers to occur in equals or hashCode functions, that is just a very costly mistake to make (either in production or during development).</p><p>The other reason is that you can change system policy for Dates for example using these utility functions. For example a function &quot;isGreatherThan&quot; for two dates. </p><p>What if the source date is null, does that mean infinity or actually null?&nbsp; How should the second date compare to the first?&nbsp; A utility function makes this policy consistent, rather than an invocation of the same functionality in each place. I&#39;m recently looking at these simple &quot;one-liners&quot; and find that a utility helper function is a great help for establishing these policies. Also, as soon as you want to construct objects and wish to add some functionality, changing the function prototype with modern IDE&#39;s will automatically flag any instances where these objects are being used.</p><p>For the above, I&#39;d actually consider using proxies, interceptors or AOP and see how this performs against manually invoked code. For example, every object of interest could become wrapped automatically and contain this &quot;dirty flag&quot;. Any method invoked that would change a property can then individually set the object to dirty without any concern to the programmer.</p><p>Then again... the name &quot;isRecordLocked&quot; in code sort of bothers me. is this Enterprise thing trying to mimic a database or what?<br />&nbsp;</p>
  • Ottokar 2006-11-29 05:48
    [quote user=&quot;Chiraz&quot;]<p>Then again... the name &quot;isRecordLocked&quot; in code sort of bothers me. is this Enterprise thing trying to mimic a database or what?<br /></p><p>[/quote]</p><p>Even if you use a database you have lock records sometimes. For example if someone wants to change a value over a webpage. </p><p>&nbsp;<br />Regards<br />&nbsp;</p>
  • another damn kiwi 2006-11-29 05:54
    <p>Im not really sure&nbsp;I understand the animosity to the Boolean object.</p><p>The&nbsp;Boolean object can only have 2 values either true or false. When the object reference is null, it means we dont have a Boolean object yet (perhapes because at the moment we dont know what value it should take, so havent created it yet)</p><p>Makes perfect sense to me, I cant see a third value in the Boolean object no matter how hard i look.</p><p>On topic, I guess all the attempts show that sometimes what we think is a piece of cake, is sometimes really a can of worms. (Watties, extra cheesy, are my favorite.)</p>
  • another damn kiwi 2006-11-29 06:02
    <p>[quote user=&quot;Anonymous&quot;] </p><p>...but maybe a little more readable</p><p>...</p><p>if( !is_unchanged ) ...<br /></p><p>[/quote]</p><p>ZOMG WTF!!! </p>
  • Ottokar 2006-11-29 06:11
    [quote user=&quot;Anonymous&quot;]<p>Makes perfect sense to me, I cant see a third value in the Boolean object no matter how hard i look.</p><p>[/quote]</p><p>A simple example:</p><p>You have a database. The DB has tables. A table can have booleans. And they can have three states (NULL, true, false). So your system has to handle the tristate DB-Values. </p><p>Or in a general speaking: Whenever you have to create an empty object which will be filled with data by a different system (other computer, other object, other ...) you should set the data to a default NULL value. Otherwise you wont know whether the &quot;fill up&quot; worked or worked not. Thats what we call &quot;defensive programing&quot;.<br /></p><p>Regards.<br />&nbsp;</p>
  • Asd 2006-11-29 06:26
    Just to point out isRecordLocked may not mean Database style record locking. It could mean, for example, that your user account is locked, or maybe an article is locked for publishing.<br />
  • donazea 2006-11-29 06:30
    [quote user=&quot;Einsidler&quot;]<p>[quote user=&quot;Anonymous&quot;]public void setIsRecordLocked(Boolean newValue) {<br />&nbsp;&nbsp;&nbsp; if (isRecordLocked != newValue)<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; context.setDirty(true);<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />&nbsp;&nbsp;&nbsp; }<br />}<br /><br />The code isn&#39;t that bad in the scheme of WTFs.&nbsp; At least it explicitly covers the right bases.<br /><br />Although why a language would need the Boolean class which is essentially a tri-state (true, false, null) rather than a native bool is beyond me.<br />[/quote]</p><p>Maybe they wanted to add a FileNotFound state<br /> </p><p>[/quote]</p><p>Yikes, I&#39;ve been hanging round here too long, this was the first thing that popped into my head. (-:&nbsp;</p>
  • wtf 2006-11-29 07:32
    Wow, of soooo many posts so far, there has only been ONE brief and concise oneliner check - Jon Haugsand that proposed the static Object.equals(o1, o2).
    Two more posts didn't have actual errors, but were kinda like stealing the idea. Unneeded...
    First someone just inlined that static inside the code. Way to go. And then someone used the same static, only in a separate ObjectUtils class instead of Object, adding unneeded setDirtyDifferent crap. Got it correct from the second try, atleast.

    A couple more correct corrections used code not as brief as Jon's, and came after he had posted it too.
    Why did this thread not STOP after the solution was given?
    Why did the wrong solutions not stop atleast?
    WTF?
  • daen 2006-11-29 07:44
    <pre>/**
    Set the value of the isRecordLocked attribute.
    @param newValue is the new isRecordLocked value.
    */
    public void setIsRecordLocked(Boolean newValue) {
    // Set dirty flag if the new value is not the same as the current value
    context.setDirty( isRecordLocked != newValue);
    <br />
    // Change the value.
    isRecordLocked = newValue;
    }
    </pre>
  • Eduardo Habkost 2006-11-29 08:29
    [quote user=&quot;Ottokar&quot;]<p>You have a database. The DB has tables. A table can have booleans. And they can have three states (NULL, true, false). So your system has to handle the tristate DB-Values. </p><p>[/quote]</p><p>&nbsp;</p><p>Of course not!<br />&nbsp;<br />They can have four states: true, false, FileNotFound, NULL.&nbsp;</p>
  • Richard 2006-11-29 08:34

    <p>I code here I&#39;ve generalised the equalsTristateBoolean to one that takes Objects</p>
    <pre>public static boolean equals(Object a, Object b)
    {<br />&nbsp;&nbsp;&nbsp; return ( (a == null &amp;&amp; b == null) || (a != null &amp;&amp; b != null &amp;&amp; a.equals(b))<br />}</pre>
    <p>I&#39;ve generally not bothered with b!=null in the second test as I expect a.equals(b) to handle that case. Maybe the extra check saves a method call.<br />
    </p>
    <p>&nbsp;</p><p>&nbsp;Captcha was &quot;Java&quot;.<br />&nbsp;</p>
  • Jim 2006-11-29 08:37
    The original code never calls setDirty with&nbsp;false as its argument. This snippet would set dirty to false depending on the equality or otherwise of the old and new values. This isn&#39;t functionally equivalent to the original.
  • An apprentice 2006-11-29 08:37
    <p>How about:</p>
    <pre>public void setIsRecordLocked(Boolean newValue) {<br /> // Call setDirty if old and new values are different<br /> if (newValue != isRecordLocked &amp;&amp; (newValue == null || !newValue.equals(isRecordLocked))) {<br /> context.setDirty(true);<br /> }<br /> isRecordLocked = newValue;<br />}</pre>

    <p>One liner and seems to be correct.</p>
  • ParkinT 2006-11-29 08:45
    [quote user=&quot;Tim Gallagher&quot;] <p>the if statements can be replaced entirely by a much more understandable one-liner.&quot;[/quote]</p><p>But, if you are being paid by SLOC...</p><p>&nbsp;</p><p>I work on a US Air Force project and many of the contractors working on this System pay their coders based on lines of code.&nbsp; Not a very good way to reward efficient coding style but, at least, it is a consistent way to measure.</p>
  • anonymous koder 2006-11-29 09:07
    <p>I think you are all missing the point, if you take pen and paper and write a table with all possible conditions, you will see that actually setDirty(true) <strong>is called every time except if both newValue and isRecordLocked are null</strong> !!!</p>
    <p>So the correct code snippet would be:<font face="Lucida Console" size="2"></font></p><p><font face="Lucida Console" size="2">public void setIsRecordLocked(Boolean newValue) {&nbsp;
    &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; if((newValue==null) &amp;&amp; (isRecorLocked==null)) {&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; setDirty(true);</font></p><p><font face="Lucida Console" size="2">&nbsp;&nbsp;&nbsp; </font><font face="Lucida Console" size="2">}
    </font></p><p><font face="Lucida Console" size="2">&nbsp;&nbsp;&nbsp; // Change the value.</font><br /></p>
    <font face="Lucida Console" size="2">
    &nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />
    }
    </font>
    <p>&nbsp;Whether this is what the function was meant to do or not, I can&#39;t tell...<br /></p>
  • Anonymous Braveheart 2006-11-29 09:23
    <p>Look at it this way.</p><p>&nbsp;</p><p>Nullable boolean types are necessary to answer inane illogically-formed questions.</p><p>&nbsp;</p><p>For example:</p><p>&nbsp;</p><p><em>Have you stopped beating your wife?</em></p><p>&nbsp;</p><p>A lot of people would like to reach for the value <strong>null</strong> right there.</p><p>&nbsp;</p><p>Case closed.&nbsp;</p>
  • tango 2006-11-29 09:29
    [quote user=&quot;Anonymous&quot;][quote user=&quot;Ottokar&quot;]<p>You have a database.
    The DB has tables. A table can have booleans. And they can have three
    states (NULL, true, false). So your system has to handle the tristate
    DB-Values. </p><p>[/quote]</p><p>&nbsp;</p><p>Of course not!<br />&nbsp;<br />They can have four states: true, false, FileNotFound, NULL.&nbsp;</p><p>[/quote]</p><p>&nbsp;</p><p>Don&#39;t forget to add &#39;yellow&#39;.&nbsp;</p>
  • pauluskc 2006-11-29 09:38
    <p>Aha!&nbsp; This is what I was saying the whole time I read thru the thread and read the other examples.&nbsp; I second your notion!&nbsp; Can&#39;t change the value of that setDirty stuff because this could be one of dozens of checks.&nbsp; Different needs at the time could require more or less checks or different checks as different objects have different properties.&nbsp; So there could also be setIsRecordChanged() and setIsRecordInvalid() and others like that.&nbsp; By having this function, especially if it&#39;s part of a master (top-level) class (gosh, my OO terminology&nbsp;is not that great), instead of a one-liner, it&#39;s reusable and can transcend to children and etc.&nbsp; Remember &quot;public&quot; in the decleration.&nbsp; Maybe the security is WTFey.&nbsp; </p><p>Without having other code around it, it could be classifiable as a WTF, IMHO.</p>
  • pauluskc 2006-11-29 09:39
    Ding!&nbsp; We have the answer.
  • Dan 2006-11-29 09:42
    Nothing about this code is a WTF. The real WTF is that in order to think this is a WTF one would have to been limited to developing in Javascript using pre-built frameworks. Just because someone with little knowledge says something is stupid doesn&#39;t mean you don&#39;t have to think about it for yourself.<br />
  • Paul 2006-11-29 09:53
    <p>
    I had to use a pen and paper to work out exactly what conditions the setDirty(true) call was made.
    <br />I found the real WTF.</p>
  • Benanov 2006-11-29 10:04
    [quote user=&quot;Anonymous&quot;]<p>Look at it this way.<br /><br />Nullable boolean types are necessary to answer inane illogically-formed questions.</p><p>For example:</p><p><em>Have you stopped beating your wife?</em></p><p>A lot of people would like to reach for the value <strong>null</strong> right there.</p><p>Case closed.&nbsp;</p><p>[/quote]</p><p>I prefer <strong>throw new ArgumentNullException(&quot;Value of Wife is NULL&quot;);</strong></p><p>but that&#39;s just me.</p><p>&nbsp;</p>
  • rmr 2006-11-29 10:29
    [quote user=&quot;Anonymous&quot;]<p>[quote]I had to use a pen and paper to work out exactly what conditions the setDirty(true) call was made.[/quote]<br />I found the real WTF.</p><p>[/quote]</p><p>Yeah!&nbsp; Man that is messed up - a programmer using paper to wrap their head around something!</p><p>. . .&nbsp;</p><p>Seriously, where did the attitude that programmers should do everything in their heads come from?<br /><br />&nbsp;</p>
  • emurphy 2006-11-29 10:49
    <pre>if ( (isRecordLocked == null &amp;&amp; newValue != null)</pre><pre> || (isRecordLocked != null &amp;&amp; newValue == null)</pre><pre> || (!isRecordLocked.equals(newValue)) ) {</pre><pre> context.setDirty(true);</pre><pre>}&nbsp;</pre><pre>&nbsp;</pre><pre>Assumptions:</pre><pre>&nbsp;&nbsp;</pre><ul><li>The system has a legit reason for using the nullable &#39;Boolean&#39; class rather than the &#39;boolean&#39; primitive.</li><li>The conditional logic is correct, i.e. setDirty(true) unless (1) they&#39;re equal according to .equals() or (2) they&#39;re both null.</li><li>The usage of setDirty() is correct, i.e. if something else did setDirty(true) before us then (1) doing a second setDirty(true) is harmless, but (2) we must not step on that result by doing setDirty(false).&nbsp; This second point rules out all setDirty(conditional) solutions.</li><li>Short-circuit evaluation applies.&nbsp; If it doesn&#39;t (i.e. if this is an obfuscation of some other language where it doesn&#39;t), then we must restrict evaluation of the equals() part via if/else.</li><li>Clarity is more important than cleverness.&nbsp; Although I think all the clever one-liners are already ruled out by one or more of the above.<br /></li></ul>
  • File Man 2006-11-29 10:52
    [quote user=&quot;Anonymous&quot;][quote user=&quot;Anonymous&quot;][quote user=&quot;Ottokar&quot;] <p>You have a database. The DB has tables. A table can have booleans. And they can have three states (NULL, true, false). So your system has to handle the tristate DB-Values. </p><p>[/quote]</p><p>Of course not!<br />&nbsp;<br />They can have four states: true, false, FileNotFound, NULL.&nbsp;</p><p>[/quote]</p><p>Don&#39;t forget to add &#39;yellow&#39;.&nbsp;</p><p>[/quote]</p><p>You guys are slipping - it took 24 posts before someone even mentioned file-not-found</p>
  • suutar 2006-11-29 10:55
    <br /><p>if (newValue != isRecordLocked) {</p><p>&nbsp;&nbsp; context.setDirty(true);</p><p>&nbsp;&nbsp; isRecordLocked=newValue;</p><p>}<br />&nbsp; </p>
  • suutar 2006-11-29 10:57
    mmm. never mind, I missed an implication.<br />
  • Shadowman 2006-11-29 11:08
    <p>What I got, from working out the &#39;before&#39; and &#39;after&#39; values of these variables in excel is:</p><p>&nbsp;</p><p>public void setIsRecordLocked(Boolean newValue) {</p><p>&nbsp;&nbsp;&nbsp;&nbsp; if (isRecordLocked != newValue) {</p><p>&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; context.setDirty(true);</p><p>&nbsp;&nbsp; }</p><p>&nbsp;&nbsp; isRecordLocked = newValue;</p><p>}&nbsp;</p><p>&nbsp;</p><p>The only time context.setDirty() remains uncalled (and I assume &#39;dirty&#39; is unchanged) is when isRecordLocked == newValue.&nbsp; In every other possible outcome, dirty is set to true.<br /></p>
  • Ghost Ware Wizard 2006-11-29 11:17
    <p>that ranks right up there with creating an object with functions in it to get a reserved type.</p><p>e.g.</p><p>public static class ObjectVariables<br />{</p><p>&nbsp;&nbsp;&nbsp; ObjectVariables<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp; }<br />&nbsp;&nbsp;&nbsp; <br />&nbsp;&nbsp;&nbsp; public static int INT()<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; int nRetVal = -1;<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; return nRetVal;<br />&nbsp;&nbsp;&nbsp; }<br />&nbsp;&nbsp;&nbsp; <br />&nbsp;&nbsp;&nbsp; public static double DOUBLE()<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; double dblRetVal = -1;<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; return dblRetVal;<br />&nbsp;&nbsp;&nbsp; }<br />&nbsp;<br />}<br /></p><p>&nbsp;</p>
  • JamesCurran 2006-11-29 11:19
    <p>[quote user=&quot;Anonymous&quot;]I think you are all missing the point, if you take pen and paper and write a table with all possible conditions, you will see that actually setDirty(true) <strong>is called every time except if both newValue and isRecordLocked are null</strong> !!![/quote]</p><p>That&#39;s just not true.&nbsp;&nbsp; Consider the case newValue &amp; isRecordLocked are both false. </p><p>if (isRecordLocked != null) {<br />&nbsp;&nbsp;&nbsp; This is true, so we continue. </p><p>if (newValue == null || <br />&nbsp;&nbsp;&nbsp; This is false so we evaluate the other side of the OR</p><p>|| !isRecordLocked.equals(newValue)) {<br />&nbsp;&nbsp;&nbsp; This is also false, so we exit the if() block.</p><p>&nbsp;&nbsp;&nbsp; else if (newValue != null) {&nbsp; context.setDirty(true);&nbsp;&nbsp;&nbsp; }<br />Since the <strong>first if()</strong> was true, we skip the else clause (did you mistakenly associate it with the second if()?)</p><p>&nbsp;&nbsp; isRecordLocked = newValue;<br />Finally, we pointless set a value which is already false to false.<br /></p><p>&nbsp;</p><p>&nbsp;</p><p>&nbsp;</p>
  • JamesCurran 2006-11-29 11:27
    <p>[quote user=&quot;Shadowman &quot;]The only time context.setDirty() remains uncalled (and I assume &#39;dirty&#39; is unchanged) is when isRecordLocked == newValue.&nbsp; In every other possible outcome, dirty is set to true.<br />[/quote]</p><p>As others have pointed out, == is only true for two references to the same object, but <strong>not</strong> for two distinct objects which just happen to have the same value. To compare to separate objects, you have to use the Boolean.equals method, but to use that, you have to make sure that the calling object is not null.</p>
  • Maurits 2006-11-29 11:53
    <p>I find that the main source of confusion in snippets like these is not the length of the code, but the negative assertions.&nbsp; One-liners don&#39;t tend to make things clearer.&nbsp; Here&#39;s my refactoring of the code to use positive assertions, as well as some (hopefully) better comments:</p><p>public void setIsRecordLocked(Boolean newValue) {<br /><br />&nbsp;&nbsp;&nbsp; if (newValue == null &amp;&amp; isRecordLocked == null) {<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; // no change; was null, still is<br />&nbsp;&nbsp;&nbsp; } else if (newValue == null || isRecordLocked == null) {<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; // change to or from null<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; context.setDirty(true);<br />&nbsp;&nbsp;&nbsp; } else if (newValue.equals(isRecordLocked)) {<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; // no change; either a) was true, still is or b) was false, still is<br />&nbsp;&nbsp;&nbsp; } else {<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; // change from true to false or vice versa<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; context.setDirty(true);<br />&nbsp;&nbsp;&nbsp; }<br />}<br /><br /></p>
  • Anonymous 2006-11-29 11:58

    <blockquote><pre>public void setIsRecordLocked(Boolean newValue) {<br /> if (!(isRecordLocked == null ?<br /> isRecordLocked == newValue :<br /> isRecordLocked.equals(newValue))) {<br /> context.setDirty(true);<br /> }<br />}</pre></blockquote>
    <p>Simple, effective, and makes the intention much clearer.</p><p>Note that the &quot;isRecordLocked == newValue&quot; could just as easily be &quot;newValue == null&quot; but using &quot;isRecordLocked == newValue&quot; makes the intention clearer.<br /></p><p>Null handling like the above comes up all the time in Java and the above pattern is repeated all over the Sun Java library implementation.&nbsp;</p>
  • Yuriy 2006-11-29 12:05
    <pre>Ok, I <span style="font-style: italic">think</span> this should be functionally identical to the original: <br /></pre><pre>public void setIsRecordLocked(Boolean newValue) {<br /> // Update state if required.<br /> if ( !(newValue == null &amp;&amp; isRecordLocked == null) &amp;&amp; !(newValue != null &amp;&amp; newValue.equals(isRecordLocked)) ) {<br /> context.setDirty(true);<br /> }<br /> // Change the value.<br /> isRecordLocked = newValue;<br />}</pre><pre>Also, the &#39;isRecordLocked = newValue;&#39; could almost certainly be moved inside the if statement.</pre><pre>The only way that would cause errors is if there is some WTF code elsewhere that compares the references instead of using .equals().&nbsp;</pre>
  • Yuriy 2006-11-29 12:10
    [quote user=&quot;Anonymous&quot;]<blockquote><pre>public void setIsRecordLocked(Boolean newValue) {<br /> if (!(isRecordLocked == null ?<br /> isRecordLocked == newValue :<br /> isRecordLocked.equals(newValue))) {<br /> context.setDirty(true);<br /> }<br />}</pre></blockquote>
    <p>Simple, effective, and makes the intention much clearer.</p><p>Note that the &quot;isRecordLocked == newValue&quot; could just as easily be &quot;newValue == null&quot; but using &quot;isRecordLocked == newValue&quot; makes the intention clearer.<br /></p><p>Null handling like the above comes up all the time in Java and the above pattern is repeated all over the Sun Java library implementation.&nbsp;</p><p>[/quote]</p><p>&nbsp;</p><p>Hmm, I could&#39;ve sworn that &#39;null == null&#39; returned false.&nbsp; Learn something new everyday :)</p><p>On the other hand, you forgot to set isRecordLocked to newValue.&nbsp;</p>
  • Mkamba 2006-11-29 12:13
    <p>No, no!&nbsp; You guys have got it all wrong!&nbsp; The real WTF has nothing to do with the code, but with the moral.</p><p>Think about this for a second.&nbsp; You&#39;re hanging around some place (coffee shop, bar, produce department) and a&nbsp;member of the appropriately attractive gender walks up to you and says &quot;setDirty( true ).&quot;&nbsp; How many of us would really withold even our spam email adress after that?</p>
  • VGR 2006-11-29 12:29

    <p>Hoo boy.&nbsp; Yes, lots and lots of wrong answers here.</p>

    <p>First, I can only address this as a Java issue, though it wouldn&#39;t surprise me if the real code is not in Java but was turned into Java as part of Alex&#39;s anonymization process.&nbsp; Since I have no way to know, I&#39;ll just pretend it really is in Java.</p>

    <p>Second, the use of the == and != operators is not acceptable, not even in Java 1.5 or 1.6 where auto(un)boxing is in use.&nbsp; You can see this by executing this code:</p>

    <p>System.out.println(Boolean.TRUE == new Boolean(true));</p>
    <p>Third, <a href="http://java.sun.com/j2se/1.5.0/docs/api/java/lang/Object.html#equals%28java.lang.Object%29">it is always permitted to pass null to the equals method.</a>&nbsp;</p>

    <p>So, the logic here is:&nbsp; if newValue differs from isRecordLocked, call setDirty(true).</p>

    <p>My version:&nbsp;</p>

    <pre>boolean changed = (isRecordLocked == null ? (newValue != null) : !isRecordLocked.equals(newValue));
    if (changed)
    {
    &nbsp;&nbsp;&nbsp; context.setDirty(true);
    &nbsp;&nbsp;&nbsp; isRecordLocked = newValue;
    }</pre>
  • SpecialBrad 2006-11-29 12:31
    <p>if (isRecordLocked == null) {</p><p>&nbsp;&nbsp;&nbsp; if (newValue == null) setDirty(true);<br />&nbsp;} else {</p><p>&nbsp;&nbsp;&nbsp; if (isRecordLocked.equals(newValue)) setDirty(true);&nbsp;</p><p>}&nbsp;</p><p>&nbsp;Not that clean, but more readable than the original (I think).<br />&nbsp;</p>
  • Paul 2006-11-29 12:31
    [quote user=&quot;rmr&quot;][quote user=&quot;Anonymous&quot;]<p>[quote]I had to use a pen and paper to work out exactly what conditions the setDirty(true) call was made.[/quote]<br />I found the real WTF.</p><p>[/quote]</p><p>Yeah!&nbsp; Man that is messed up - a programmer using paper to wrap their head around something!</p><p>. . .&nbsp;</p><p>Seriously, where did the attitude that programmers should do everything in their heads come from?<br /><br />&nbsp;</p><p>[/quote]</p><p>There&#39;s nothing wrong with using pen and paper to analyze code in and of itself. However, if you need pen and paper to evaluate something this simple, you&#39;re going to have problems when you come to something more than a few lines long. It&#39;s like needing paper to evaluate 4 * 5 -- if the basics trip you up, the hard stuff is going to kill you.</p><p>On another note, I&#39;m rather surprised by the number of people failing to grasp the code and failing horribly in their attempts to correct or simplify it. I can understand if Java isn&#39;t your language, but if it is...&nbsp;</p>
  • dasmb 2006-11-29 12:37
    <p>This particular example is in Java, and in Java a boolean and a Boolean have this important distinction: a boolean is not an Object.&nbsp; Because it is not an object, you can&#39;t use a primitive boolean as a parameter to any method that takes an Object, which means you can&#39;t use a boolean in a Collection or Map.&nbsp; Thus there&#39;s Boolean, an immutable wrapper around a true or false value.&nbsp; This is called &quot;boxing,&quot; which I&#39;m sure most of you know but I&#39;m putting it out there because there seems to be some confusion about it.</p><p>In an application that deals extensively with Collections and Maps -- such as pretty much any ORM system -- you&#39;re likely to have more boxed primitives than actual primitives, and to unbox and rebox them before each method call would be kind of stupid.&nbsp; In such an application, you&#39;d see a lot of method signatures looking for the boxed versions of Booleans -- not because you need ternary logic, but because you already have the object.<br /></p><p>I have no problem whatsoever with this code.&nbsp; Draw out the truth table and you find that we want to dirty the record when both aren&#39;t null AND both aren&#39;t equal.&nbsp; That&#39;s exactly what this code is testing, while being careful not to call a method on a null isRecordLocked object.</p><p>Okay, there is an unnecessary test of a null newValue which I&#39;m sure you can chock up to a misunderstanding of the equals() contract.&nbsp; But the real WTF here is why Carl Cerecke thinks context.setDirty( (isRowLocked != null &amp;&amp; !isRowLocked.equals(newValue))&nbsp; || newValue != null ) is more legible.&nbsp; You don&#39;t make better code by condensation!<br /></p>
  • tibo 2006-11-29 12:38
    [quote user=&quot;Tim Gallagher&quot;]<p>It was no real surprise that the if statements can be replaced entirely by a much more understandable one-liner.&quot;</p><p>[/quote]</p><p>&nbsp;Yeah, that&#39;s true, but the one-liner would messier. There&#39;s absolutely nothing wrong with the code snippet (other than manipulating the static variable &quot;context&quot; like this).<br />&nbsp;</p>
  • dasmb 2006-11-29 12:40
    [quote user=&quot;Anonymous&quot;]<p>if (isRecordLocked == null) {</p><p>&nbsp;&nbsp;&nbsp; if (newValue == null) setDirty(true);<br />&nbsp;} else {</p><p>&nbsp;&nbsp;&nbsp; if (isRecordLocked.equals(newValue)) setDirty(true);&nbsp;</p><p>}&nbsp;</p><p>&nbsp;Not that clean, but more readable than the original (I think).<br /></p><p>[/quote]</p><p>You are a special kind of idiot for fucking this up.<br /></p>
  • timg 2006-11-29 12:41
    I am a machine, you insensitive clod!
  • Anonymous 2006-11-29 12:43
    <p>[quote user=&quot;Anonymous&quot;]Okay, there is an unnecessary test of a null newValue which I&#39;m sure you can chock up to a misunderstanding of the equals() contract.&nbsp; But the real WTF here is why Carl Cerecke thinks context.setDirty( (isRowLocked != null &amp;&amp; !isRowLocked.equals(newValue))&nbsp; || newValue != null ) is more legible.&nbsp; You don&#39;t make better code by condensation![/quote]</p><p>Plus that code would likely be wrong: it would unset the dirty flag when setting isRecordLocked to its current value.</p>
  • Martin 2006-11-29 12:48
    <p>If you really really want the setDirty logic in one line:</p><p>&nbsp;</p><pre>public void setIsRecordLocked(Boolean newValue) {<br /> if( ( isRecordLocked != null &amp;&amp; !isRecordLocked.equal( newValue ) ) || ( isRecordLocked == null &amp;&amp; newValue != null ) ) context.setDirty( true );<br /> isRecordLocked = newValue;<br />}</pre>
  • dasmb 2006-11-29 12:49
    [quote user=&quot;Anonymous&quot;]<p>[quote user=&quot;Anonymous&quot;]Okay, there is an unnecessary test of a null newValue which I&#39;m sure you can chock up to a misunderstanding of the equals() contract.&nbsp; But the real WTF here is why Carl Cerecke thinks context.setDirty( (isRowLocked != null &amp;&amp; !isRowLocked.equals(newValue))&nbsp; || newValue != null ) is more legible.&nbsp; You don&#39;t make better code by condensation![/quote]</p><p>Plus that code would likely be wrong: it would unset the dirty flag when setting isRecordLocked to its current value.</p><p>[/quote]</p><p>Good catch. &nbsp;&nbsp; I bow to the superior intelligence.&nbsp; It&#39;s got to be a minimum two-liner (if statement, value set) testing a minimum of three booleans.&nbsp;</p>
  • tibo 2006-11-29 12:53
    [quote user=&quot;Anonymous&quot;]<p>If you really really want the setDirty logic in one line:</p><p>&nbsp;</p><pre>public void setIsRecordLocked(Boolean newValue) {<br /> if( ( isRecordLocked != null &amp;&amp; !isRecordLocked.equal( newValue ) ) || ( isRecordLocked == null &amp;&amp; newValue != null ) ) context.setDirty( true );<br /> isRecordLocked = newValue;<br />}</pre><p>[/quote]</p><p>Wrong: if newValue is null this will throw a NullPointerException.&nbsp;</p>
  • Martin 2006-11-29 12:57
    <p>From Boolean.equals API:</p><p>&nbsp;&quot;Returns <code>true</code> if and only if the argument is not
    <code>null</code> and is a <code>Boolean </code>object that
    represents the same <code>boolean</code> value as this object.&quot;</p><p>No null pointer exception.<br />&nbsp;</p>
  • dasmb 2006-11-29 12:58
    [quote user=&quot;Anonymous&quot;]<p>If you really really want the setDirty logic in one line:</p><p>&nbsp;</p><pre>public void setIsRecordLocked(Boolean newValue) {<br /> if( ( isRecordLocked != null &amp;&amp; !isRecordLocked.equal( newValue ) ) || ( isRecordLocked == null &amp;&amp; newValue != null ) ) context.setDirty( true );<br /> isRecordLocked = newValue;<br />}</pre><p>[/quote]</p><p>Come on, <a href="http://jalopy.sourceforge.net/">Jalopify </a>your shit, man!&nbsp; It would really be:</p><pre>public void setIsRecordLocked(Boolean newValue) {<br /> if( ( isRecordLocked != null &amp;&amp; !isRecordLocked.equal( newValue ) ) <br /> || ( isRecordLocked == null &amp;&amp; newValue != null ) ) <br /> context.setDirty( true );<br /> isRecordLocked = newValue;<br />}</pre><p>So much more legible!&nbsp; So much more clear!&nbsp; I love the code snippet of the day!</p><p>PS: in Perl you can do all this with a single character.&nbsp; It&#39;s Unicode 0x25EC, provided that it doesn&#39;t come after a bang, hash, crack, semi-colon or the word &quot;bindle.&quot;&nbsp; I don&#39;t know why you fuckers aren&#39;t using it.</p>
  • tibo 2006-11-29 13:00
    [quote user=&quot;Anonymous&quot;]<p>From Boolean.equals API:</p><p>&nbsp;&quot;Returns <code>true</code> if and only if the argument is not
    <code>null</code> and is a <code>Boolean </code>object that
    represents the same <code>boolean</code> value as this object.&quot;</p><p>No null pointer exception.<br />&nbsp;</p><p>[/quote]</p><p>&nbsp;</p><p>I stand corrected.&nbsp;</p>
  • Ak 2006-11-29 13:03
    Okay on a WTF scale of 1-10 this ranks about 3. Its just some poorly structured code which I happen to see everyday.<br />
  • Asd 2006-11-29 13:03
    First ObjectUtils already exists in the jakarta commons-lang library, and it is better to reuse that than write it yourself. Second, my point was not to reduce the code to an uber-leet single liner. This kind of check probably happens throughout the object as any attribute change would probably mean you need to save it out or something like that. So my solution was intended to reduce this duplication to one line per attribute. Of course proxies/AOP may be a better way to do this, but would be a huge complication.<br />
  • Martin 2006-11-29 13:04
    <p>[quote user=&quot;Anonymous&quot;]Come on, <a href="http://jalopy.sourceforge.net/">Jalopify </a>your shit, man![/quote]</p><p>It ceases to be a one liner, which was the implied challenge of the WTF post.</p><p>I wouldn&#39;t write my code this way.&nbsp; It was deliberate.&nbsp;</p>
  • dasmb 2006-11-29 13:14
    <p>[quote user=&quot;Anonymous&quot;]First ObjectUtils already exists in the jakarta commons-lang library, and it is better to reuse that than write it yourself.<br />[/quote]</p><p>Not neccessarily.&nbsp; For one thing, we&#39;ve been bit on the ass here numerous times by shoddy, amateurish code from the jakarta commons project (funny how so many shitty hackers are giving their work away for free).&nbsp; For another, we may not want an entire library to be required everywhere our object goes (let&#39;s say it&#39;s a web service, or an applet, or J2ME).&nbsp; Finally, seems like a whole lot of fucking work just to test a handful of local booleans.<br /></p><p>[quote user=&quot;Anonymous&quot;] <br />Second, my point was not to reduce the code to an uber-leet single liner. This kind of check probably happens throughout the object as any attribute change would probably mean you need to save it out or something like that. So my solution was intended to reduce this duplication to one line per attribute. Of course proxies/AOP may be a better way to do this, but would be a huge complication.<br />[/quote]</p><p>You&#39;re right about that -- and it would be functionally equivalent to this code only much slower, assuming it were functional.&nbsp; If it weren&#39;t functional, well we&#39;d be stuck because even if we were able to find the defect, we wouldn&#39;t be able to fix it in code.&nbsp; We&#39;d have to patch the post AOP compilation class, all for what?&nbsp; To eliminate an obvious, clearcut, understandable pattern and replace it with a buzzword capable cycle-waste?</p><p>I&#39;ll have my pizza with extra Enterprise, please.<br /><br />&nbsp;</p>
  • rmr 2006-11-29 13:54
    [quote user=&quot;Anonymous&quot;][quote user=&quot;rmr&quot;][quote user=&quot;Anonymous&quot;]<p>[quote]I had to use a pen and paper to work out exactly what conditions the setDirty(true) call was made.[/quote]<br />I found the real WTF.</p><p>[/quote]</p><p>Yeah!&nbsp; Man that is messed up - a programmer using paper to wrap their head around something!</p><p>. . .&nbsp;</p><p>Seriously, where did the attitude that programmers should do everything in their heads come from?<br /><br />&nbsp;</p><p>[/quote]</p><p>There&#39;s nothing wrong with using pen and paper to analyze code in and of itself. However, if you need pen and paper to evaluate something this simple, you&#39;re going to have problems when you come to something more than a few lines long. It&#39;s like needing paper to evaluate 4 * 5 -- if the basics trip you up, the hard stuff is going to kill you.</p><p>On another note, I&#39;m rather surprised by the number of people failing to grasp the code and failing horribly in their attempts to correct or simplify it. I can understand if Java isn&#39;t your language, but if it is...&nbsp;</p><p>[/quote]</p><p>But it isn&#39;t simple. . . that&#39;s why it is on TDWTF.</p><p>I think that is a big part of the problem here.&nbsp; Programmers aren&#39;t willing to admit that they &quot;need pen and paper&quot;.&nbsp; If the original programmer had bee smart enough to realize that he needed to analyze what this function was doing a little bit more, this entry wouldn&#39;t exist!<br /></p>
  • Calling Shotgun 2006-11-29 14:05
    <p>Attempt at the one liner (Although, this is about as readable as the way he did it.)</p><p>&nbsp;&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; if(<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; (isRecordLocked != null &amp;&amp; !isRecordLocked.equals(newValue))&nbsp; ||<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; (newValue != null &amp;&amp; !newValue.equals(isRecordLocked)) ||<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; ((isRecordLocked = newValue).valueOf(&quot;false&quot;))<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; )<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp; {<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; context.setDirty(true);<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp; }<br /><br />&nbsp;</p><p>basically, context.setDirty(true) only occurs if isRecordLocked != newValue.&nbsp; I put two comparisons in the if statement, so that whichever one wasn&#39;t null would be compared to the other, and then verified inequality.&nbsp; I didn&#39;t put in a case for when they&#39;re both null, because it&#39;s redundant.</p><p>I then added a third line which nests the isRecordLocked = newValue into the if statement (thereby having a &quot;one-liner&quot; with two seperate assignment statements - cheap workarounds, ahoy!), using the ().valueOf(&quot;false&quot;) because that&#39;s the only way the compiler would allow that assignment inside the conditional area.&nbsp; I designed it so the overall condition is if (&quot;we should execute this code&quot;) || false ) so that the assignment at the end didn&#39;t change the logic for going into the if statement&#39;s code block.</p><p>-Alex&nbsp;</p>
  • tchize 2006-11-29 14:05
    [quote user=&quot;Tim Gallagher&quot;]<p>I had to use a pen and paper to work out exactly what conditions the setDirty(true) call was made. It was no real surprise that the if statements can be replaced entirely by a much more understandable one-liner.&quot;</p>

    <blockquote><pre>/**<br /> Set the value of the isRecordLocked attribute.<br /> @param newValue is the new isRecordLocked value.<br />*/<br />public void setIsRecordLocked(Boolean newValue) {<br /> // Update state if required.<br /> if (isRecordLocked != null) {<br /> if (newValue == null || !isRecordLocked.equals(newValue)) {<br /> context.setDirty(true);<br /> }<br /> }<br /> else if (newValue != null) {<br /> context.setDirty(true);<br /> }<br /><br /> // Change the value.<br /> isRecordLocked = newValue;<br />}</pre></blockquote>

    <p>Moral of the story? Do not try to use this code as a pick up line.</p><p>[/quote]</p><p>Poor thing. This is a typical null safe equality check in OO (if both null | ( first not null &amp; first equals second)).<br /></p><p>You could make it if ( ( isRecordLocked == newValue == null) || ( ( isRecordLocked!=null) &amp;&amp; (isRecordLocked.equals(newValue))))</p><p>But it&#39;s quite less easy to read. Two if inside each other is the most readable way. Nothing wrong with code, and if you need a pencil an paper to understand this... Wow, that&#39;s a wtf.</p><p>Clearly a code that has nothing to do here.&nbsp;</p>
  • merreborn 2006-11-29 14:14
    <p>[quote user=&quot;Anonymous&quot;]<br />Although why a language would need the Boolean class which is essentially a tri-state (true, false, null) rather than a native bool is beyond me.<br />[/quote]</p><p>Because for things like data structures, everything needs an object wrapper.&nbsp; For example, <a href="http://preview.tinyurl.com/y4x9us" target="_blank">Hashtable.put()</a> only takes objects as arguments.<br />&nbsp;</p>
  • NZ'er 2006-11-29 14:19
    [quote user=&quot;bruzie&quot;]<p>Having done several contracts for different NZ Government departments, I can make a reasonable guess of the companies involved. It&#39;s probably one of these:</p><ul><li>Inland Revenue/EDS</li><li>Accident Compensation Corporation/Unisys</li></ul><p>My experiences when working at government department clients have always been consistent when dealing with their outsourced IT partners - they&#39;ve been atrocious. Five days to get a log on, one day to get a password reset (what am I supposed to do in the meantime?). Unisys is worse - we had a Java enterprisey project and the Unisys Java Team Leader (Australian) was supposed to give us the EJB to work from - but went skiing instead, then was caught out in several lies trying to cover it up.</p><p>Grrrr.<br /></p><p>[/quote]</p><p>&nbsp;</p><p>I have to agree&nbsp; Accident Compensation Corporation/Unisys sounds like an excelent guess at this one.&nbsp; I have never had any dealings with Inland Revenue/EDS but can imagine they are not much better (or could be worse).<br /></p>
  • Paul 2006-11-29 14:22
    [quote user=&quot;Anonymous&quot;]<p>Attempt at the one liner (Although, this is about as readable as the way he did it.)</p><p>&nbsp;&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; if(<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; (isRecordLocked != null &amp;&amp; !isRecordLocked.equals(newValue))&nbsp; ||<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; (newValue != null &amp;&amp; !newValue.equals(isRecordLocked)) ||<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; ((isRecordLocked = newValue).valueOf(&quot;false&quot;))<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; )<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp; {<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; context.setDirty(true);<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp; }<br /><br />&nbsp;</p><p>basically, context.setDirty(true) only occurs if isRecordLocked != newValue.&nbsp; I put two comparisons in the if statement, so that whichever one wasn&#39;t null would be compared to the other, and then verified inequality.&nbsp; I didn&#39;t put in a case for when they&#39;re both null, because it&#39;s redundant.</p><p>I then added a third line which nests the isRecordLocked = newValue into the if statement (thereby having a &quot;one-liner&quot; with two seperate assignment statements - cheap workarounds, ahoy!), using the ().valueOf(&quot;false&quot;) because that&#39;s the only way the compiler would allow that assignment inside the conditional area.&nbsp; I designed it so the overall condition is if (&quot;we should execute this code&quot;) || false ) so that the assignment at the end didn&#39;t change the logic for going into the if statement&#39;s code block.</p><p>-Alex&nbsp;</p><p>[/quote]</p><p>Two problems.</p><p>1. You used &quot;|| ((isRecordLocked = newValue).valueOf(&quot;false&quot;))&quot; -- a conditional OR operator. This means the assignment never takes place if the dirty flag is going to be set.</p><p>2. Even if you fix #1 by using a regular OR (single pipe &#39;|&#39; ), order of operations is changed -- setDirty() is called after the
    assignments, which may or may not affect the code. This is an iffy point.</p><p>Two lines :</p><p>
    for (int i = 0; (i++ == 0) &amp;&amp; ((isRecordLocked == null)?(newValue != null):!isRecordLocked.equals(newValue)); context.setDirty(true));<br />isRecordLocked = newValue;<br />
    </p><p>One line, ignoring order of operations:</p><p>
    for (int i = 0; ((i++ == 0) &amp;&amp; ((isRecordLocked == null)?(newValue != null):!isRecordLocked.equals(newValue))) | ((isRecordLocked = newValue) != newValue); context.setDirty(true));
    &nbsp;</p><p>&nbsp;</p>
  • AdT 2006-11-29 14:23

    <p>[quote user=&quot;Anonymous&quot;]This forum really is a haven for complete idiots. Only about three of the commenters actually understood what this code was doing (and I suspect the submitter didn&#39;t either). When they are going to so much effort to support tri-value booleans they probably have a reason for it. This code is slightly ugly but is not broken, unlike most of the suggested replacements.[/quote]<br />
    <br />
    So true, and tragicomically underlined by the fact that your own solution doesn&#39;t work either. Reading through all the responses brought me to the conclusion that 80% of the baboons that post to this forum wouldn&#39;t be able to peel a banana while blindfolded.<br />
    <br />
    In fact, the few who&#39;re smarter than the rest have illustrated that (non-standard extension methods aside) there just isn&#39;t any way to accomplish what the cited function accomplishes in a true one-liner, unless you&#39;re going to make that line much longer than code readability guidelines recommend, flat out contradicting the submitter&#39;s claims. This, together with the <strong>correctness</strong> of the original code means The Real WTF is that this function ever made it to the &quot;CodeSOD&quot; at all. It is not perfect but far from a WTF. If this is the worst code in the entire project, I commend the developers for the excellent work.<br />
    <br />
    Most of the suggested replacements, on the other hand, show that the submitter makes dangerous assumptions about the evaluated objects, or they are <strong>plain wrong</strong> even under the most favorable conditions. Maybe I&#39;m too old school, but as someone who&#39;s allowed himself to be much more influenced by Donald &quot;It works&quot; Knuth than by Bill &quot;It looks good and sells&quot; Gates, I&#39;m absolutely convinced that correct and slightly convoluted code is infinitely preferrable to elegant and incorrect solutions. Unfortunately, that once more seems to make me part of a minority.</p>
  • Paul 2006-11-29 14:24
    Oh, and a third problem -- if newValue is null, you&#39;d throw a NullPointerException<br />
  • obediah 2006-11-29 14:24
    [quote user=&quot;Ottokar&quot;][quote user=&quot;Anonymous&quot;]<p>Makes perfect sense to me, I cant see a third value in the Boolean object no matter how hard i look.</p><p>[/quote]</p><p>A simple example:</p><p>You have a database. The DB has tables. A table can have booleans. And they can have three states (NULL, true, false). So your system has to handle the tristate DB-Values. </p><p>[/quote]</p><p>&nbsp;<br />And their was much gnashing of teeth and wrenching of hands and pounding of foreheads. And Obediah looked up and at his meakest spaketh, </p><p>&quot;Oh, Great Lord - our crops whither and our people starve. We endeareth to make pleasure for thine eyes and spirit, but our booleans are sinful, broken creatures. All is well when they speak yay or nay, but their silence bringeth forth exception and behaviour most undefined.&quot;<br /></p><p>And the Lord looked down on pitiful Obediah and his burning lands. He knew the people of earth were good and granted them a respite from this one trial - he granted them the gift of &#39;NOT NULL&#39;. And there was much rejoicing and singing and sighs to acknowledging god&#39;s infinite wisdom.&nbsp; Except for some MySQL jerks.<br /></p><p>&nbsp;</p><p>&nbsp;</p>
  • VGR 2006-11-29 14:26
    [quote user=&quot;Anonymous&quot;]
    <p>[quote user=&quot;Anonymous&quot;]First ObjectUtils already exists in the jakarta commons-lang library, and it is better to reuse that than write it yourself.<br />
    [/quote]</p>
    <p>Not neccessarily.&nbsp; For one thing, we&#39;ve been bit on the ass here numerous times by shoddy, amateurish code from the jakarta commons project (funny how so many shitty hackers are giving their work away for free).&nbsp; For another, we may not want an entire library to be required everywhere our object goes (let&#39;s say it&#39;s a web service, or an applet, or J2ME).&nbsp; Finally, seems like a whole lot of fucking work just to test a handful of local booleans.<br />
    &nbsp;</p>
    <p>[/quote]</p>
    <p>I agree.&nbsp; Every external dependency greatly reduces the portability and usability of one&#39;s code.&nbsp; Every extra jar an app needs is going to lower my enthusiasm for using the thing, and it doesn&#39;t take many before I decide it&#39;s just not worth the bother at all.&nbsp; (This also applies to many open source projects, both Java and non-Java.)&nbsp; It&#39;s another jar to have in the classpath, another API to become familiar with (if it even has javadoc), and some cases, the dependency on it is weaved into the code base and can never be removed without a sweeping rewrite.<br />
    </p>
    <p>So, knowing that an external dependency carries such a high cost, what is the benefit you get in exchange for taking such a hit?&nbsp; A class with an embarrassingly bad name (&quot;ObjectUtils&quot; is its own WTF, in my opinion), and a method that does something only slightly more complex than adding two numbers.</p>
    <p>I would much rather put &#39;<code>private static boolean equals(Object o1, Object o2) { return o1 == null ? (o2 == null) : o1.equals(o2); }</code>&#39; in multiple classes than be saddled with dependency on a third-party jar for something so trivial.</p>
    <p>And I agree that many of Jakarta&#39;s offerings, particularly their Java offerings, are sloppy and unreliable.&nbsp; I like the web server, and I like ant, but that doesn&#39;t give the other projects a free pass.&nbsp; I have a strong dislike of the Jakarta Commons in particular.</p>

  • Paul 2006-11-29 14:35
    [quote user=&quot;obediah&quot;][quote user=&quot;Ottokar&quot;][quote user=&quot;Anonymous&quot;]<p>Makes perfect sense to me, I cant see a third value in the Boolean object no matter how hard i look.</p><p>[/quote]</p><p>A simple example:</p><p>You have a database. The DB has tables. A table can have booleans. And they can have three states (NULL, true, false). So your system has to handle the tristate DB-Values. </p><p>[/quote]</p><p>&nbsp;<br />And their was much gnashing of teeth and wrenching of hands and pounding of foreheads. And Obediah looked up and at his meakest spaketh, </p><p>&quot;Oh, Great Lord - our crops whither and our people starve. We endeareth to make pleasure for thine eyes and spirit, but our booleans are sinful, broken creatures. All is well when they speak yay or nay, but their silence bringeth forth exception and behaviour most undefined.&quot;<br /></p><p>And the Lord looked down on pitiful Obediah and his burning lands. He knew the people of earth were good and granted them a respite from this one trial - he granted them the gift of &#39;NOT NULL&#39;. And there was much rejoicing and singing and sighs to acknowledging god&#39;s infinite wisdom.&nbsp; Except for some MySQL jerks.<br /></p><p>&nbsp;[/quote]</p><p>NOT NULL doesn&#39;t handle the problem, it skips them, which is different.</p><p>Here&#39;s an example: You have a Boolean field specifying an option. You can set it to Boolean.TRUE or Boolean.FALSE to explicitly set the option, or use null to indicate a default or inherited value.<br /></p>
  • FrostCat 2006-11-29 14:37
    <p>[quote user=&quot;Anonymous&quot;]<br />Although why a language would need the Boolean class which is essentially a tri-state (true, false, null) rather than a native bool is beyond me.<br />[/quote]</p><p>&nbsp;</p><p>All right, now stop that.&nbsp; Nullable variables are actually useful, as long as you understand what they&#39;re for.</p>
  • FrostCat 2006-11-29 14:37
    <p>[quote user=&quot;Anonymous&quot;]<br />Although why a language would need the Boolean class which is essentially a tri-state (true, false, null) rather than a native bool is beyond me.<br />[/quote]</p><p>&nbsp;</p><p>All right, now stop that.&nbsp; Nullable variables are actually useful, as long as you understand what they&#39;re for.</p>
  • obediah 2006-11-29 15:03
    [quote user=&quot;Anonymous&quot;]<p>NOT NULL doesn&#39;t handle the problem, it skips them, which is different.</p><p>Here&#39;s an example: You have a Boolean field specifying an option. You can set it to Boolean.TRUE or Boolean.FALSE to explicitly set the option, or use null to indicate a default or inherited value.<br /></p><p>[/quote]</p><p>I wasn&#39;t attempting to speak to the Java Object issue, only the comment of null boolean support being needed in a programming language to handle null values in booleans taken from a database. NOT NULL nullifies that concern. :) </p><p>&nbsp;FWIW, I&#39;ve never seen a situation where hacking a third state into a db boolean via NULL was the best option. I haven&#39;t seen all situations - but it&#39;s something I&#39;d have to think about long and hard before implementing.<br /><br />&nbsp;</p>
  • Thuktun 2006-11-29 15:24

    <p>Sheesh. A null reference in this case may indicating that a value has
    not yet been specified by anyone and needs to be specified before something else
    occurs.</p>

    <p>The value is changed if (1) the reference is different, (2)
    becomes null, (3) becomes non-null, or (4) becomes something
    different. Leveraging the fact that Object.equals is supposed to
    return false when passed a null and omit (2), we can bundle checks (2) and (4) together.</p>

    <pre>public void setIsRecordLocked(Boolean newValue)
    {
    // Update state if required.
    context.setDirty(
    isRecordLocked != newValue || // (1)
    isRecordLocked == null || // (3)
    !isRecordLocked.equals( newValue ) ); // (2) and (4)

    // Change the value.
    isRecordLocked = newValue;
    }</pre>
    <p>The original quoted code is not incorrect, it&#39;s just overly verbose.</p>
    <p>(The Real WTF (tm) is the HTML that this forum editor produces!)</p>
  • Fred Flintstone 2006-11-29 15:29
    [quote user=&quot;Anonymous&quot;]<p>Anonymous wrote:</p><p style="margin-left: 40px">if (isRecordLocked != newValue)</p><p>No! In Java, Boolean is a class, not a simple type like bool, so:</p><ol><li>The != operator tests whether the two object references refer to the same object; it does not check whether the two object references refer to objects with the same value.</li><li>Object references might be null, which you don&#39;t account for.</li></ol><p>Someone has already beaten me to suggest the following replacement:<br /></p><p style="margin-left: 40px">if (isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))</p><p>&nbsp;</p><p>[/quote]</p><p>if ((isRecordLocked != newValue) || isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue)) </p><p>Adding in the direct comparison of the references handles the case of both being null.&nbsp;</p>
  • aakoch 2006-11-29 15:35

    <p>[quote user=&quot;Anonymous&quot;]Oh, and a third problem -- if newValue is null, you&#39;d throw a NullPointerException<br />
    [/quote]</p>

    <p>No, it doesn&#39;t.</p>

    <p>Here&#39;s my version:</p>

    <pre>/**
    Set the value of the isRecordLocked attribute.
    @param newValue is the new isRecordLocked value.
    */
    public void setIsRecordLocked(Boolean newValue) {
    // Update state if required.
    if (isRecordLocked == null &amp;&amp; newValue == null) {
    // do nothing
    }
    else if (isRecordLocked != null &amp;&amp; newValue == null) {
    context.setDirty(true);
    }
    else if (isRecordLocked == null &amp;&amp; newValue != null) {
    context.setDirty(true);
    }
    else if (!isRecordLocked.equals(newValue)) {
    context.setDirty(true);
    }
    // Change the value.
    isRecordLocked = newValue;
    }
    </pre>
    <br />
    Clean and understandable. Any objections?<br />
  • Paul 2006-11-29 15:37
    [quote user=&quot;Thuktun&quot;]<p>Sheesh. A null reference in this case may indicating that a value has
    not yet been specified by anyone and needs to be specified before something else
    occurs.</p>

    <p>The value is changed if (1) the reference is different, (2)
    becomes null, (3) becomes non-null, or (4) becomes something
    different. Leveraging the fact that Object.equals is supposed to
    return false when passed a null and omit (2), we can bundle checks (2) and (4) together.</p>

    <pre>public void setIsRecordLocked(Boolean newValue)<br />{<br /> // Update state if required.<br /> context.setDirty(<br /> isRecordLocked != newValue || // (1)<br /> isRecordLocked == null || // (3)<br /> !isRecordLocked.equals( newValue ) ); // (2) and (4)<br /><br /> // Change the value.<br /> isRecordLocked = newValue;<br />}</pre>
    <p>The original quoted code is not incorrect, it&#39;s just overly verbose.</p>
    <p>(The Real WTF (tm) is the HTML that this forum editor produces!)</p><p>[/quote]</p><p>As has been pointed out before, calling setDirty(false) is WRONG. Practical reason why? Because if you call setIsRecordLocked twice in a row with the same value, the dirty flags is set to true, and then false, which is very likely wrong.<br /></p>
  • Paul 2006-11-29 15:40
    [quote user=&quot;aakoch&quot;]<p>[quote user=&quot;Anonymous&quot;]Oh, and a third problem -- if newValue is null, you&#39;d throw a NullPointerException<br />
    [/quote]</p>

    <p>No, it doesn&#39;t.</p><p>[/quote]</p><p>Yes it does. You missed this part:</p><p>[quote]if(<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; (isRecordLocked != null &amp;&amp; !isRecordLocked.equals(newValue))&nbsp; ||<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; (newValue != null &amp;&amp; !newValue.equals(isRecordLocked)) ||<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; ((isRecordLocked = newValue).valueOf(&quot;false&quot;))<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; )[/quote]</p><p>If newValue is null, isRecordLocked is set to null, and then .valueOf() is called on it.&nbsp;</p>
  • aakoch 2006-11-29 15:40
    [quote user=&quot;Thuktun&quot;]<p>Sheesh. A null reference in this case may indicating that a value has
    not yet been specified by anyone and needs to be specified before something else
    occurs.</p>

    <p>The value is changed if (1) the reference is different, (2)
    becomes null, (3) becomes non-null, or (4) becomes something
    different. Leveraging the fact that Object.equals is supposed to
    return false when passed a null and omit (2), we can bundle checks (2) and (4) together.</p>

    <pre>public void setIsRecordLocked(Boolean newValue)<br />{<br /> // Update state if required.<br /> context.setDirty(<br /> isRecordLocked != newValue || // (1)<br /> isRecordLocked == null || // (3)<br /> !isRecordLocked.equals( newValue ) ); // (2) and (4)<br /><br /> // Change the value.<br /> isRecordLocked = newValue;<br />}</pre>
    <p>The original quoted code is not incorrect, it&#39;s just overly verbose.</p>
    <p>(The Real WTF (tm) is the HTML that this forum editor produces!)</p><p>[/quote]</p><p>This doesn&#39;t work. Try running it.</p><p>Also, you don&#39;t want to set dirty to false if somewhere else already set it to true.&nbsp;</p>
  • aakoch 2006-11-29 15:41
    [quote user=&quot;Anonymous&quot;][quote user=&quot;aakoch&quot;]<p>[quote user=&quot;Anonymous&quot;]Oh, and a third problem -- if newValue is null, you&#39;d throw a NullPointerException<br />
    [/quote]</p>

    <p>No, it doesn&#39;t.</p><p>[/quote]</p><p>Yes it does. You missed this part:</p><p>[quote]if(<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; (isRecordLocked != null &amp;&amp; !isRecordLocked.equals(newValue))&nbsp; ||<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; (newValue != null &amp;&amp; !newValue.equals(isRecordLocked)) ||<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; ((isRecordLocked = newValue).valueOf(&quot;false&quot;))<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; )[/quote]</p><p>If newValue is null, isRecordLocked is set to null, and then .valueOf() is called on it.&nbsp;</p><p>[/quote]</p><p>Sorry. Thought you were talking about the original post. I wasn&#39;t following the discussion closely enough. My apologies.<br /></p>
  • Paul 2006-11-29 15:43
    [quote user=&quot;Anonymous&quot;][quote user=&quot;aakoch&quot;]<p>[quote user=&quot;Anonymous&quot;]Oh, and a third problem -- if newValue is null, you&#39;d throw a NullPointerException<br />
    [/quote]</p>

    <p>No, it doesn&#39;t.</p><p>[/quote]</p><p>Yes it does. You missed this part:</p><p>[quote]if(<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; (isRecordLocked != null &amp;&amp; !isRecordLocked.equals(newValue))&nbsp; ||<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; (newValue != null &amp;&amp; !newValue.equals(isRecordLocked)) ||<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; ((isRecordLocked = newValue).valueOf(&quot;false&quot;))<br />&nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; &nbsp;&nbsp;&nbsp; )[/quote]</p><p>If newValue is null, isRecordLocked is set to null, and then .valueOf() is called on it.&nbsp;</p><p>[/quote]</p><p>Oops, you&#39;re right - valueOf() is a static method. Bad form, but valid Java.&nbsp;</p>
  • mrprogguy 2006-11-29 16:13
    [quote user=&quot;Anonymous&quot;] <p>Anonymous wrote:</p><p style="margin-left: 40px">if (isRecordLocked != newValue)</p><p>No! In Java, Boolean is a class, not a simple type like bool, so:</p><ol start="1"><li>The != operator tests whether the two object references refer to the same object; it does not check whether the two object references refer to objects with the same value.</li><li>Object references might be null, which you don&#39;t account for.</li></ol><p>Someone has already beaten me to suggest the following replacement:<br /></p><p style="margin-left: 40px">if (isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))</p><p>&nbsp;</p><p>[/quote] </p><p>&nbsp;</p><p>Just another in a long, long, list of reasons not to use Java.&nbsp; Ever.&nbsp; For anything.</p>
  • aakoch 2006-11-29 16:25
    [quote user=&quot;mrprogguy&quot;]<p>&nbsp;Just another in a long, long, list of reasons not to use Java.&nbsp; Ever.&nbsp; For anything.</p><p>[/quote]</p><p>You can use &quot;boolean&quot; if you like. It&#39;s not a class.&nbsp;</p>
  • JB 2006-11-29 16:28
    [quote user=&quot;Anonymous&quot;]<p>This forum really is a haven for complete idiots. Only about three of the commenters actually understood what this code was doing (and I suspect the submitter didn&#39;t either). When they are going to so much effort to support tri-value booleans they probably have a reason for it. This code is slightly ugly but is not broken, unlike most of the suggested replacements.</p><p>For anyone using Java who needs to compare possibly null objects may I suggest Jakarta commons-lang which include the Object.equals methods in a previous comment as ObjectUtils.equals (along which a bunch of other useful stuff which probably should be in Java - and some complete crap of course).</p><p>&nbsp;</p><p>My solution would be to change the Context class:</p><p>class Context {</p><p>&nbsp;public void setDirtyIfDifferent(Object obj1, Object obj2) {</p><p>&nbsp;&nbsp;&nbsp; setDirty(!ObjectUtils.equals(obj1, obj2));<br /></p><p>}&nbsp;</p><p>}</p><p>&nbsp;then the original method could be:<br /></p><pre>public void setIsRecordLocked(Boolean newValue) {<br /> context.setDirtyIfDifferent(newValue, isRecordLocked);<br /><br /> // Change the value.<br /> isRecordLocked = newValue;<br />}</pre><p>[/quote]</p><p>&nbsp;</p><p>Could not agree more. Never before have I seen so many totally incompetent people gathering together over a piece of code, trying to be &quot;clever&quot;, not having the slightest idea of what they are talking about.</p><p>&nbsp;</p>
  • obediah 2006-11-29 16:32
    [quote user=&quot;Anonymous&quot;]<br /><p>Could not agree more. Never before have I seen so many totally incompetent people gathering together over a piece of code, trying to be &quot;clever&quot;, not having the slightest idea of what they are talking about.<br />[/quote]</p><p>You must not have a CS degree.<br />&nbsp;</p>
  • anon 2006-11-29 16:37

    <p>This is actually longer but may be more clear (or not):</p>
    <blockquote><code></code><pre>return (isRecordLocked==null) != (newValue==null)
    ||
    isRecordLocked!=null <font color="green">// prevent null pointer exception</font>
    &amp;&amp; !isRecordLocked.equals(newValue);
    </pre></blockquote>
    <p>Or here&#39;s another way:</p>
    <blockquote><code></code><pre>Object o1 = (isRecordLocked==null ? &quot;null&quot; : isRecordLocked);
    Object o2 = (newValue==null ? &quot;null&quot; : newValue);
    return !o1.equals(o2);
    </pre></blockquote>

  • ItIsGreen 2006-11-29 16:40
    [quote user=&quot;Anonymous&quot;]<p>[quote user=&quot;Anonymous&quot;]This forum really is a haven for complete idiots. Only about three of the commenters actually understood what this code was doing (and I suspect the submitter didn&#39;t either). When they are going to so much effort to support tri-value booleans they probably have a reason for it. This code is slightly ugly but is not broken, unlike most of the suggested replacements.[/quote]<br />
    <br />
    So true, and tragicomically underlined by the fact that your own solution doesn&#39;t work either. Reading through all the responses brought me to the conclusion that 80% of the baboons that post to this forum wouldn&#39;t be able to peel a banana while blindfolded.<br />
    <br />
    In fact, the few who&#39;re smarter than the rest have illustrated that (non-standard extension methods aside) there just isn&#39;t any way to accomplish what the cited function accomplishes in a true one-liner, unless you&#39;re going to make that line much longer than code readability guidelines recommend, flat out contradicting the submitter&#39;s claims. This, together with the <strong>correctness</strong> of the original code means The Real WTF is that this function ever made it to the &quot;CodeSOD&quot; at all. It is not perfect but far from a WTF. If this is the worst code in the entire project, I commend the developers for the excellent work.<br />
    <br />
    Most of the suggested replacements, on the other hand, show that the submitter makes dangerous assumptions about the evaluated objects, or they are <strong>plain wrong</strong> even under the most favorable conditions. Maybe I&#39;m too old school, but as someone who&#39;s allowed himself to be much more influenced by Donald &quot;It works&quot; Knuth than by Bill &quot;It looks good and sells&quot; Gates, I&#39;m absolutely convinced that correct and slightly convoluted code is infinitely preferrable to elegant and incorrect solutions. Unfortunately, that once more seems to make me part of a minority.</p><p>[/quote]</p><p>&nbsp;</p><p>I have to agree here. It took the original coder two minutes to write the code; it takes a decent coder two minutes to understand the code. And it works. There&#39;s nothing complicated about it. Instead of wasting time trying to optimize this code to some optimal one liner, I hope the original programmer spent his time working on more substantial things.</p><p>&nbsp;</p><p>&nbsp;</p>
  • emurphy 2006-11-29 16:53
    [quote user=&quot;Anonymous&quot;]<blockquote><pre>public void setIsRecordLocked(Boolean newValue) {<br /> if (!(isRecordLocked == null ?<br /> isRecordLocked == newValue :<br /> isRecordLocked.equals(newValue))) {<br /> context.setDirty(true);<br /> }<br />}</pre></blockquote>
    <p>Simple, effective, and makes the intention much clearer.</p><p>Note that the &quot;isRecordLocked == newValue&quot; could just as easily be &quot;newValue == null&quot; but using &quot;isRecordLocked == newValue&quot; makes the intention clearer.<br /></p><p>Null handling like the above comes up all the time in Java and the above pattern is repeated all over the Sun Java library implementation.&nbsp;</p><p>[/quote]</p><p>&nbsp;</p><p>It&#39;s still non-trivial to understand, but once you do understand it, it&#39;s actually elegant.<br /></p><p>&nbsp;</p><p>[quote user=&quot;VGR&quot;]</p><p><code>private static boolean equals(Object o1, Object o2) { return o1 == null ? (o2 == null) : o1.equals(o2); }</code></p><p>[/quote]</p><p>&nbsp;</p><p>This <em>almost</em> abstracts the elegance of the above.&nbsp; I would prefer</p><p>&nbsp;</p><p><code>private static boolean equals(Object o1, Object o2) { return o1 == null ? (o1 == o2) : o1.equals(o2); }</code></p><p>&nbsp;</p>
  • PseudoNoise 2006-11-29 16:57
    <p>OK, I&#39;ll hold myself up for ridicule.&nbsp; I wrote out the truth table: 9 entries, with newValue and isRecordLocked being 0, 1 or NULL.&nbsp; &quot;context.SetDirty (true)&quot; is called everywhere except the middle diagonal.&nbsp; If both inputs are null, or if the inputs have equal value, then &quot;context.SetDirty(true)&quot; is not called.</p><p>My suggestion would be split off the no-op cases and judiciously comment:<br /></p><p>if (newValue == null &amp;&amp; isRecordLocked == null)</p><p>&nbsp;&nbsp;&nbsp; return; // take no action if both are null<br /></p><p>if (newValue != null &amp;&amp; isRecordLocked != null &amp;&amp; isRecordLocked.equals (newValue))</p><p>&nbsp;&nbsp;&nbsp; return; // take no action if both inputs are non-null and are equal</p><p>// fall-through: variables differ, so set the dirty flag and update isRecordLocked</p><p>context.setDirty(true);</p><p>isRecordLocked = newValue;</p><p>&nbsp;</p><p>Granted, this differs from the example code itself because it only updates isRecordLocked when required, but it does follow the comment in the original code.</p><p>&nbsp;I don&#39;t see how a one-liner would be warranted here since equals can&#39;t be called on a null source or target.&nbsp; Caveat: I don&#39;t know Java, only C/C++.<br />&nbsp; </p><p><br /><br />&nbsp;</p>
  • Paul 2006-11-29 17:17
    [quote user=&quot;Anonymous&quot;] <p>OK, I&#39;ll hold myself up for ridicule.&nbsp; I wrote out the truth table: 9 entries, with newValue and isRecordLocked being 0, 1 or NULL.&nbsp; &quot;context.SetDirty (true)&quot; is called everywhere except the middle diagonal.&nbsp; If both inputs are null, or if the inputs have equal value, then &quot;context.SetDirty(true)&quot; is not called.</p><p>My suggestion would be split off the no-op cases and judiciously comment:<br /></p><p>if (newValue == null &amp;&amp; isRecordLocked == null)</p><p>&nbsp;&nbsp;&nbsp; return; // take no action if both are null<br /></p><p>if (newValue != null &amp;&amp; isRecordLocked != null &amp;&amp; isRecordLocked.equals (newValue))</p><p>&nbsp;&nbsp;&nbsp; return; // take no action if both inputs are non-null and are equal</p><p>// fall-through: variables differ, so set the dirty flag and update isRecordLocked</p><p>context.setDirty(true);</p><p>isRecordLocked = newValue;</p><p>&nbsp;</p><p>Granted, this differs from the example code itself because it only updates isRecordLocked when required, but it does follow the comment in the original code.</p><p>&nbsp;I don&#39;t see how a one-liner would be warranted here since equals can&#39;t be called on a null source or target.&nbsp; Caveat: I don&#39;t know Java, only C/C++.<br />&nbsp; </p><p>[/quote]</p><p>Close, but there is a difference -- whether it is significant can&#39;t be inferred from the code snippet. The condition &quot;if (newValue != null &amp;&amp; isRecordLocked != null &amp;&amp; isRecordLocked.equals (newValue))&quot; should set &quot;isRecordLocked = newValue;&quot; before returning, because although they may be .equals() (equal logical value), they may not be == (same reference).</p>
  • emurphy 2006-11-29 17:21
    [quote user=&quot;Anonymous&quot;]<p>I don&#39;t see how a one-liner would be warranted here since equals can&#39;t be called on a null source or target.&nbsp; Caveat: I don&#39;t know Java, only C/C++.<br />[/quote]</p><p>&nbsp;</p><p>equals() can&#39;t be called on a null source, but you can pass a null target to it.</p><p>&nbsp;</p>
  • newfweiler 2006-11-29 17:21
    <p>Am I the frist* to notice that this code is not thread-safe?</p><p>Suppose a maintenance thread is triggered by setDirty(true) to write out the changed data.&nbsp; (For that matter, suppose it&#39;s a single-threaded application and setDirty(true) writes out the changed data and then returns.)&nbsp; Dirty is then set false.&nbsp; The assignment to isRecordLocked could happen after that.</p><p>*Frist:&nbsp; An expert who makes a diagnosis based on scanty data.&nbsp; After Dr. Bill Frist, United States Senator (R., Tennessee) who diagnosed Terry Sciavo by watching a video.</p><p>&nbsp;</p>
  • Paul 2006-11-29 17:28
    [quote user=&quot;newfweiler&quot;] <p>Am I the frist* to notice that this code is not thread-safe?</p><p>Suppose a maintenance thread is triggered by setDirty(true) to write out the changed data.&nbsp; (For that matter, suppose it&#39;s a single-threaded application and setDirty(true) writes out the changed data and then returns.)&nbsp; Dirty is then set false.&nbsp; The assignment to isRecordLocked could happen after that.[/quote]</p><p>For all we know, the synchronization/locking could be done outside of the code. It&#39;s beyond the scope of the snippet.</p>
  • BA 2006-11-29 17:31
    [quote user=&quot;Anonymous&quot;]
    <p>OK, I&#39;ll hold myself up for ridicule.&nbsp; I wrote out the truth table: 9 entries, with newValue and isRecordLocked being 0, 1 or NULL.&nbsp; &quot;context.SetDirty (true)&quot; is called everywhere except the middle diagonal.&nbsp; If both inputs are null, or if the inputs have equal value, then &quot;context.SetDirty(true)&quot; is not called.</p>
    <p>My suggestion would be split off the no-op cases and judiciously comment:<br />
    </p>
    <p>if (newValue == null &amp;&amp; isRecordLocked == null)</p>
    <p>&nbsp;&nbsp;&nbsp; return; // take no action if both are null<br />
    </p>
    <p>if (newValue != null &amp;&amp; isRecordLocked != null &amp;&amp; isRecordLocked.equals (newValue))</p>
    <p>&nbsp;&nbsp;&nbsp; return; // take no action if both inputs are non-null and are equal</p>
    <p>// fall-through: variables differ, so set the dirty flag and update isRecordLocked</p>
    <p>context.setDirty(true);</p>
    <p>isRecordLocked = newValue;</p>
    <p>&nbsp;</p>
    <p>Granted, this differs from the example code itself because it only updates isRecordLocked when required, but it does follow the comment in the original code.</p>
    <p>&nbsp;I don&#39;t see how a one-liner would be warranted here since equals can&#39;t be called on a null source or target.&nbsp; Caveat: I don&#39;t know Java, only C/C++.</p>
    <p>[/quote]</p>
    <p>I think we can step it up a bit.</p>
    <p>You want to return if either condition happens, so:</p>
    <p>if ((newValue == null &amp;&amp; isRecordLocked == null) ||<br />
    &nbsp;&nbsp;&nbsp; (newValue != null &amp;&amp; isRecordLocked != null &amp;&amp; isRecordLocked.equals (newValue)))<br />
    &nbsp; return;</p>
    <p>That&#39;s the first step we can perform. Next, we are effectively calling setDirty if all of this is not true. So we can instead do this:</p>
    <p>if (!((newValue == null &amp;&amp; isRecordLocked == null) ||<br />
    &nbsp;&nbsp;&nbsp;&nbsp;&nbsp; (newValue != null &amp;&amp; isRecordLocked != null &amp;&amp; isRecordLocked.equals (newValue))))<br />
    &nbsp;&nbsp; setDirty(true);</p>
    <p>isRecordLocked = newValue is always performed, regardless of the value of newValue. The comments bear this out. It says that the isRecordLocked is changed to the value of newValue without imposing any conditions on the assignment. The only conditional is to whether or not to mark it &quot;dirty&quot;. And basically, it only wants to mark it &quot;dirty&quot; if the value of isRecordLocked changes. And even still, I don&#39;t believe assignment to the same value is going to make that much of a difference, so in or out of the if block is largely semantic as assignment should be trivial. (Unless, we are assigning the whole newValue object to isRecordLocked, in which case, I&#39;d say always perform the assignment as the two objects are probably always different and it may affect something else if it doesn&#39;t happen. (You can tell I have the whole, &quot;Don&#39;t change anything you have the slightest doubt over&quot; mentality)).<br />
    </p>
    <p>I believe a cleaner implementation would have it always mark &quot;dirty&quot;. Unless the &quot;cleansing code&quot; is a bottleneck and this whole &quot;dirty marker&quot; was used to create conditional execution. In that case, there is probably a better way to solve the performance issue than to code around the problem.<br />
    </p>
  • Yet another Kiwi 2006-11-29 17:33
    [quote user=&quot;Anonymous&quot;][quote user=&quot;bruzie&quot;]<p>Having done several contracts for different NZ Government departments, I can make a reasonable guess of the companies involved. It&#39;s probably one of these:</p><ul><li>Inland Revenue/EDS</li><li>Accident Compensation Corporation/Unisys</li></ul><p>My experiences when working at government department clients have always been consistent when dealing with their outsourced IT partners - they&#39;ve been atrocious. Five days to get a log on, one day to get a password reset (what am I supposed to do in the meantime?). Unisys is worse - we had a Java enterprisey project and the Unisys Java Team Leader (Australian) was supposed to give us the EJB to work from - but went skiing instead, then was caught out in several lies trying to cover it up.</p><p>Grrrr.<br /></p><p>[/quote]</p><p>&nbsp;</p><p>I have to agree&nbsp; Accident Compensation Corporation/Unisys sounds like an excelent guess at this one.&nbsp; I have never had any dealings with Inland Revenue/EDS but can imagine they are not much better (or could be worse).<br /></p><p>[/quote]

    </p><p>I&#39;m going to guess at Ministry of Economic Development/Hewlett Packard. </p><p>www.companies.govt.nz - Witness the amazing fusion of Plone and J2EE.&nbsp;</p>
  • lilo_booter 2006-11-29 17:57
    Well said :-).<br />
  • Anonymous 2006-11-29 18:12
    [quote user=&quot;Anonymous&quot;]<blockquote><pre>public void setIsRecordLocked(Boolean newValue) {<br /> if (!(isRecordLocked == null ?<br /> isRecordLocked == newValue :<br /> isRecordLocked.equals(newValue))) {<br /> context.setDirty(true);<br /> }<br />}</pre></blockquote>
    <p>Simple, effective, and makes the intention much clearer.</p><p>Note that the &quot;isRecordLocked == newValue&quot; could just as easily be &quot;newValue == null&quot; but using &quot;isRecordLocked == newValue&quot; makes the intention clearer.<br /></p><p>Null handling like the above comes up all the time in Java and the above pattern is repeated all over the Sun Java library implementation.&nbsp;</p><p>[/quote]</p><p>Finally, a sane replacement that actually works!&nbsp; I would think that<br /></p><pre> if (!(isRecordLocked == null ? newValue == null : isRecordLocked.equals(newValue))) context.setDirty(true);</pre><pre>would be clearer, but that&#39;s just me.</pre><p>&nbsp;</p>
  • verisimilidude 2006-11-29 18:13
    [quote user=&quot;Wiggles&quot;][quote user=&quot;Goplat&quot;] <p>If you compare Boolean (or any reference type, actually) with != you are comparing the references, not the object contents like the original function does.<br /></p><p>[/quote]</p><p>&nbsp;This makes the assumption that recordLocked is a Boolean.&nbsp; If it&#39;s of the primitive type boolean and Java 1.5 is being used then autoboxing will come into effect and&nbsp;simple object equality == would work fine.</p><p>[/quote]</p><p>&nbsp;In Java there are just two possible Boolean&#39;s, one wraps true and the other wraps false.&nbsp; Yes, you are comparing references but since there are only two == and != work fine.<br />&nbsp;</p>
  • chrismcb 2006-11-29 18:17
    [quote user=&quot;Anonymous&quot;] <blockquote><p>Note that the &quot;isRecordLocked == newValue&quot; could just as easily be &quot;newValue == null&quot; but using &quot;isRecordLocked == newValue&quot; makes the intention clearer.</p><p>[/quote]</p><p>&nbsp;Yeah&nbsp;I generally write A == B all the time when I really mean&nbsp; B == null, because A == B makes my intentions that I am comparing B to NULL very clear. Clear as in clear as mud.</p><p>You know I realize what you are saying. If you have &quot;if B == null then if B == A...&quot;, then its the same as &quot;if B == null then if null == A...&quot; But its not CLEARER, its muddier. When you are reading the statement B == A you have to realize you are in the portion where B == null case. WHY make the reader work harder when you can just explictly spell it out?</p></blockquote>
  • chrismcb 2006-11-29 18:38
    <p>I&#39;m amazed (although I guess I shouldn&#39;t be.) I assumed that most of the readers here were above some of the nonsense that shows up on thedailwtf, instead I see many of the readers here are boning up on trying to write their own contributions.</p><p>&nbsp;Some people claim this isn&#39;t a WTF, and its simple, yet there are almost 200 comments about it.</p><p>Some people write &quot;clearer&quot; code that is more difficult to read than the original.</p><p>Some people don&#39;t understand that the original isn&#39;t wrong, it just isn&#39;t clear.</p><p>Most people can&#39;t seem to understand how to write this correctly (even if the code is messier than the original)</p><p>A bunch of people offered &quot;clearer&quot; code that sets the dirty flag more times than the original does. And some point out that there is nothing wrong with the original. You do realize that you should never have the exact same code in two different part of the conditionals (except in extreme cases) Simplify/rewrite the conditional to lead to the same statement. In this particular case the line of code being executed was a simple assignment. But what if you needed to add a second statement. Now you have to do it in two places (or 4 in one &quot;clearer&quot; example) if there is a bug you have to fix it in two places (or probably fix it in one, forget or not know to fix it in the other) You may argue that this is a simple case and its ok to break the rules. Well you shouldn&#39;t. You should also strive to write good code. Break the rules only for extreme exceptions.</p><p>There were over 180 posts, and there was only ONE solution that looked simpler and was correct. (There were a few that were correct but not as simple as could be.) Why do &quot;bool b = blah; if b then do blah?&quot; When you can just say &quot;if blah then do blah?&quot;</p><p>&nbsp;My only question, is it not valid to say:</p><p>foo = null</p><p>if someboolean.equals(foo)???? If you can&#39;t do this, then java is really lame.</p>
  • verisimilidude 2006-11-29 18:58
    [quote user=&quot;Anonymous&quot;][quote user=&quot;Anonymous&quot;]<p>Anonymous wrote:</p><p style="margin-left: 40px">if (isRecordLocked != newValue)</p><p>No! In Java, Boolean is a class, not a simple type like bool, so:</p><ol><li>The != operator tests whether the two object references refer to the same object; it does not check whether the two object references refer to objects with the same value.</li><li>Object references might be null, which you don&#39;t account for.</li></ol><p>Someone has already beaten me to suggest the following replacement:<br /></p><p style="margin-left: 40px">if (isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue))</p><p>&nbsp;</p><p>[/quote]</p><p>if ((isRecordLocked != newValue) || isRecordLocked==null || newValue==null || !isRecordLocked.equals(newValue)) </p><p>Adding in the direct comparison of the references handles the case of both being null.&nbsp;</p><p>[/quote]</p><p>&nbsp;NO, NO, NO!&nbsp; In Java you can directly compare Boolean&#39;s and get the same result as if you unboxed both variables.&nbsp; Check out http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.21.2<br />&nbsp;</p>
  • Anonymous 2006-11-29 19:15
    I personally agree with you, but the reason for B==A is emphasize the parallel to B.equals(A).<br />
  • VGR 2006-11-29 19:23
    <p>[quote user=&quot;Anonymous&quot;]</p><p>&nbsp;In Java there are just two possible Boolean&#39;s, one wraps true and the
    other wraps false.&nbsp; Yes, you are comparing references but since there
    are only two == and != work fine.<br /></p><p>[/quote]</p><p>Not correct.&nbsp; &#39;new Boolean(true)&#39; will always create a new object.&nbsp; It is not the same object as Boolean.TRUE.&nbsp; Comparing it to Boolean.TRUE using the &#39;==&#39; operator will return false.&nbsp; Try it for yourself.&nbsp;</p><p>[quote user=&quot;Anonymous&quot;]</p><p>&nbsp;NO, NO, NO!&nbsp; In Java you can directly compare Boolean&#39;s and get the same result as if you unboxed both variables.&nbsp; Check out http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.21.2<br />&nbsp;</p><p>[/quote]</p>You may want to read that link more closely.&nbsp; It states, more than once, that unboxing is done if ONE of the operands is a Boolean instance.&nbsp; The very next section states that if both operands are reference types (that is, objects), the operation is reference equality.
  • Why? 2006-11-29 21:14
    <p>&nbsp;</p><p>Why is this a WTF - it&#39;s a property accessor method for the isRecordLocked&nbsp;field which is&nbsp;a Boolean object (which in this particular language can be NULL). When the value is changed, the isDirty field also needs to be set. It&#39;s all pretty standard stuff - maybe it could be refactored a little, and it certainly looks like it was generated using some sort of template or code generation system - but at the end of the day it would be&nbsp; WTF if all the accessor didn&#39;t check for NULL before using the object - that&#39;s far more common.</p><p>Incidentally I&#39;ve worked for a large NZ Govt transmission company - they had real WTF code - a Fortran code (8000 lines all in 1 function) which have a different variable for every property of every powerstation in New Zealand... Compiler used to fall over presumably </p>
  • Anonymous 2006-11-29 21:50
    <p>
    <br />Hmm, I could&#39;ve sworn that &#39;null == null&#39; returned false.&nbsp; Learn something new everyday :)</p><p>
    </p><p>&nbsp;</p><p>Yeah, it works that way in SQL, but not in Java.</p><p>&nbsp;</p><p>If this were my code, I&#39;d be grepping through the source to see if I could change to &#39;boolean&#39;s instead of &#39;Boolean&#39;s, which is my preferred way to simplify this method.</p><p>&nbsp;</p><p>&nbsp;</p>
  • Eugene Kaganovich 2006-11-30 01:30
    <p>OMG WTF at the comments!</p><p>The original code is correct and about as readable as any proposed alternative, and way more correct than most (go back to VB you wankers!). The WTF with it is that setIsRecordLocked should have been defined to take a boolean not Boolean.</p><p>&nbsp;DUUUUUUH!<br />&nbsp;</p><p>&nbsp;</p>
  • space cowboy 2006-11-30 02:29
    <p>&nbsp;people...&nbsp;</p><p>dirty = newValue != isRecordLocked</p><p>&nbsp;</p>
  • space cowboy 2006-11-30 02:55
    <p>oops. forgot this was java code... once again, I say people...</p><p>&nbsp;dirty = !((newValue == isRecordLocked)||(newValue!=null &amp;&amp; newValue.equals(isRecordLocked)));</p>
  • disaster 2006-11-30 03:27
    <p>The real WTF is the undocumented side-effect. </p><p>This method would also definitely benefit from a couple of unit tests - which, of course, may well exist.<br /></p>
  • DagSverre 2006-11-30 03:56
    <p>&gt; oops. forgot this was java code... once again, I say people...<br />&gt; dirty = !((newValue == isRecordLocked)||(newValue!=null &amp;&amp; newValue.equals(isRecordLocked)));</p><p>&nbsp;<br />No no no. Don&#39;t try to be clever. Here&#39;s why:</p><p>

    (original state of o: not dirty, not locked)

    </p><p>o.setRecordLocked(true);
    <br />o.setRecordLocked(true);

    </p><p>Result: o is NOT dirty, but has changed state to locked. If you want a &quot;nice oneliner&quot; syntax then dirty = dirty &amp;&amp; ... will do it, but rewriting if-conditions to boolean operator logic is hardly something that is very important.

    </p><p>Can we all stop discussing a perfectly good piece of code now? Just because it is posted on TDWTF apparently doesn&#39;t mean there&#39;s something wrong with it.<br />
    </p>
  • Earl Purple 2006-11-30 03:59
    <p>No, I would have to say that the real wtf is using Boolean instead of an enum that can have 3 possible values, although Java has only just introduced enums in version 5.</p><p>public enum LockStatus { UNKNOWN, UNLOCKED, LOCKED };<br />private LockStatus lockStatus;<br /></p><p>public void setLockStatus( LockStatus newValue )<br />{<br />&nbsp;&nbsp;&nbsp; if ( newValue != lockStatus )<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; context.setDirty( true );<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; lockStatus = newValue;<br />&nbsp;&nbsp;&nbsp; }<br />}<br /></p><p>Now doesn&#39;t that make life a whole lot simpler?</p><p>&nbsp;</p><p>&nbsp;</p>
  • emurphy 2006-11-30 05:28
    [quote user=&quot;Earl Purple&quot;]<p>No, I would have to say that the real wtf is using Boolean instead of an enum that can have 3 possible values, although Java has only just introduced enums in version 5.</p><p>public enum LockStatus { UNKNOWN, UNLOCKED, LOCKED };<br />private LockStatus lockStatus;<br /></p><p>public void setLockStatus( LockStatus newValue )<br />{<br />&nbsp;&nbsp;&nbsp; if ( newValue != lockStatus )<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; context.setDirty( true );<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; lockStatus = newValue;<br />&nbsp;&nbsp;&nbsp; }<br />}<br /></p><p>Now doesn&#39;t that make life a whole lot simpler?</p><p>&nbsp;</p><p>&nbsp;</p><p>[/quote]</p><p>&nbsp;</p><p>Yes, it doesn&#39;t.&nbsp; Wait till your cow orkers get a hold of this, and insist on adding MAYBELOCKED and VERYLOCKED and FILENOTFOUND.</p><p>&nbsp;</p><p>Won&#39;t someone <em>please</em> think of the <em>children</em>?</p><p>&nbsp;</p>
  • Earl Purple 2006-11-30 06:13
    <p>Actually, having them pass an enum can be more descriptive than getting them to pass in true or false.</p><p>Have a look at a function call made to an API that passes parameters as true or false etc and see if you can tell without looking up in the API what those boolean values stand for.</p><p>Now instead if you pass in LOCKED or UNLOCKED it can make a lot more sense.</p><p>And it&#39;s easier to add more enums if necessary. How would you do that with Boolean?<br />And how is MAYBELOCKED different from UNKNOWN? I was just showing how to write a tri-state properly.</p><p>&nbsp;</p>
  • Anonymous 2006-11-30 07:25
    <code>public void setIsRecordLocked(Boolean newValue) {<br /> &nbsp;&nbsp;if (isRecordLocked!=newValue &amp;&amp; (isRecordLocked==null&nbsp;||&nbsp;newValue==null || !isRecordLocked.equals(newValue)) {<br /> &nbsp;&nbsp;&nbsp;&nbsp;context.setDirty(true);<br />&nbsp;&nbsp;}<br />&nbsp;&nbsp;isRecordLocked = newValue;<br /> }<br /><br /></code>And look up short-circuiting before telling me it&#39;ll throw an NPE.<br /><br />TRWTF are the comments.<br />
  • Asd 2006-11-30 07:55
    [quote user=&quot;VGR&quot;]<p>I would much rather put &#39;<code>private static boolean equals(Object o1, Object o2) { return o1 == null ? (o2 == null) : o1.equals(o2); }</code>&#39; in multiple classes than be saddled with dependency on a third-party jar for something so trivial.</p>
    <p>And I agree that many of Jakarta&#39;s offerings, particularly their Java offerings, are sloppy and unreliable.&nbsp; I like the web server, and I like ant, but that doesn&#39;t give the other projects a free pass.&nbsp; I have a strong dislike of the Jakarta Commons in particular.</p><p>[/quote]</p><p>This is fair enough. I usually use the jakarta commons stuff because I already am stuck with it as it is a dependency of some other library I am using :)&nbsp;</p><p>(Though I think HttpClient&nbsp; is pretty good, and collections and logging just should not exist)<br /></p>
  • Thuktun 2006-11-30 09:56
    <p>[quote user=&quot;Anonymous&quot;]Why is this a WTF[/quote]</p><p>It isn&#39;t, really.&nbsp; The original quoted version appears to be correct.&nbsp; All the flailing about for replacements that don&#39;t quite work correctly would seem to indicate that it&#39;s not as simple as everyone seems to think at first glance.<br /></p>
  • Earl Purple 2006-11-30 10:07
    <p>I also don&#39;t see why a record lock should be&nbsp;a tri-state situation. Either it&#39;s locked or it isn&#39;t.</p><p>The initial state should be unlocked until someone locks it.</p><p>&nbsp;</p>
  • AdT 2006-11-30 10:34
    [quote user=&quot;DagSverre&quot;]<p>If you want a &quot;nice oneliner&quot; syntax then dirty = dirty &amp;&amp; ... will do it</p><p>[/quote]</p><p>Nope. You have one more guess what the correct operator is.</p><p>Captcha: genius (the opposite of a TDWTF comment poster)<br /><br /></p>
  • Web 1.0 2006-11-30 12:33
    <p>Another kick at the cat.</p><p>The 1-liner solution is a trick. The much more understandable, intention-revealing 1-liner that preserves the original code&nbsp;is:<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; SetDirtyIfDifferent( isRecordLocked, newValue );</p><p><br />To me the WTF is that it&#39;s hard to tell it&#39;s just a property-setter that flags a dirty bit (or whatever SetDirty() does) when the value changes. I&#39;d bet the whole class (and hierarchy) is littered with that code copy-pasted. Wheeeeee:</p><p>1. You&#39;re micromanaging the dirty state of the object in each and every property setter and probably reusing the same code all over the place&nbsp;- write the code once.<br />2. Do you really want to SetDirty(false)? Isn&#39;t it more intention-revealing to have SetDirty() and ClearDirty()?</p><pre>// assumption: value == null is a valid value

    public void SetMyValue( Boolean value )
    {
    SetDirtyIfDifferent( myValue, value );
    myValue = value;
    }

    // this can probably be pulled up to a common base class.
    private void SetDirtyIfDifferent( Boolean currentValue, Boolean newValue )
    {
    // both values null
    if( currentValue == null &amp;&amp; newValue == null )
    return;

    // one of the values is null implies dirty
    if( (currentValue == null &amp;&amp; newValue != null) ||
    (currentValue != null &amp;&amp; newValue == null) )
    {
    SetDirty();
    return;
    }

    // nether value is null, dirty if different
    if( !currentValue.Equals(newValue) )
    {
    SetDirty();
    }

    }
    </pre><p>&nbsp;&nbsp;<br />(BTW I swear that I wrote this before reading all of&nbsp;replies and only before posting noticed Asd&#39;s response and LOL&#39;d when I realised we both called it SetDirtyIfDifferent :-)</p><p>The 1-liner bit is a trick.&nbsp;Trying to shove all of that into a single line is unreadable and bad practice.&nbsp;Some of the solutions are more of a wtf than the original. Truly shocking.</p><p>As others point out, Tri-state booleans are legal and necessary. Think of it as Yes / No / Don&#39;t know. That said, using null is probably a bad choice for this. An enum would have been better. For more complicated objects, using the Null Object pattern might make more sense so client code isn&#39;t continually comparing against null.</p>
  • AdT 2006-11-30 13:28
    [quote user=&quot;Anonymous&quot;]<pre> if( (currentValue == null &amp;&amp; newValue != null) ||<br /> (currentValue != null &amp;&amp; newValue == null) )</pre><pre>[/quote]</pre><pre>Why not simply</pre><pre>if ((currentValue == null) != (newValue == null))</pre><pre>?&nbsp;</pre>
  • DagSverre 2006-11-30 13:45
    [quote user=&quot;Anonymous&quot;][quote user=&quot;DagSverre&quot;]<p>If you want a &quot;nice oneliner&quot; syntax then dirty = dirty &amp;&amp; ... will do it</p><p>[/quote]</p><p>Nope. You have one more guess what the correct operator is.</p><p>[/quote]</p><p><br />Ouch! That should teach me something about commenting the mistakes of others :-)<br />&nbsp;</p>
  • Yuriy 2006-11-30 15:16
    <p>[quote user=&quot;Anonymous&quot;]<code>public void setIsRecordLocked(Boolean newValue) {<br /> &nbsp;&nbsp;if (isRecordLocked!=newValue &amp;&amp; (isRecordLocked==null&nbsp;||&nbsp;newValue==null || !isRecordLocked.equals(newValue)) {<br /> &nbsp;&nbsp;&nbsp;&nbsp;context.setDirty(true);<br />&nbsp;&nbsp;}<br />&nbsp;&nbsp;isRecordLocked = newValue;<br /> }<br /><br /></code>And look up short-circuiting before telling me it&#39;ll throw an NPE.<br /><br />TRWTF are the comments.<br />[/quote]</p><p>That works, but can be simplified to this, which someone has already posted (or maybe it was something very similar):</p><p><code>public void setIsRecordLocked(Boolean newValue) {</code><br />&nbsp;<code> if (isRecordLocked!=newValue &amp;&amp; (isRecordLocked==null&nbsp;|| !isRecordLocked.equals(newValue)) {<br /> &nbsp;&nbsp;&nbsp;&nbsp;context.setDirty(true);<br />&nbsp; }<br />&nbsp;isRecordLocked = newValue;</code><br />}&nbsp;</p><br />
  • Earl Purple 2006-11-30 15:35
    [quote user=&quot;DagSverre&quot;] <p>Result: o is NOT dirty, but has changed state to locked. If you want a &quot;nice oneliner&quot; syntax then dirty = dirty &amp;&amp; ... will do it, but rewriting if-conditions to boolean operator logic is hardly something that is very important. </p><p>[/quote]</p><p>It would need to be dirty&nbsp;= dirty || ...</p><p>or preferably dirty ||= ...</p><p>but &quot;context&quot; seems to be an external object so that is not possible.</p><p>There is something strange about the name of the function though. Are we marking the record as dirty if it has changed state so that we know we must update it when we come to persist it? Why are we calling setIsRecordLocked, why not lock() or unlock(). And locking a record by itself does not make it &quot;dirty&quot;.</p><p>I would like to know the context that this code is in.</p><p>&nbsp;</p>
  • chrismcb 2006-11-30 18:35
    <p>[quote user=&quot;Anonymous&quot;]I personally agree with you, but the reason for B==A is emphasize the parallel to B.equals(A).<br />[/quote]</p><p>And therein lies the problem. Its only a &quot;parallel.&quot; The whole concept of &quot;left as an exercise to the reader&quot; should not apply to code. Give the reader (read future maintainers) as much information as possible. Make it as easy as possible to understand what is going on. You are trying to write the function &quot;B.equals(A)&quot; but you can&#39;t if B is null. Be concise, and explicit. if you are comparing a value to null, compare it to null. Don&#39;t compare it to something that is sometimes null. It would be a different story if you had some constant that equaled null.</p><p>&nbsp;For those readers who missed the original quote. It was suggested to write:</p><p>&quot;if (B == null) then if (B == A) ...&quot; instead of &quot;if (B == null) then if (null == A) ...&quot; as the first is &quot;clearer,&quot; which of course it isn&#39;t.</p><p>&nbsp;Here is the problem. You can&#39;t simply write &quot;if B == A&quot; if both aren&#39;t NULL. If someone who is in a hurry, and doesn&#39;t bother to read the entire statement sees this holy trinity of if&#39;s, they might think &quot;Well geeze we are trying to compare B and A, so lets just delete the null check, and leave if (B == A)&quot; At first glance it seems the right thing to do, but as many people already pointed out, you can&#39;t do that.</p><p>So you just made it harder for future maintainers to maintain this code, just so you can keep your little &quot;parallel&quot; going. If you mean null then say it. </p>
  • Kobes 2006-11-30 18:52
    <p>public bool compare(Object o1, Object o2) {<br />&nbsp;&nbsp;&nbsp; return (o1 == null || o2 == null) ? o1 == o2 : o1.equals(o2);<br />}</p><p>public void setIsRecordLocked(Boolean newValue) {<br />&nbsp;&nbsp;&nbsp; if (!compare(isRecordLocked, newValue)) context.setDirty(true);<br />&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />}</p>
  • emurphy 2006-11-30 20:12
    [quote user=&quot;Earl Purple&quot;]<p>Have a look at a function call made to an API that passes parameters as true or false etc and see if you can tell without looking up in the API what those boolean values stand for.</p><p>&nbsp;</p><p>And yes, sometimes you might need to allow more than T/F/null, but you need to exercise reasonable judgment as to whether this need is likely to actually occur for a given case - otherwise you get code bloat, with a whole bunch of separate enums that are all basically equivalent to a boolean choice, and most of which will likely stay that way.&nbsp;</p><p>[/quote]</p><p>&nbsp;</p><p>In the case of &quot;setIsRecordLocked(Boolean newValue)&quot;, it&#39;s obvious what the boolean values stand for.&nbsp; That is not the WTF.<br /><br /></p><p>[quote user=&quot;chrismcb&quot;]</p><p>[quote user=&quot;Anonymous&quot;]I personally agree with you, but the reason for B==A is emphasize the parallel to B.equals(A).<br />[/quote]</p><p>And
    therein lies the problem. Its only a &quot;parallel.&quot; The whole concept of
    &quot;left as an exercise to the reader&quot; should not apply to code. Give the
    reader (read future maintainers) as much information as possible. Make
    it as easy as possible to understand what is going on. You are trying
    to write the function &quot;B.equals(A)&quot; but you can&#39;t if B is null. Be
    concise, and explicit. if you are comparing a value to null, compare it
    to null. Don&#39;t compare it to something that is sometimes null. It would
    be a different story if you had some constant that equaled null.</p><p>&nbsp;For those readers who missed the original quote. It was suggested to write:</p><p>&quot;if
    (B == null) then if (B == A) ...&quot; instead of &quot;if (B == null) then if
    (null == A) ...&quot; as the first is &quot;clearer,&quot; which of course it isn&#39;t.</p><p>&nbsp;Here
    is the problem. You can&#39;t simply write &quot;if B == A&quot; if both aren&#39;t NULL.
    If someone who is in a hurry, and doesn&#39;t bother to read the entire
    statement sees this holy trinity of if&#39;s, they might think &quot;Well geeze
    we are trying to compare B and A, so lets just delete the null check,
    and leave if (B == A)&quot; At first glance it seems the right thing to do,
    but as many people already pointed out, you can&#39;t do that.</p><p>So you
    just made it harder for future maintainers to maintain this code, just
    so you can keep your little &quot;parallel&quot; going. If you mean null then say
    it. </p>[/quote]<p>&nbsp;</p><p>The intent is to compare A and B.&nbsp; Whether they are null or not-null, the intent is <em>still</em> to compare A to B.&nbsp; Placing &quot;A == null&quot; (or &quot;null == B&quot;, etc.) after the ? part of the operator obscures this purpose.</p><p>&nbsp;</p><p>In either case, the proper way to prevent future coders in a hurry from improperly collapsing the logic is to comment it:</p><p>&nbsp;</p><p>boolean AreEqual(Object a, Object b) {</p><p>&nbsp; return (a != null)</p><p>&nbsp;&nbsp;&nbsp; ? (a.Equals(b)) // use Equals() if possible<br /></p><p>&nbsp;&nbsp;&nbsp; : (a == b); // otherwise use ==</p><p>}</p><p>&nbsp;</p><p>or</p><p><br />boolean AreEqual(Object a, Object b) {</p>
    <p>&nbsp; return (a == null)</p>
    <p>&nbsp;&nbsp;&nbsp; ? (b == null) // if a is null, then only another null can be equal to it<br />
    </p>
    <p>&nbsp;&nbsp;&nbsp; : (a.Equals(b)); // if a is not null, then use a.Equals()<br /></p>
    <p>}</p>
    <p>&nbsp;</p>
  • brendan 2006-12-01 00:22
    The way I would have done this is by doing:

    public void setIsRecordLocked(Boolean newValue) {
    // Update state if required.
    context.setDirty(!(isRecordLocked == newValue));
    isRecordLocked = newValue;
    // Change the value.

    }

    but this is two lines.
  • Peter Lawrey 2006-12-01 02:51
    <pre>// I agree, Boolean comparison is ugly...
    if (isRecordLocked != null ? !isRecordLocked.equals(newValue) : newValue != null) {
    context.setDirty(true);
    isRecordLocked = newValue;
    }
    </pre>
  • Anonymous 2006-12-01 05:39
    [quote user=&quot;Anonymous&quot;]<p>[quote user=&quot;Anonymous&quot;]<code>public void setIsRecordLocked(Boolean newValue) {<br /> &nbsp;&nbsp;if (isRecordLocked!=newValue &amp;&amp; (isRecordLocked==null&nbsp;||&nbsp;newValue==null || !isRecordLocked.equals(newValue)) {<br /> &nbsp;&nbsp;&nbsp;&nbsp;context.setDirty(true);<br />&nbsp;&nbsp;}<br />&nbsp;&nbsp;isRecordLocked = newValue;<br /> }<br /><br /></code>And look up short-circuiting before telling me it&#39;ll throw an NPE.<br /><br />TRWTF are the comments.<br />[/quote]</p><p>That works, but can be simplified to this, which someone has already posted (or maybe it was something very similar):</p><p><code>public void setIsRecordLocked(Boolean newValue) {</code><br />&nbsp;<code> if (isRecordLocked!=newValue &amp;&amp; (isRecordLocked==null&nbsp;|| !isRecordLocked.equals(newValue)) {<br /> &nbsp;&nbsp;&nbsp;&nbsp;context.setDirty(true);<br />&nbsp; }<br />&nbsp;isRecordLocked = newValue;</code><br />}&nbsp;</p><br />[/quote]<br /><br />Nope, the API documentation says this: &quot;The equals method implements an equivalence relation on non-null object references&quot;. If newValue is null and isRecordLocked isn&#39;t, then the results of your method are undefined. Probably it&#39;ll throw an NPE!<br /><br />I think it was posted as <code>setDirty(isRecordLocked!=newValue &amp;&amp; (isRecordLocked==null&nbsp;|| !isRecordLocked.equals(newValue))</code>, which is just wrong.<br />
  • Anonymous 2006-12-01 05:41
    [quote user=&quot;Anonymous&quot;]Nope, the API documentation says this: &quot;The equals method implements an equivalence relation on non-null object references&quot;. If newValue is null and isRecordLocked isn&#39;t, then the results of your method are undefined. Probably it&#39;ll throw an NPE!<br />[/quote]<br />Blast! Just re-read the documentation. It also says that x.equals(null) must always return false. So you win.<br /><br />Captcha = truthiness, at last.<br />
  • Stephanos Piperoglou 2006-12-01 07:15
    [quote user=&quot;Ottokar&quot;]Hello,

    a tristate Boolean makes sense. For example if you have to deal with databases. DBs can store booleans with a NULL value.[/quote]<div><br class="khtml-block-placeholder" /></div><div>Absolutely true, but ironically, JDBC offers no way to read a Boolean from a ResultSet - only a primitive boolean. Bring out the&nbsp;</div><div><br class="khtml-block-placeholder" /></div><div>rs.isNull(column) ? null : new Boolean(rs.getBoolean(column));</div><div><br class="khtml-block-placeholder" /></div><div>Regardless, your point brings up the question, why would you support tri-state booleans in the db in the first place? Tri-state booleans are semantically useful when reading from a database column, but more so when reading one marked NOT NULL: Boolean.TRUE means it&#39;s true, Boolean.FALSE means it&#39;s false, and null means the field hasn&#39;t been read from the database yet ;-)</div>
  • Web 1.0 2006-12-01 09:12
    <p>Don&#39;t read into the semantics of what&nbsp;the property of IsRecordLocked really is. You&#39;re just making an assumption:&nbsp;read it as setIsFoo.</p><p>Don&#39;t read into the semantics that it&#39;s a java boolean and forget your knowledge of that. So what, it&#39;s a class, and the reference can be null. I&#39;m glad you understand object.Equals semantics, it&#39;s important, but it does require the object to be non-null. Some languages (like c#) have overrides so that you can again write o1 == o2. Java might be &#39;correct&#39; in comparing references, but it sure does confuse a lot of people, but that&#39;s not the point either. Figure it out, it&#39;s your job.</p><p>Here I&#39;ll randomly take some posters simplification. I&#39;m not checking it for correctness. I don&#39;t care for my illustration if it&#39;s deficient in some way.</p><p><font face="Courier New">public void setFoo(Boolean newValue) {<br />&nbsp;&nbsp;if (isFoo!=newValue &amp;&amp; (isFoo==null&nbsp;||&nbsp;newValue==null || !isFoo.equals(newValue)) {<br />&nbsp;&nbsp;&nbsp;&nbsp;context.setDirty(true);<br />&nbsp;&nbsp;}<br />&nbsp;&nbsp;isFoo= newValue;<br />}<p><font face="Courier New">public void setIsBar(Boolean newValue) {<br />&nbsp;&nbsp;if (isBar!=newValue &amp;&amp; (isBar==null&nbsp;||&nbsp;newValue==null || !isBar.equals(newValue)) {<br />&nbsp;&nbsp;&nbsp;&nbsp;context.setDirty(true);<br />&nbsp;&nbsp;}<br />&nbsp;&nbsp;isBar= newValue;<br />}</font></p><p><font face="Courier New">public void setIsBaz(Boolean newValue) {<br />&nbsp;&nbsp;if (isBaz!=newValue &amp;&amp; (isBaz==null&nbsp;||&nbsp;newValue==null || !isBar.equals(newValue)) {<br />&nbsp;&nbsp;&nbsp;&nbsp;context.setDirty(true);<br />&nbsp;&nbsp;}<br />&nbsp;&nbsp;isBaz= newValue;<br />}</font></p><p>// VERSUS</p><p><font face="Courier New">public void setFoo(Boolean newValue) {</font><font face="Courier New"><br />&nbsp;&nbsp;SetDirtyIfDifferent(isFoo, newValue);<br />&nbsp;&nbsp;isFoo= newValue;<br />}<p><font face="Courier New">public void setIsBar(Boolean newValue) {<br />&nbsp;&nbsp;SetDirtyIfDifferent(isBar, newValue);<br />&nbsp;&nbsp;isBar= newValue;<br />}</font></p><p><font face="Courier New">public void setIsBaz(Boolean newValue) {<br />&nbsp;&nbsp;SetDirtyIfDifferent(isBaz, newValue);<br />&nbsp;&nbsp;isBaz= newValue;<br />}</font></p><p>// You could even make this easier yet by having SetDirtyIfDifferent return newValue, but that&#39;s just icing.</p><p>public void setFoo(Boolean newValue) {<font face="Courier New"><br />&nbsp;&nbsp;isFoo = SetDirtyIfDifferent(isFoo, newValue);<br />}</font></p><p>PS In the original setIsBaz() did you spot the (intentional) copy-paste error? That&#39;s a subtle bug just waiting to happen. That&#39;s why you write the code just once to eliminate subtle bugs and to better communicate intention. I don&#39;t even need to pepper comments around because the method name is self-explanitory.</p></font></p><p><font face="Courier New">public void setIsBar(Boolean newValue) {<br />&nbsp;&nbsp;SetDirtyIfDifferent(isBar, newValue);<br />&nbsp;&nbsp;isBar= newValue;<br />}</font></p><p><font face="Courier New">public void setIsBaz(Boolean newValue) {<br />&nbsp;&nbsp;SetDirtyIfDifferent(isBaz, newValue);<br />&nbsp;&nbsp;isBaz= newValue;<br />}</font></p><p>// You could even make this easier yet by having SetDirtyIfDifferent return newValue, but that&#39;s just icing.</p><p>public void setFoo(Boolean newValue) {<font face="Courier New"><br />&nbsp;&nbsp;isFoo = SetDirtyIfDifferent(isFoo, newValue);<br />}</font></p><p>PS In the original setIsBaz() did you spot the (intentional) copy-paste error? That&#39;s a subtle bug just waiting to happen. That&#39;s why you write the code just once to eliminate subtle bugs and to better communicate intention. I don&#39;t even need to pepper comments around because the method name is self-explanitory.</p></font></p><p><font face="Courier New">public void setIsBar(Boolean newValue) {<br />&nbsp;&nbsp;if (isBar!=newValue &amp;&amp; (isBar==null&nbsp;||&nbsp;newValue==null || !isBar.equals(newValue)) {<br />&nbsp;&nbsp;&nbsp;&nbsp;context.setDirty(true);<br />&nbsp;&nbsp;}<br />&nbsp;&nbsp;isBar= newValue;<br />}</font></p><p><font face="Courier New">public void setIsBaz(Boolean newValue) {<br />&nbsp;&nbsp;if (isBaz!=newValue &amp;&amp; (isBaz==null&nbsp;||&nbsp;newValue==null || !isBar.equals(newValue)) {<br />&nbsp;&nbsp;&nbsp;&nbsp;context.setDirty(true);<br />&nbsp;&nbsp;}<br />&nbsp;&nbsp;isBaz= newValue;<br />}</font></p><p>// VERSUS</p><p><font face="Courier New">public void setFoo(Boolean newValue) {</font><font face="Courier New"><br />&nbsp;&nbsp;SetDirtyIfDifferent(isFoo, newValue);<br />&nbsp;&nbsp;isFoo= newValue;<br />}<p><font face="Courier New">public void setIsBar(Boolean newValue) {<br />&nbsp;&nbsp;SetDirtyIfDifferent(isBar, newValue);<br />&nbsp;&nbsp;isBar= newValue;<br />}</font></p><p><font face="Courier New">public void setIsBaz(Boolean newValue) {<br />&nbsp;&nbsp;SetDirtyIfDifferent(isBaz, newValue);<br />&nbsp;&nbsp;isBaz= newValue;<br />}</font></p><p>// You could even make this easier yet by having SetDirtyIfDifferent return newValue, but that&#39;s just icing.</p><p>public void setFoo(Boolean newValue) {<font face="Courier New"><br />&nbsp;&nbsp;isFoo = SetDirtyIfDifferent(isFoo, newValue);<br />}</font></p><p>PS In the original setIsBaz() did you spot the (intentional) copy-paste error? That&#39;s a subtle bug just waiting to happen. That&#39;s why you write the code just once to eliminate subtle bugs and to better communicate intention. I don&#39;t even need to pepper comments around because the method name is self-explanitory.</p></font></p><p><font face="Courier New">public void setIsBar(Boolean newValue) {<br />&nbsp;&nbsp;SetDirtyIfDifferent(isBar, newValue);<br />&nbsp;&nbsp;isBar= newValue;<br />}</font></p><p><font face="Courier New">public void setIsBaz(Boolean newValue) {<br />&nbsp;&nbsp;SetDirtyIfDifferent(isBaz, newValue);<br />&nbsp;&nbsp;isBaz= newValue;<br />}</font></p><p>// You could even make this easier yet by having SetDirtyIfDifferent return newValue, but that&#39;s just icing.</p><p>public void setFoo(Boolean newValue) {<font face="Courier New"><br />&nbsp;&nbsp;isFoo = SetDirtyIfDifferent(isFoo, newValue);<br />}</font></p><p>PS In the original setIsBaz() did you spot the (intentional) copy-paste error? That&#39;s a subtle bug just waiting to happen. That&#39;s why you write the code just once to eliminate subtle bugs and to better communicate intention. I don&#39;t even need to pepper comments around because the method name is self-explanitory.</p>
  • Web 1.0 2006-12-01 09:16
    <p>Whoops .. what happened there? PEBKAC</p><p>The whole design looks like it&#39;s compensating for indescriminate setting of properties by client code. Fix the client instead.</p><p>void setIsRecordLocked( Boolean newValue )<br />{<br />&nbsp;&nbsp; SetDirty();<br />&nbsp;&nbsp; isRecordLocked = newValue;<br />}</p>
  • IcemanK 2006-12-01 11:58
    [quote user=&quot;Anonymous&quot;][quote user=&quot;Ottokar&quot;]Hello,
    a tristate Boolean makes sense. For example if you have to deal with
    databases. DBs can store booleans with a NULL value.[/quote]<div><br class="khtml-block-placeholder" /></div><div>Absolutely
    true, but ironically, JDBC offers no way to read a Boolean from a
    ResultSet - only a primitive boolean. Bring out the&nbsp;</div><div><br class="khtml-block-placeholder" /></div><div>rs.isNull(column) ? null : new Boolean(rs.getBoolean(column));</div><div><br class="khtml-block-placeholder" /></div><p>[/quote]</p><p>You are supposed to call rs.wasNull() to determine if the last value read from the recordset was null.</p><p>Thus, the tristate Boolean can be handled by the code.&nbsp;</p>
  • Olddog 2006-12-01 22:58
    <br /><strong>Just curious</strong>... after several pages of excellent boolean arguments, why would it matter if &quot;isRecordLocked&quot; was dirty or not?
  • Anonymous 2006-12-01 23:40
    [quote user=&quot;Anonymous&quot;]<br /><p>Why is this a WTF - it&#39;s a property accessor method for the isRecordLocked&nbsp;field which is&nbsp;a Boolean object (which in this particular language can be NULL). When the value is changed, the isDirty field also needs to be set</p><p>[/quote]</p><p>After reading 4 pages of comments, I have to disagree.&nbsp; This is the best WTF I&#39;ve seen in a long time.<br />&nbsp;</p>
  • Jonathan 2006-12-02 18:36
    <a href="http://programming.reddit.com/info/tdls/comments/ctdzw?context=5">Solution on Reddit</a><br />
  • Jerry 2006-12-03 10:55
    <p>Ooops...</p><p>&nbsp;This solution will assign False to context.SetDirty where the original code would only assign True.&nbsp; The side effect of this code that is not present in the original would be to clear a True SetDirty from a prior execution.<br />&nbsp;</p>
  • Ben M 2006-12-03 21:32
    <p>don&#39;t u mean?:</p><p>public void setIsRecordLocked(Boolean newValue) {<br />&nbsp;&nbsp;&nbsp; if (isRecordLocked != newValue)<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; context.setDirty(true);</p><p>&nbsp;&nbsp;&nbsp; } else {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />&nbsp;&nbsp;&nbsp; }<br />}</p>
  • Weaselâ„¢ 2006-12-03 23:53
    LMFAO
  • anonymous koder 2006-12-04 05:13
    <p>You are right, it does not call setDirty if both newValue and isRecordLocked hold the same value (true ot false).</p><p>My fault!!</p><p>&nbsp;</p><blockquote style="margin-left: 40px" style="margin-left: 40px"><div><img src="/Themes/default/images/icon-quote.gif" /> <strong>Anonymous:</strong></div><div>I
    think you are all missing the point, if you take pen and paper and
    write a table with all possible conditions, you will see that actually
    setDirty(true) <strong>is called every time except if both newValue and isRecordLocked are null</strong> !!!</div></blockquote><p style="margin-left: 40px">That&#39;s just not true.&nbsp;&nbsp; Consider the case newValue &amp; isRecordLocked are both false. </p><p style="margin-left: 40px">if (isRecordLocked != null) {<br />&nbsp;&nbsp;&nbsp; This is true, so we continue. </p><p style="margin-left: 40px">if (newValue == null || <br />&nbsp;&nbsp;&nbsp; This is false so we evaluate the other side of the OR</p><p style="margin-left: 40px">|| !isRecordLocked.equals(newValue)) {<br />&nbsp;&nbsp;&nbsp; This is also false, so we exit the if() block.</p><p style="margin-left: 40px">&nbsp;&nbsp;&nbsp; else if (newValue != null) {&nbsp; context.setDirty(true);&nbsp;&nbsp;&nbsp; }<br />Since the <strong>first if()</strong> was true, we skip the else clause (did you mistakenly associate it with the second if()?)</p><div style="margin-left: 40px">&nbsp;&nbsp; isRecordLocked = newValue;<br />Finally, we pointless set a value which is already false to false.</div><p>&nbsp;</p>
  • correct 2006-12-04 16:35
    In Java objects can be null so you should already have one of these:<br /><br /><br />
    <br />public class Objects<br />{<br />&nbsp;&nbsp;&nbsp; public static boolean equals( Object a, Object b )<br />&nbsp;&nbsp;&nbsp; {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; return ( a == b || ( a != null &amp;&amp; a.equals( b ) ) );<br />&nbsp;&nbsp;&nbsp; }<br />}<br />
    <br /><br />Then you can rewrite the original method like so:<br /><br />
    <br />public void setIsRecordLocked(Boolean newValue) {<br />&nbsp;&nbsp;&nbsp; if ( !Objects.equal( isRecordLocked, newValue ) {<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; context.setDirty(true);<br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp; isRecordLocked = newValue;<br />&nbsp;&nbsp;&nbsp; }<br />}<br />
    <br /><br />You will probably find many times for reusing this utility method and improves readability a great deal.<br /><br /><br /><br />&nbsp;
  • Henry 2006-12-11 20:55
    How about this?

    if(newValue!=NULL)
    {
    if (isRecordLock==NULL or !isRecordLock.Equals(newValue))
    {
    context.SetDirty(True)
    isRecordLock=newValue;
    }
    }

  • Brandito 2006-12-12 00:05
    if (!(isRec == newVal == null || isRec == newVal != null)) setDirty(true);<br /><br />isRec = newVal;
  • Cessair 2007-07-18 22:03
    OMG. I think I wrote that code. At least, I did write code for large NZ Govt departments a while ago, and it looks horribly familiar.

    If so:
    - I'm sorry, guys
    - we were forced to use objects rather than primitive classes
    - I'm pretty sure it was actually C++, not Java
    - it was definitely pre-Google and pre-WTF, probably about 1996
    - it took AAAAGES to work out how to do it, and many of your arguments and questions went through my head at the time
    - I don't code any more....