Execute sequence of methods conditionally and log possible error

Question

I'm not sure if this is the adequate site (maybe CodeReview?) but it's the only one here in StackExchange that have got a "clean code" tag ... There is no need for downvotes if this is not the correct place just inform me in the comments and I will move it.

I've got this piece of java code

 private void executeCommand() {

    boolean part1ExecutionOK = executePart1();
    boolean part2ExecutionOK = false;
    boolean part3ExecutionOK = false;

    if (part1ExecutionOK) {
         part2ExecutionOK = executePart2();

         if (part2ExecutionOK) {
               part3ExecutionOK = executePart3();

               if (part3ExecutionOK) {
                    finishCommand();
               }
         }
    }

    // [*] Log what happened depending on boolean flags
}

I know that I can simplify it just like this

private void executeCommand() {

    if (executePart1() && executePart2() && executePart3()) {
         finishCommand();
    } else {
         // [*] Log what happened - I don't know which part failed!
    }
}

I could throw a specific exception from each execute method and capture it

 private boolean executePart1() throws MyException {

      // Do whatever

      if (!checkConditionsPart1()) {
           throw new MyException(ERROR_CODE_1);
      }

      return true;
 }

 ...


private void executeCommand() {

    try {
         if (executePart1() && executePart2() && executePart3()) {
               finishCommand();
         } 

         // [*] Log what happened -> Execution OK

    } catch (MyException e) {
        // [*] Log what happened -> Execution KO depending on e.getMyErrorCode();
    }
}

But I'm not confortable throwing exceptions because a method executePart that not accomplish its expected conditions is not an exceptional behaviour.

Is there a better way to structure the secuential but conditional execution of my three methods and log which one of them has failed?

Answer 1

My opinion is this question would be better suited on Code Review but, anyway, here is what I would do.

The nested ifs of your first code are really ugly, and they will become even worse if, in the future, you won't have 4 steps but 10.

You could try this:

private void executeCommand() {
    if (!executePart1()) {
        // Log your error about part 1
        return;
    }

    if (!executePart2()) {
        // Log your error about part 2
        return;
    }

    if (!executePart3()) {
        // Log your error about part 3
        return;
    }

     finishCommand();
}

So you won't have to throw exceptions and you won't have deeply nested logic.

---

Edit because of your first comment:

If you cannot have multiple return statements, well maybe you could try to change your project's clean code requirements because, in my opinion, multiple return statements greatly improve the code's readability.

I understand this when you explain what your function must do: execute part 1, then maybe stop the execution. Then execute part 2, then maybe stop the execution. etc. If you translate to code, you get exactly this:

executePart1();
if (some condition) {
    return; // stop the execution
}
// etc...

Ok let's imagine you cannot change those requirements. You could do this:

private void executeCommand() {
    List<Supplier<Boolean>> parts = Arrays.asList(
        () -> executePart1(),
        () -> executePart2(),
        () -> executePart3(),
        () -> finishCommand()
    );
    int partNumber = 1;
    for (Supplier<Boolean> part : parts) {
        if (!part.get()) {
            // Log about part #partNumber
            break; // Aha! this is not a return statement!
        }
        partNumber++;
    }
}

With only 4 parts, this solutions is less readable than the previous one. But in the case the number of steps increases, it may even be more elegant.

Answer 2

Early return is the most useful solution, but if you must not use them, then you could do:

 private void executeCommand() {
    boolean bOK = true;

    if (bOK) {
      bOK = executePart1();
      if (!bOK) {
        // error handling
      }
    }

    if (bOK) {
      bOK = executePart2();
      if (!bOK) {
        // error handling
      }
    }

    if (bOK) {
      bOK = executePart3();
      if (!bOK) {
        // error handling
      }
    }

    if (bOK) {
      finishCommand();
    }
}

Which, like the other answer with the for loop, just goes to show how stupid forbidding multiple return is.