Early out makes the error conditions explicit, which is what one is interested in 90% of the time. After the last if you know that all of the above conditions are false, so you don’t need to keep them in your head.
And this is just a silly example with 3 predicates, imagine how a full function with lots of state looks. You would need to keep the entire decision tree in your head at all times. That’s the opposite of maintainable.
I’m sure you are capable of rewriting that in 3 lines and a single nested scope followed by a single return. In fact in languages that use exceptions you have to use at least one subscope.
Notice that in my example I didn’t even broach the example with error conditions, cause it’s trivial to write cleanly either way. Instead I talked about returns inside business logic. You can’t unfuck that. 🐞
Since my previous example didn’t really have return value, I am changing it slightly. So if I’m reading your suggestion of “rewriting that in 3 lines and a single nested scope followed by a single return”, I think you mean it like this?
int retval = 0;
// precondition checks:if (!p1) retval = -ERROR1;
if (p2) retval = -ERROR2;
if (!p3 && p4) retval = -ERROR3;
// business logic:if (p1 && !p2 && (p3 || !p4))
{
retval = 42;
}
// or perhaps would you prefer the business logic check be like this?if (retval != -ERROR1 && retval != -ERROR2 && retval != -ERROR3)
{
retval = 42;
}
// or perhaps you'd split the business logic predicate like this? (Assuming the predicates only have a value of 0 or 1)int ok = p1;
ok &= !p2;
ok &= p3 || !p4;
if (ok)
{
retval = 42;
}
return retval;
Using a retval has the exact problem that you want to avoid: at the point where we do return retval, we have no idea how retval was manipulated, or if it was set multiple times by different branches.
It’s mutable state inside the function, so any line from when the variable is defined to when return retval is hit must now be examined to know why retval has the value that it has.
Not to mention that the business logic then needs to be guarded with some predicate, because we can’t early return. And if you need to add another precondition check, you need to add another (but inverted) predicate to the business logic check.
You also mentioned resource leaks, and I find that a more compelling argument for having only a single return. Readability and understandability (both of which directly correlate to maintainability) are undeniably better with early returns. But if you hit an early return after you have allocated resources, you have a resource leak.
Still, there are better solutions to the resource leak problem than to clobber your functions into an unreadable mess. Here’s a couple options I can think of.
Don’t: allow early returns only before allocating resources via a code standard. Allows many of the benfits of early returns, but could be confusing due to using both early returns and a retval in the business logic
If your language supports it, use RAII
If your language supports it, use defer
You can always write a cleanup function
Example of option 1
// precondition checksif(!p1) return -ERROR1;
if(p2) return -ERROR2;
if(!p3 && p4) return -ERROR3;
void* pResource = allocResource();
int retval = 0;
// ...// some business logic, no return allowed// ...
freeResource(pResource);
return retval; // no leaks
Example of option 2
// same precondition checks with early returns, won't repeat them for brevityauto Resource = allocResource();
// ...// some business logic, return allowed, the destructor of Resource will be called when it goes out of scope, freeing the resources. No leaks// ...return42;
Example of option 3
// precondition checksvoid* pResource = allocResource();
defer freeResource(pResource);
// ...// some business logic, return allowed, deferred statements will be executed before return. No leaks// ...return42;
Example of option 4
intfreeAndReturn(void* pResource, constint retval)
{
freeResource(pResource);
return retval;
}
intdoWork()
{
// precondition checksvoid* pResource = allocResource();
// ...// some business logic, return allowed only in the same form as the following line// ...return freeAndReturn(pResource, 42);
}
Not sure why you had to do the inverted predicate check again in your first example. You already have the information encoded in the value of retval. It can be written like this:
int result = 0;
if (!p1) result = -ERROR1;
if (p2) result = -ERROR2;
if (!p3 && p4) result = -ERROR3;
if (result != 0) {
result = 42;
}
return result;
With a return value you have to add 4 extra lines. This overhead remains constant as you add more checks and more business logic.
Yes all the other suggestions are better than early returns in business logic and would help with leaks. Would be nice if we had RAII outside of C++. I think Rust has it? Haven’t done Rust yet.
Bad advice. Early return is way easier to parse and comprehend.
if (p1) { if(!p2) { if(p3 || !p4) { *pOut = 10; } } }
vs
if(!p1) return; if(p2) return; if(!p3 && p4) return; *pOut = 10;
Early out makes the error conditions explicit, which is what one is interested in 90% of the time. After the last if you know that all of the above conditions are false, so you don’t need to keep them in your head.
And this is just a silly example with 3 predicates, imagine how a full function with lots of state looks. You would need to keep the entire decision tree in your head at all times. That’s the opposite of maintainable.
I’m sure you are capable of rewriting that in 3 lines and a single nested scope followed by a single return. In fact in languages that use exceptions you have to use at least one subscope.
Notice that in my example I didn’t even broach the example with error conditions, cause it’s trivial to write cleanly either way. Instead I talked about returns inside business logic. You can’t unfuck that. 🐞
Since my previous example didn’t really have return value, I am changing it slightly. So if I’m reading your suggestion of “rewriting that in 3 lines and a single nested scope followed by a single return”, I think you mean it like this?
int retval = 0; // precondition checks: if (!p1) retval = -ERROR1; if (p2) retval = -ERROR2; if (!p3 && p4) retval = -ERROR3; // business logic: if (p1 && !p2 && (p3 || !p4)) { retval = 42; } // or perhaps would you prefer the business logic check be like this? if (retval != -ERROR1 && retval != -ERROR2 && retval != -ERROR3) { retval = 42; } // or perhaps you'd split the business logic predicate like this? (Assuming the predicates only have a value of 0 or 1) int ok = p1; ok &= !p2; ok &= p3 || !p4; if (ok) { retval = 42; } return retval;
as opposed to this?
// precondition checks: if(!p1) return -ERROR1; if(p2) return -ERROR2; if(!p3 && p4) return -ERROR3; // business logic: return 42;
Using a retval has the exact problem that you want to avoid: at the point where we do
return retval
, we have no idea howretval
was manipulated, or if it was set multiple times by different branches. It’s mutable state inside the function, so any line from when the variable is defined to whenreturn retval
is hit must now be examined to know whyretval
has the value that it has.Not to mention that the business logic then needs to be guarded with some predicate, because we can’t early return. And if you need to add another precondition check, you need to add another (but inverted) predicate to the business logic check.
You also mentioned resource leaks, and I find that a more compelling argument for having only a single return. Readability and understandability (both of which directly correlate to maintainability) are undeniably better with early returns. But if you hit an early return after you have allocated resources, you have a resource leak.
Still, there are better solutions to the resource leak problem than to clobber your functions into an unreadable mess. Here’s a couple options I can think of.
defer
Example of option 1
// precondition checks if(!p1) return -ERROR1; if(p2) return -ERROR2; if(!p3 && p4) return -ERROR3; void* pResource = allocResource(); int retval = 0; // ... // some business logic, no return allowed // ... freeResource(pResource); return retval; // no leaks
Example of option 2
// same precondition checks with early returns, won't repeat them for brevity auto Resource = allocResource(); // ... // some business logic, return allowed, the destructor of Resource will be called when it goes out of scope, freeing the resources. No leaks // ... return 42;
Example of option 3
// precondition checks void* pResource = allocResource(); defer freeResource(pResource); // ... // some business logic, return allowed, deferred statements will be executed before return. No leaks // ... return 42;
Example of option 4
int freeAndReturn(void* pResource, const int retval) { freeResource(pResource); return retval; } int doWork() { // precondition checks void* pResource = allocResource(); // ... // some business logic, return allowed only in the same form as the following line // ... return freeAndReturn(pResource, 42); }
Not sure why you had to do the inverted predicate check again in your first example. You already have the information encoded in the value of retval. It can be written like this:
With a return value you have to add 4 extra lines. This overhead remains constant as you add more checks and more business logic.
Yes all the other suggestions are better than early returns in business logic and would help with leaks. Would be nice if we had RAII outside of C++. I think Rust has it? Haven’t done Rust yet.
goto is used in C for this exact kind of early return management. The person you answered to does not maintain code I think
goto cleanup is not the same as return. I didn’t badmouth goto cleanup.
This is virtually the same thing with a different keyword, I’d like to hear where you (and the down voters) draw the line.