Detecting change of a name or a part

Question

We have a fair amount of code which looks like this:

private boolean isThingChanged(String newName, Thing thing) {
    final Part part = thing.getPart();
    if (part == null && newName == null) {
        return false;
    }
    if (part == null || newName == null) {
        return true;
    }
    return !part.getName().equals(newName);
}

That is, if the thing's current part is null and we're trying to set its name to null, that's not a change. If the part is null or the name is null, but not both, that is a change. And if neither is null, we have to look at it a little closer.

Thing and Part represent different entities for different such methods, so the first and final lines have the same general form but different specifics.

As written, this generates a SimplifiableIfStatement warning in Intellij, which would prefer to see it as:

private boolean isThingChanged(String newName, Thing thing) {
    final Part part = thing.getPart();
    return !(part == null && newName == null) && (part == null || newName == null || !part.getName().equals(newName));
}

Which is arguably simpler, but certainly not easier for me to read. I can of course suppress the warning with @SuppressWarnings("SimplifiableIfStatement") but prefer to avoid that if possible.

I would like to find a way to extract this bit:

if (part == null && newName == null) {
    return false;
}
if (part == null || newName == null) {
    return true;
}

which is duplicated in all the instances of this code, because (a) it triggers the SimplifiableIfStatement warning and (b) it just seems too wordy to have it everywhere. So far I've come up with this:

private Boolean changedFromNull(Object a, Object b) {
    // if both are null, nothing has changed.
    if (a == null && b == null) {
        return false;
    }
    // if only one is null, something has changed
    if (a == null || b == null) {
        return true;
    }
    // if neither is null, further analysis is required
    return null;
}

but that's arguably even worse, because now the calling code has to null-check the result of this method in addition to all the other null checking already going on.

private boolean isThingChanged(String newName, Thing thing) {
    final Part part = thing.getPart();
    final Boolean changedFromNull = changedFromNull(part, newName);
    return changedFromNull != null ? changedFromNull : !part.getName().equals(newName);
}

What's a good approach to this?

Answer 1

A good option is to use the Objects utility class (available since Java 7)

Note also, that instead of doing if-conditions and returning true, or false, you can do a direct return of the condition... For example, the part == null is common to these two expressions:

if (part == null && newName == null) {
    return false;
}
if (part == null || newName == null) {
    return true;
}

And those can be simplified to just:

if (part == null) {
    return newName != null;
}

Note also, that your code runs the risk of a NullPointerException if part.getName() returns null. Is that possible?

Your code:

private boolean isThingChanged(String newName, Thing thing) {
    final Part part = thing.getPart();
    if (part == null && newName == null) {
        return false;
    }
    if (part == null || newName == null) {
        return true;
    }
    return !part.getName().equals(newName);
}

Could be:

private boolean isThingChanged(String newName, Thing thing) {
    if (thing.getPart() == null) {
        return newName != null;
    }
    return !Objects.equal(thing.getPart().getName(), newName);
}

Writing that as a ternary is an option:

private boolean isThingChanged(String newName, Thing thing) {
    return thing.getPart() == null ? newName != null : !Objects.equal(thing.getPart().getName(), newName);
}

Answer 2

If the part is null or the name is null, but not both, that is a change.

If part == null how would you set the name no matter if it is null or not. How should something impossible change something ?? So, if part == null this method should always return false.

If part != null we have the possibilities that

• part.getName() == null && newName == null -> false
• part.getName() == null || newName == null -> true
• part.getName() == newName -> false
• part.getName() != newName -> true

which we can check by Objects.equals().

which will result in

private boolean isThingChanged(String newName, Thing thing) {
    final Part part = thing.getPart();
    if (part == null) {
        return false;
    }
    return !Objects.equal(part.getName(), newName);
}  

I know, this does not result in the same results as the former method, hence it is not a true refactoring, but IMHO your logic about when Thing is changed is flawed.

Update based on the comment

...changing by adding a part with a name counts just as much as changing the name of an existing part.

• if a Part had been added, it should exist, hence thing.getPart() != null.
• if a Part does not exist, the isThingChanged() mustn't depend on Part. Otherwise the method should be named willThingChange().