Behrang Saeedzadeh

A clean answer to the DailyWTF equality checking issue...

Friday, 1. December 2006, 07:10:41

This DailyWTF post about implementing a method that assigns a new value to a member variable and sets a dirty flag accordingly, actually does have a cleaner solution.

Here's the original code:

/**
    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) {
        if (newValue == null || !isRecordLocked.equals(newValue)) {
            context.setDirty(true);
        }
    }
    else if (newValue != null) {
        context.setDirty(true);
    }
    // Change the value.
    isRecordLocked = newValue;
}

And here's the cleaner implementation:

public void setIsRecordLocked(Boolean newValue) {

   if (newValue == null) {
      context.setDirty(isRecordLocked != null);
   } else {
      context.setDirty(!newValue.equals(isRecordLocked));
   }
   
   isRecordLocked = newValue;

}

Actually, at the first glance I found myself in a mind freeze situation. But then in a a few minutes I came up with the cleaner solution.

Snap looks very promisingWhat keyboard is that?

Comments

Anonymous # 2. December 2006, 04:14

Anonymous writes:

The above is incorrect, since it sometimes calls setDirty(false) where the original code does not. It may not be valid to call setDirty(false) if for instance another method call on the object has already called setDirty(true). Then calling setDirty(false) would clear a dirty flag that should not be cleared.

Anonymous # 3. December 2006, 00:51

Anonymous writes:

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

although I'd probably write it

public void setIsRecordLocked(Boolean newValue) {
// Update state if required.
if ( newValue != null && newValue.equals(isRecordLocked) ) {
} else {
context.setDirty(true);
}
// Change the value.
isRecordLocked = newValue;
}

behrangsa # 3. December 2006, 14:28

Oops! You are right dude! Thanks for the correction

Anonymous # 3. December 2006, 20:10

pedrow writes:

Correct me if I'm wrong, but that still isn't right, is it? If both newValue and isRecordLocked are null, the WTF code doesn't call the function, but the post above does.

How about
public void setIsRecordLocked(Boolean newValue) {
// Update state if required.
if ( (newValue == isRecordLocked) || ((newValue != null) && newValue.equals(isRecordLocked))) {
} else {
context.setDirty(true);
}
// Change the value.
isRecordLocked = newValue;
}

?