CODEWITHSUNDEEP
javaif-statementnestedrefactoring
bez
bez

Reputation: 187

refactoring nested if statements

I'm trying to refactor some code that looks similar to the code below. I feel that there should be a simpler way to do what it does. My main concern is the call to getDataB() in both if and else blocks of the (!dataA.isEmpty()) condition.

    DataA dataA = getDataA();

    if (!dataA.isEmpty()) {

        if (dataA.getStatus() == Status.FINAL) {
            // process finalized dataA
        }

        if (dataA.getStatus() == Status.RECEIVED) {
            DataB dataB = getDataB();
            if (!dataB.isEmpty()) {
                dataA.setStatus(dataB.getStatus());
                // process dataA with updated status from dataB
            }
        }

        if (dataA.getStatus() == STATUS.IN_PROGRESS) {
            // process in progress dataA
        }

    // empty dataA
    } else {
        Datab dataB = getDataB();
        if (!dataB.isEmpty()) {
            // process dataB
        }
    }

Upvotes: 1

Views: 166

Answers (3)

displayName
displayName

Reputation: 14369

First, take the processing of dataB (present in the bottom else in your code) out in its own function like:

private void processDataB() {
    DataB dataB = getDataB();
    if (dataB.isEmpty()) return;
    // process dataB
}

Notice how we inverted the if condition to return from the method if dataB is empty.

Next, apply the same inverting of if to the main code. The new code will look like:

DataA dataA = getDataA();
if (dataA.isEmpty()) { //Inverted the test.
    processDataB();
    return;
}

if (dataA.getStatus() == Status.FINAL) {
    // process finalized dataA
}

if (dataA.getStatus() == Status.RECEIVED) {
    DataB dataB = getDataB();
    if (!dataB.isEmpty()) {
        dataA.setStatus(dataB.getStatus());
        // process dataA with updated status from dataB
    }
}

if (dataA.getStatus() == STATUS.IN_PROGRESS) {
    // process in progress dataA
}

Upvotes: 1

Lionel Cichero
Lionel Cichero

Reputation: 563

There is still room for improvement, but as an initial refactoring you can do something like this:

// I like to get rid of the negations, assuming that DataA and DataB are your objects.
// I would normally declare a method with more meaning, like hasData(), otherwise just use the negation and the isEmpty.

DataA dataA = getDataA();
DataB dataB = getDataB();

// When DataA has data
if (dataA.hasData()) {  
    Status statusA = dataA.getStatus(); 

    switch(statusA) {
        case Status.FINAL:
            // process finalized dataA
            break;
        case Status.RECEIVED:
            if (dataB.hasData()) {
                dataA.setStatus(dataB.getStatus());
                // process dataA with updated status from dataB
            }
            break; // If you want to process dataA in progress after setting the dataB status in A, then remove this break.
        case Status.IN_PROGRESS:
            // process in progress dataA
            break;          
    }

// When DataB has data
} else if (dataB.hasData()) {
    // process dataB
}

Upvotes: 1

shmosel
shmosel

Reputation: 50716

You can move the duplicate logic to a method that accepts a lambda:

void processDataB(Consumer<DataB> action) {
    DataB dataB = getDataB();
    if (!dataB.isEmpty()) {
        action.accept(dataB);
    }
}

Then call it like this:

//...
    if (dataA.getStatus() == Status.RECEIVED) {
        processDataB(dataB -> {
            dataA.setStatus(dataB.getStatus());
            // process dataA with updated status from dataB
        });
    }
//...
} else {
    processDataB(dataB ->
        // process dataB
    });
}

Upvotes: 0

Related Questions