Simplifying conditional logic for readibility

Question

I have following code that sets vrstaProizvoda.

private String napraviVrstuProizvoda(String kreditJeAktivanKod, String idArmPlana) {
    String vrstaProizvoda = null;
    if (kreditJeAktivanKod != null && kreditJeAktivanKod.equals("Y")) {
        vrstaProizvoda = VrstaProizvoda.STEP.value();
    } else if (idArmPlana != null && !idArmPlana.isEmpty() && !idArmPlana.equals("0000")){
        vrstaProizvoda = VrstaProizvoda.ARM.value();
    }
    return vrstaProizvoda;

}

Looking at else if statement, everything is negating values. Is there a better way to write idArmPlana condition so it is easier to read? Or is it not worth it?

`if (kreditJeAktivanKod != null && kreditJeAktivanKod.equals("Y"))` can be rewritten as `if ("Y".equals(kreditJeAktivanKod))` — JonK, Apr 24 '19 at 15:19

score 2 · Accepted Answer · answered Apr 24 '19 at 15:17

2

You could write something along the lines of:

!(idArmPlana == null || idArmPlana.isEmpty() || idArmPlana.equals("0000"))

The logic is still the same, but it is slightly more readable. Having long chains of and's or or's is never super readable, but doing something like this where you have your simple conditions, or them together, then negate the result can work.

answered Apr 24 '19 at 15:17

Fishy

1,275
1
12
26

9

This is an application of *DeMorgan's Law*. – Scott Hunter Apr 24 '19 at 15:18

acarlstein · Answer 2 · 2019-04-24T15:28:48.837

To make it easier to read, just create small functions that have a logical name:

private String napraviVrstuProizvoda(String kreditJeAktivanKod, String idArmPlana) {
    String vrstaProizvoda = null;
    if (isYes(kreditJeAktivanKod)) {
        vrstaProizvoda = VrstaProizvoda.STEP.value();
    } else if (!isZero(idArmPlana)){
        vrstaProizvoda = VrstaProizvoda.ARM.value();
    }
    return vrstaProizvoda;

}

function boolean isYes(String string){
  return (null != string && string.equals("Y");
}

function boolean isZero(String string){
 return (null != string && !string.isEmpty() && string.equals("0000");
}

score 1 · Answer 3 · answered Apr 24 '19 at 15:24

Not entirely sure about Arrays.asList, but the recurring idArmPlana could be used just once:

return "Y".equals(kreditJeAktivanKod)
        ? VrstaProizvoda.STEP.value()
        : !Arrays.<String>asList("", "0000", null).contains(idArmPlana)
        ? VrstaProizvoda.ARM.value()
        : null;

score 1 · Answer 4 · answered Apr 24 '19 at 15:32

1

Using Apache commons-lang3 library you can:

import org.apache.commns.lang3.StringUtils;

if (StringUtils.isNotBlank(StringUtils.stripStart(idArmPlana,"0")))

stripStart stolen from How to remove leading zeros from alphanumeric text?

answered Apr 24 '19 at 15:32

Conffusion

4,335
2
16
28

score 1 · Answer 5 · answered Apr 24 '19 at 15:45

My preference is one of these:

private String napraviVrstuProizvoda(String kreditJeAktivanKod, String idArmPlana) {
    if ( (kreditJeAktivanKod != null) && kreditJeAktivanKod.equals("Y") ) {
        return VrstaProizvoda.STEP.value();
    } else if ( (idArmPlana != null) && !idArmPlana.isEmpty() && !idArmPlana.equals("0000") ) {
        return VrstaProizvoda.ARM.value();
    } else {
        return null;
    }
}

private String napraviVrstuProizvoda(String kreditJeAktivanKod, String idArmPlana) {
    if ( strEquals(kreditJeAktivanKod, "Y") ) {
        return VrstaProizvoda.STEP.value();
    } else if ( !strIsEmpty(idArmPlana) && !strEquals(idArmPlana, "0000") ) {
        return VrstaProizvoda.ARM.value();
    } else {
        return null;
    }
}

Here are a number of rewrites to show a range of alternatives, and to show how to reach the above with incremental adjustments:

Rewritten with more spaces and parenthesis. This makes it easier to pick out the long variable names, and relieves the reader of all need to organize the expression logic:

private String napraviVrstuProizvoda(String kreditJeAktivanKod, String idArmPlana) {
    String vrstaProizvoda = null;
    if ( (kreditJeAktivanKod != null) && kreditJeAktivanKod.equals("Y") ) {
        vrstaProizvoda = VrstaProizvoda.STEP.value();
    } else if ( (idArmPlana != null) && !idArmPlana.isEmpty() && !idArmPlana.equals("0000") ) {
        vrstaProizvoda = VrstaProizvoda.ARM.value();
    }
    return vrstaProizvoda;
}

Rewritten to remove the default 'null' value. Having such a value is problematic. Consider if the logic were much more complex. Having a default value takes away the opportunity for the compiler to detect unhandled cases.

private String napraviVrstuProizvoda(String kreditJeAktivanKod, String idArmPlana) {
    String vrstaProizvoda;
    if ( (kreditJeAktivanKod != null) && kreditJeAktivanKod.equals("Y") ) {
        vrstaProizvoda = VrstaProizvoda.STEP.value();
    } else if ( (idArmPlana != null) && !idArmPlana.isEmpty() && !idArmPlana.equals("0000") ) {
        vrstaProizvoda = VrstaProizvoda.ARM.value();
    } else {
        vrstaProizvoda = null;
    }
    return vrstaProizvoda;
}

Rewritten with multiple return values. This is my preference, but some prefer a single return statement, as was present in the original method.

private String napraviVrstuProizvoda(String kreditJeAktivanKod, String idArmPlana) {
    if ( (kreditJeAktivanKod != null) && kreditJeAktivanKod.equals("Y") ) {
        return VrstaProizvoda.STEP.value();
    } else if ( (idArmPlana != null) && !idArmPlana.isEmpty() && !idArmPlana.equals("0000") ) {
        return VrstaProizvoda.ARM.value();
    } else {
        return null;
    }
}

Rewritten, with helper methods (see below). This is a little clearer, but at a cost of obscuring the test logic. Splitting code into a lot of small methods, while often encouraged, is not always preferred in practice.

private String napraviVrstuProizvoda(String kreditJeAktivanKod, String idArmPlana) {
    if ( strEquals(kreditJeAktivanKod, "Y") ) {
        return VrstaProizvoda.STEP.value();
    } else if ( !strIsEmpty(idArmPlana) && !strEquals(idArmPlana, "0000") ) {
        return VrstaProizvoda.STEP.value();
    } else {
        return null;
    }
}

Helper methods:

// Test that two strings are equal.  Handle null values.
private boolean strEquals(String value1, String value2) {
    if ( value1 == null ) {
        return ( value2 == null );
    } else if ( value2 == null ) {
        return false;
    } else {
        return value1.equals(value2);
    }
}

// Test that two strings are equal.  Handle null values.
private boolean strEquals(String value1, String value2) {
    boolean result;
    if ( value1 == null ) {
        result = ( value2 == null );
    } else if ( value2 == null ) {
        result = false;
    } else {
        result = value1.equals(value2);
    }
    return result;
}

// Test if a string is neither null nor empty.
private boolean strIsNotEmpty(String value) {
    return ( (value != null) && !value.isEmpty() );
}

Eritrean · Answer 6 · 2019-04-24T16:09:29.650

To add one more alternative to the already given good answers:

private String napraviVrstuProizvoda(String kreditJeAktivanKod, String idArmPlana) {
    return Optional.ofNullable(kreditJeAktivanKod).filter(e->e.equals("Y"))
           .isPresent()? VrstaProizvoda.STEP.value() :
           Optional.ofNullable(idArmPlana).filter(e->!e.equals("0000")).filter(e->!e.isEmpty())
           .isPresent()? VrstaProizvoda.ARM.value(): 
            null;
}

Simplifying conditional logic for readibility

6 Answers6