Style guide: Best practice on if/return

[javier-godoy]

We should have some guidelines on how to use if/return (based on https://softwareengineering.stackexchange.com/q/157407/134681)

Which of the following examples are good practices, and which are bad practices? Why? Does the recommended practice vary depending on whether "something more here" is just a few lines or several nested statements?

Example 1A:

public boolean MyFunction(String myString) {
   if (myString == null) {
      return false;
   } else {
      // Do something more here...
      return true;
   }
}

Example 1B:

public boolean MyFunction(String myString) {
   if (myString == null) {
      return false;
   } 
   // Do something more here...
   return true;  
}

Example 2A:

public void MyFunction(String myString) {
   if (myString != null) {
      // Do something more here...
   }
}

Example 2B:

public void MyFunction(String myString) {
   if (myString == null) {
      return;
   } 
   // Do something more here...
}

[elPeiretti]

IMO Example 2B is easier to read and allows to prevent an extra indentation level. But I think it should only be used as guard blocks at the start of a method/function. Otherwise the code can easily become very messy and harder to read with multiple exit points.

public boolean okMethod(Integer param1, double param2) {
    if (param1 == null || param2 <0) {
        return false;
    }
    if (another check) {
        return false;
    }

     // Do something more here...
}

public boolean notOkMethod(Integer param1) {
     // Do something more here...
     if(complex condition) {
       return true;
     }
     // Do something more here...
    if (complex condition2) {
         return false;
    }
     // Do something more here...
}

Regarding Example 1A, I think the return statement should be after the if-else clause if there is something to do when myString==null. Otherwise, example 2B should be applied.

public boolean MyFunction(String myString) {
   if (myString == null) {
      // Do something more here...
   } else {
      // Do something more here...
   }

   return condition;
}

After all, I feel this ends up depending entirely on the coder's taste which is reflected on the shared blogpost. Most of the comments starts with My personal practice, I prefer, I like, etc. even my own suggestions.

[javier-godoy]

Most of the comments starts with My personal practice ...

Indeeed, my personal practice has been 1A/2A for simple cases, and 1B/2B for more compex cases where the if block checks preconditions.

I've GPTed some arguments for 1B/2B in complex cases in more complex cases, and I find myself agreeing with them:

Reduces Redundant Code: Example 1A has a redundant else block. Since the if statement returns a value and exits the function if myString is null, there's no need for an else block. The code after the if statement will only execute if the condition is false.

Simplifies Logic: Example 1B simplifies the logic by avoiding unnecessary nesting. This makes the code easier to read and understand because there is no need to mentally track an additional block scope.

Cleaner and More Readable: The code in Example 1B is cleaner and more straightforward. It follows a common pattern known as "early return" or "guard clause," which makes it easier to follow the flow of the function, especially in more complex scenarios.

Less Indentation: Avoiding the else block reduces indentation, which helps keep the code more horizontally compact. This contributes to better readability, especially in large codebases.

It also suggested that Example 1B is still better, even if the // Do something more here... is a single line, but I’m still not convinced:

Consistency: Keeping the early return structure makes the code consistent. Whether the additional logic is a single line or multiple lines, using the same pattern improves readability across different parts of the code.

Simplicity: Even with a single line of code, avoiding the else block simplifies the structure and reduces the cognitive load on the reader. The reader doesn't have to track whether the code is inside an else block.

Maintainability: If the single line of code eventually expands into multiple lines, Example 1B remains easier to maintain. You won't need to refactor to remove unnecessary nesting later on.

[ngonzalezpazFC]

In my opinion, using early returns with guard blocks and avoiding unnecessary else blocks (1A) leads to cleaner and more maintainable code. This is why I prefer example 1B over 1A and 2B over 2A. Also, avoiding the use of guard blocks tends to cause arrow code which can be harder to read. Regarding example 2, I prefer version B because it explicitly handles the null case with an early return, making the code clearer for the reader. This makes clear that the function should exit early, and focuses on the main logic afterward. I prefer to handle invalid cases at the beginning and then the subsequent logic, specially if it is more complex.