Early Exits in small sections

[u/Lasagna6278]

Hello, I have been practicing early exits throughout my code and was told I probably shouldn't be using them in small cases like the following where nesting wouldn't bring the code out too much, this is c++ code:

int findMaxInArray(const int arr[], const int num_elements) {  
  int maxIndex = 0;

  for (int i = 1; i < num_elements; i++) {
    if (!(arr[i] > arr[maxIndex])) {
      continue;
    }
    maxIndex = i;
  }
  return arr[maxIndex];
}

[u/flag_ua]

In this case couldn’t you just invert the condition and have it set maxIndex within the if statement?

[u/Lasagna6278 → u/flag_ua]

Yes I could have, that was the question. In cases like this I actually have no idea if either way is better. In other cases (with nested if statements or larger code) I think it is much easier to read with early exits though.

[u/Theunbiasedputting]

Early exits are fine, but in this case you are not even exiting early, you just inverted the condition to use continue instead of wrapping the update in a block.

[u/edwbuck]

So many small issues.

int maxIndex = 0;
for (int i = 1; i < num_elements; i++) {
    if (arr[i] > arr[maxIndex]) {
      maxIndex = i;
    }
}
return arr[maxIndex];

but since you're not returning the index...

int maximum = arr[0];
for (int i = 1; i < num_elements; i++) {
    if (arr[i] > maximum) {
      maximum = arr[i];
    }
}
return maximum;

And you might want to check that arr has at least one element.

You really aren't exiting anything early in this code. You're just skipping over the assignment. "continue" isn't an early exit.

for (int i = 1; i < num_elements; i++) {
    if (!(arr[i] > arr[maxIndex])) {
      continue;
    }
    maxIndex = i;
  }

refactors into

for (int i = 1; i < num_elements; i++) {
    if (!(arr[i] > arr[maxIndex])) {
      continue;
    } else {
    }
    maxIndex = i;
  }

refactors into

for (int i = 1; i < num_elements; i++) {
    if (!(arr[i] > arr[maxIndex])) {
      continue;
      maxIndex = i;
    } else {
      maxIndex = i;
    }
  }

refactors into

for (int i = 1; i < num_elements; i++) {
    if (!(arr[i] > arr[maxIndex])) {
      continue;
    } else {
      maxIndex = i;
    }
  }

refactors into

for (int i = 1; i < num_elements; i++) {
    if (arr[i] > arr[maxIndex]) {
       maxIndex = i; 
    } else {
      continue;
    }
  }

refactors into

for (int i = 1; i < num_elements; i++) {
    if (arr[i] > arr[maxIndex]) {
       maxIndex = i; 
    }
  }

and now the loop is both smaller, and easier to read.

[u/Available_Safe1818 → u/edwbuck]

I was hoping you'd keep going with more refactors. I was really getting into it.

Also, why not just go crazy with something like this?:

for (int i = 1; i < num_elements; i++) if (arr[i] > arr[maxIndex]) maxIndex = i;

[u/desrtfx]

If you use continue (which is not an early exit, BTW) in your loop, you should directly stop and think again.

There are reasons to use it, but you have to reason its use very well. Your use case is not one of the reasons.

In your case, invert the condition and set maxindex directly. Makes the code much cleaner and much easier to read and follow.

In short: avoid continue.

SImilar for break - yes, it has its justification and perfect use cases, but it should never become a default approach. There are proper, better approaches in way too many cases.

[u/Available_Safe1818]

Every time you do this:

if (!(arr[i] > arr[maxIndex])) {

instead of

if (arr[i] <= arr[maxIndex]) {

you should slap yourself.

[u/serge-mv]

I am guessing, you are trying to avoid things like wrapping the whole body of a function in an if-statement, like if argument is not null, proceed to do the main task.

While the idea is good for reducing the amount of nested structures, it's not a universal rule that should be just used in everything. You do things that make sense. In this case it makes more sense to have a positive condition and do work inside the if-statement. This is exactly what the conditionals are for. You made your code more complex and the condition is actually doing nothing here. I mean your condition is for when is work skipped, rather than for when the work is done. There is no reason to do it this way.

[u/Natehhggh]

I wouldn't overthink it in small cases like this, do what makes more sense. you can throw both into godbolt, and they compile the the same assembly.

https://godbolt.org/z/azdh3a1Yo

https://godbolt.org/z/o5nesPe3E

The real worry with early returns is badly nested if/else statements that are hard to understand, where maybe you might need to add something that always needs to run after like logging for example.. And even in those cases the problem usually is more from the if/else statements than the early returns.

[u/ComputerWhiz_]

Early exits and guard clauses can be a nice way of reducing nesting, but they can also make it more challenging to follow the code. If you use a lot of them, you get these cases where your method now have multiple exit points, opposed to basically being able to just read the method from top to bottom.

They can also be brittle if another developer (or you) adds code later in a method, expecting it to run, not realizing that it may not because of some early exit elsewhere.

In your code example, the benefit gained by using the early exit is minimal compared to just having the code in the of statement. The other thing you've introduced is that now you have the if statement condition inverted, which can also make it more difficult to read.