Reducing the cyclomatic complexity, multiple if st

问题:

I have the following code:

private Facility updateFacility(Facility newFacility, Facility oldFacility) {
    if (newFacility.getCity() != null)
        oldFacility.setCity(newFacility.getCity());
    if (newFacility.getContactEmail() != null) 
        oldFacility.setContactEmail(newFacility.getContactEmail());
    if (newFacility.getContactFax() != null) 
        oldFacility.setContactFax(newFacility.getContactFax());
    if (newFacility.getContactName() != null) 
        oldFacility.setContactName(newFacility.getContactName());
    // ......
}

There are around 14 such checks and assignments. That is, except for a few, I need to modify all the fields of the oldFacility object. I'm getting a cyclomatic complexity of this code 14, which is "greater than 10 authorized" as per SonarQube. Any ideas upon how to reduce the cyclomatic complexity?

回答1:

At some point in your program, you will have to implement the logic:

• If the new facility has a property defined, update the old facility accordingly
• If not, do not override the previous value from the old facility.

Without having a global look at your project, what you can do is to move that logic inside the setters of each property:

public class Facility {

    public void setSomething(String something) {
        if (something != null) {
            this.something = something;
        }
    }

}

This way, your update method would simply be:

private Facility updateFacility(Facility newFacility, Facility oldFacility) {
    oldFacility.setSomething(newFacility.getSomething());
    // etc for the rest
}

回答2:

I think you can apply Builder Pattern to resolve the issue, it may help you remove the frustration in the loop of if statement. Please see this link for more detials

回答3:

You can override hashCode and equals methods in Facility class and do as follows:

if(!newFacility.equals(oldFacility))
 {
    //only when something is changed in newFacility, this condition will be excecuted
    oldFacility = newFacility;
 }
 return oldFacility;
 //This is just and example, you can return newFacility directly

NOTE : You can include all params or only those which decide the uniqueness. Its up to you.
Hope this helps!

回答4:

You could copy the fields for the oldFacility object that you don't want to modify to some other variables, then update the whole oldFacility object, and just replace the fields that you didn't want to change with the content stored in the other variables. i.e.

private Facility updateFacility(Facility newFacility, Facility oldFacility){
    String contentNotToBeModified; // or whatever variable type
    contentNotToBeModified = oldFacility.getCity();
    // Do the same for all data that you want to keep

    oldFacility = newFacility;
    newFacility.setCity(contentNotToBeModified);
}

So copy the data that you want to keep out of oldFacility first, then substitute oldFacility for newFacility, and replace the required attributes of newFacility with the data from oldFacility.

回答5:

The not null check seems pointless to me since the NullPointerException won't be thrown if you slightly modify your example like this:

private Facility updateFacility(Facility newFacility, Facility oldFacility) {

if (newFacility != null) {

    oldFacility.setCity(newFacility.getCity());
    oldFacility.setContactEmail(newFacility.getContactEmail());
    oldFacility.setContactFax(newFacility.getContactFax());
    oldFacility.setContactName(newFacility.getContactName());
    ...
}

This will assign null values to references which were referencing to nulls anyway and will not cause any issues.

Assuming you were doing something like newFacility.getCity().toString() then the checks would be useful.

回答6:

You could use Java Reflection for avoiding that copy/paste/write-same-Problem:

public Facility updateFacility(Facility newFacility, Facility oldFacility)
{
    String[] properties = {"City", "ContactEmail", "ContactFax", "ContactName"};
    for(String prop : properties) {
        try {
            Method getter = Facility.class.getMethod("get"+prop);
            Method setter = Facility.class.getMethod("set"+prop, getter.getReturnType());
            Object newValue = getter.invoke(newFacility);
            if (newValue != null)
                setter.invoke(oldFacility, newValue);
        } catch (NoSuchMethodException | 
                SecurityException | 
                IllegalAccessException | 
                InvocationTargetException ex) {
             throw new RuntimeException(ex);
        }
    }
    ...
}

Now you can simple change the properties[] array when there are new properties in the Facility class which you want to update that way.

EDIT: If you use the return type of the getter method to find the setter method, it is not neccessary to assume that the properties of Facility are all of the same type.

CAVEATS: Be careful in method renaming! This code will lead to runtime errors if you rename or remove methods from the Facility class. If you have to possibility to change the code of the Facility class, you should consider using an annotation to indicate which properties should be updated.