CODEWITHSUNDEEP
javaif-statementnull-check
developer new
developer new

Reputation: 51

Java reduce cognitive complexity of method which has multiple if statements

Hello I am looking for a way to reduce cognitive complexity of method which has multiple if statements. All are basically nullchecks for each item of object and is required.

Code snippet:

private void assignFruit(FruitBasket1<?> fruit1, FruitBasket2 fruit2) {
        
        if (fruit1.name != null) {
            fruit2.setName(fruit1.name.toString());
        }

        if (fruit1.number != null) {
            fruit2.setNumber(fruit1.number.toString());
        }

        if (fruit1.color != null) {
            fruit2.setColor(fruit1.color.toString());
        }

        if (fruit1.region != null) {
            fruit2.setRegion(fruit1.region.toString());
        }
        
}

I have tried to use switch case but since its only null check didnt find feasible, also tried ternary operator. Is there a simpler way to do this as although in the code snippet its only few fields there are almost 20 fields which need be null checked. What are the other possibilities to do this simple manner without modifying the POJO class and managing in this method itself.

Note: There was an error in the code in the way values are set

Upvotes: -1

Views: 502

Answers (5)

akop
akop

Reputation: 7871

You can use Optional<>.

// static imported Optional.ofNullable()
private void assignFruit(FruitBasket1<?> fruit1, FruitBasket2 fruit2) {
   ofNullable(fruit1.name).ifPresent(fruit2::setName);
   ofNullable(fruit1.number).ifPresent(fruit2::setNumber);
   ofNullable(fruit1.color).ifPresent(fruit2::setColor);
   ofNullable(fruit1.region).ifPresent(fruit2::setRegion);
}

You can improve it a little bit more with a static import of ofNullable().

Edit

I'm not sure, but you need to change the type somehow?
Also that is fluent possible with Optional.

// static imported Optional.ofNullable()
private void assignFruit(FruitBasket1<?> fruit1, FruitBasket2 fruit2) {
   ofNullable(fruit1.name).map(Object::toString).ifPresent(fruit2::setName);
   ofNullable(fruit1.number).map(Object::toString).ifPresent(fruit2::setNumber);
   ofNullable(fruit1.color).map(Object::toString).ifPresent(fruit2::setColor);
   ofNullable(fruit1.region).map(Object::toString).ifPresent(fruit2::setRegion);
}

Upvotes: 0

WJS
WJS

Reputation: 40047

First, if these attributes are not supposed to be null, how do they get to be null in the first place? And if they are null, why not just copy them? Without fully understanding your use case, it doesn't make much sense. Especially when one wonders what the getter would return.

In any event, presuming that you never want to set the fruit in a basket if it is null, then enforce that invariant in the setMethod itself.

class FruitBasket2 {
   public void setName(FruitBasket1<?> fb) {
         if(fb.name != null) {
             this.name = fb.name;
          }
   }
   ...
   ...
}

Consider that if someone were to use your class FruitBasket, why make the user check for null before setting the value?

Upvotes: 0

hermit
hermit

Reputation: 1117

If you are willing to add an additional dependency, MapStruct is a very good choice to map Beans/Dtos in a very readable manner.

You could simply add an interface and map these two objects.

In order to ignore null values, you need to set the property, nullValuePropertyMappingStrategy to NullValuePropertyMappingStrategy.IGNORE

Thus, the final code looks like:

@Mapper(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE )
public interface SimpleSourceDestinationMapper {
    FruitBasket2 sourceToDestination(FruitBasket1 fruitBasket1);
}

Since, FruitBasket1 and FruitBasket2 have same attributes, it will automatically map them all except the null values.

Note: It is just a more readable version of the mapping. In terms of complexity, it is the same.

Guides to Mapstruct: https://www.baeldung.com/mapstruct

Upvotes: 3

Maurice Perry
Maurice Perry

Reputation: 9658

Yes, you can also define the following method:

private static <T> void setIfNotNull(T value, Consumer<T> cons) {
    if (value != null) {
        cons.accept(value);
    }
}

And use it as follows:

    setIfNotNull(fruit1.name, fruit2::setName);
    setIfNotNull(fruit1.number, fruit2::setNumber);
    setIfNotNull(fruit1.color, fruit2::setColor);
    setIfNotNull(fruit1.region, fruit2::setRegion);

Upvotes: 0

f1sh
f1sh

Reputation: 11941

You can extract that logic by using method references for the getters and setters.

For example you can extract Fruit::getColor to a Function<Fruit, String> and squeeze the setter into a BiConsumer<Fruit, String>.

Then you can match each getter to a setter using some generic Pair such as a simple

record Pair<R,L> (R a, L b){}

Then you can store these Pairs into a List and iterate over them:

    List<Pair<Function<Fruit, String>, BiConsumer<Fruit, String>>> getterSetterPair = new ArrayList<>();
    //"connect" getters and setters here
    getterSetterPair.add(new Pair<>(Fruit::getColor, Fruit::setColor));
    getterSetterPair.add(new Pair<>(Fruit::getName, Fruit::setName));

    Fruit fruit1 = null, fruit2 = null;
    for(Pair<Function<Fruit, String>, BiConsumer<Fruit, String>>pair:getterSetterPair) {
        Function<Fruit, String> getter = pair.a();
        BiConsumer<Fruit, String> setter = pair.b();
        //this is doing the logic you did before
        String value = getter.apply(fruit1);
        if(value != null) {
            setter.accept(fruit2, value);
        }
    }

In general, I would also advise you to use a proven library such as MapStruct (see @hermit's) answer for that.

Upvotes: 0

Related Questions