Avoiding nested if statements

Question

I currently have the following code. I am using nested if statements to return different errors based on different cases

if(!cipherFileRead.isEmpty() && !freqFileRead.isEmpty() ){

    if(freqChooser.getSelection()!=null){

        if(nearestFreq.isSelected()){
            file.writeToFile(decrypter.nearestFreq(cipherFileRead, freqFileRead), "output.txt");;
        }
        else if (rankingFreq.isSelected()){
            file.writeToFile(decrypter.byRanking(cipherFileRead, freqFileRead), "output.txt");;
        }

        returnMessage = "Succesfully decrypted to output.txt";
    }
    else{
        returnMessage = "Please select decryption type. Decryption unsuccesful";}
}
else{
    returnMessage = "Both files must be loaded and contain text. Decryption unsuccesful.";}

JOptionPane.showMessageDialog(null, returnMessage);
}  

Is there a more elegant way to achieve this without loads of horrible IF statements?

Answer 1

You can indeed reduce the level of nesting significantly. However I leave it up to you whether you like it or not.

Your code currently has three levels of nesting, that's relatively okay. Use comments to make your code more readable instead. However you may adapt some of the following tricks.

---

Is still unassigned

String returnMessage = null;

// Something is empty
if (cipherFileRead.isEmpty() || freqFileRead.isEmpty()) {
    returnMessage = "Both files must be loaded and contain text. Decryption unsuccessful.";
}

// No description type given
if (returnMessage == null && freqChooser.getSelection() == null) {
    returnMessage = "Please select decryption type. Decryption unsuccessful";
}

// Decrypt
if (returnMessage == null) {
    // Select technique
    String decryptedText = null;
    if (nearestFreq.isSelected()) {
        decryptedText = decrypter.nearestFreq(cipherFileRead, freqFileRead);
    } else if (rankingFreq.isSelected()) {
        decryptedText = decrypter.byRanking(cipherFileRead, freqFileRead);
    }

    // Write decrypted text
    file.writeToFile(decryptedText, "output.txt");

    returnMessage = "Successfully decrypted to output.txt";
}

// Show return message
JOptionPane.showMessageDialog(null, returnMessage);

So the trick was to check whether returnMessage is still unassigned.

The disadvantage of this style is that it slows the program a bit because it makes it harder for the processor to guess the conditions correct for pipelining. This is called branch prediction and all processors do that.

But if it enhances readability, for example by reducing a really deep nesting, you may use it.

---

Return from method

However if this is a method I would definitely give the advice to use such a style, but then by using return like:

public String decryptFile() {
    // Something is empty
    if (cipherFileRead.isEmpty() || freqFileRead.isEmpty()) {
        return "Both files must be loaded and contain text. Decryption unsuccessful.";
    }

    // No description type given
    if (freqChooser.getSelection() == null) {
        return "Please select decryption type. Decryption unsuccessful";
    }

    // Decrypt
    // Select technique to use
    String decryptedText = null;
    if (nearestFreq.isSelected()) {
        decryptedText = decrypter.nearestFreq(cipherFileRead, freqFileRead);
    } else if (rankingFreq.isSelected()) {
        decryptedText = decrypter.byRanking(cipherFileRead, freqFileRead);
    } else {
        // Out of techniques?
        return "No decryption technique can be applied.";
        // Or if this must not happen due to context, use:
        // throw new AssertionError();
    }

    // Write decrypted text
    file.writeToFile(decryptedText, "output.txt");

    return "Successfully decrypted to output.txt";
}

And the caller will then do it like:

// Show return message
JOptionPane.showMessageDialog(null, decryptFile());

In this scenario, next to reducing the nesting, you also reduce the amount of time you spend in the code itself.

You should definitely pack this inside a method. It should not be the decision of the decryption where and how to display the return message. Instead return the message and let the caller decide how to use it.

Note that in practice you probably want to add some arguments to the method and so on. Also you may want to use exceptions instead of plain String messages, like IllegalArgumentException.

Answer 2

I'll answer by using the principles of one of my favorite book called "Clean Code":

1. A function should do only one thing, if you have nested if like this your function is certainly doing more than one thing.

2. So that means, each if/else if/else block should then be in its own function. This solved the nested if problem.

3. And the extracted function should be given a descriptive name of what it does. This is even better then commenting it because comment can go out of date, or even irrelevant, and thus become misleading.

So if I were try to refactor your function with the above principles, I may come up with something like this:

public void displayDecryptResult() {
    String decryptResult = decrypt();
    JOptionPane.showMessageDialog(null, decryptResult);
}

private String decrypt() {
    String decryptResult;

    if(!cipherFileRead.isEmpty() && !freqFileRead.isEmpty() ){
        decryptResult = decryptForValidInputFiles();
    } else{
        decryptResult = "Both files must be loaded and contain text. Decryption unsuccesful.";
    }

    return decryptResult;
}

private String decryptForValidInputFiles() {
    if (freqChooser.getSelection() != null){
        message = decryptForValidInputFilesAndDecryptionType();
    } else {
        message = "Please select decryption type. Decryption unsuccesful";}
    }

    return message;
}

private String decryptForValidInputFilesAndDecryptionType() {
    if(nearestFreq.isSelected()){
        file.writeToFile(decrypter.nearestFreq(cipherFileRead, freqFileRead), "output.txt");;
    }
    else if (rankingFreq.isSelected()){
        file.writeToFile(decrypter.byRanking(cipherFileRead, freqFileRead), "output.txt");;
    }

    return "Succesfully decrypted to output.txt";
}

Some may find the function name is too descriptive that it becomes laughable. But after a long time you've written the code, and you have to debug it, you would thank you had given those descriptive name, so you can easily skim through the pseudocode like names to get idea of what it's trying to do.