HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Detecting change of a name or a part

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
changepartnamedetecting

Problem

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

Solution

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);
}

Code Snippets

if (part == null && newName == null) {
    return false;
}
if (part == null || newName == null) {
    return true;
}
if (part == null) {
    return newName != null;
}
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);
}
private boolean isThingChanged(String newName, Thing thing) {
    if (thing.getPart() == null) {
        return newName != null;
    }
    return !Objects.equal(thing.getPart().getName(), newName);
}
private boolean isThingChanged(String newName, Thing thing) {
    return thing.getPart() == null ? newName != null : !Objects.equal(thing.getPart().getName(), newName);
}

Context

StackExchange Code Review Q#71706, answer score: 4

Revisions (0)

No revisions yet.