What's the better option?

Question

procedure MyProc(Eval: Boolean);  
begin       
    if not Eval then  
        Exit;  
    /* do stuff */  
    /* do more stuff */
end;

OR

procedure MyProc(Eval: Boolean);  
begin  
   if Eval then  
      begin  
         /* do stuff */  
         /* do more stuff */  
      end;

  /* no Exit needed, but now we got what I think unpleasing code:
  having a indentation level and a begin-end statement */
end;

Answer 1

There are cases when either method is appropiate. Typically, however, the first offers more readable code and better control flow. I'm not very familiar with Delphi programming, but in C# I always try to use the former when possible. By the look of it, I see no reason that the approach should differ in Delphi.

To list a few of the advantages:

• No need to indent subsequent code.
• Easier to extend to multiple conditions. (Just add additional if statements rather than logical operators, which makes things clearer IMO.)
• The idea that you are "opting out" of the method rather than opting in is more aesthetically pleasing, since the following code should be executed in the "normal" case.

Still, there are situations where the second option is more appropiate. In particular, when the method needs to be broken into sub-sections (though this is often a sign that you need to refactor).

Answer 2

I think this is a matter of preference. However if you have a number of checks which you must perform before doing the 'real work' then the first form (IMO) looks tidier, and is easier to follow the flow. For example:

procedure MyProc(Eval: Boolean);
begin
    if not Eval then
        Exit;
    if not Eval2 then
        Exit;
    /* do stuff */
    /* do more stuff */ 
end;

v.s.

procedure MyProc(Eval: Boolean);
begin
    if Eval then
    begin
        if Eval2 then
        begin
            /* do stuff */
            /* do more stuff */
        end;
    end;
end;

Answer 3

There are those that will argue you should only have one exit point in your function but I'm not in that camp.

I frequently use multi-returns and it doesn't affect the readability of short well-thought-out functions so I'd say just go for whatever you think is the more readable.

I've had people refactor code I've written to follow the "rules" and it's almost always ended up with a morass of unreadable code due mostly to excessive indentation. They should have either left it alne or done it properly, breaking it into more functions.

One particular annoyance I see is the likes of the "else" in:

if (some condition)
    return false;
else
    keep going;

Do they think the flow of control is somehow going to escape from the "then" clause?

Answer 4

I use the first form a lot. It is a bit literal programming of pre conditions, specially because when code is inserted between the actual checks in the second example, the exact conditions are less clear.

I do try to limit EXIT use to the first condition check part of the procedure though, to avoid too much spaghetti, but I think the first form is cleaner than the hoops to jump to preserve that one exit point.

And it seems to be used a lot, considering the fact that exit(returnvalue) was added in D2009 (and FPC having it for a decade)

Answer 5

The reality is this is too simple an example to prevaricate on. You'll choose your approach based on your existing convention -- and this example is too straightforward to make a decision on convention. What's more interesting is what to do with multiple get-out-now options and nested IF statements - it's a real grey area.

So what is my convention? Ah... I am pragmatic, I don't have any rules. If it's possible to jump out the window in the first few lines, I take it, because I don't have to remember whenever I edit the code that some "unrequited function call" could still be live at any point during the function.

Over time this function will be moulded and kneaded by bugs and enhancements. You are more likely to introduce a new bug with the latter approach than the former, because the quick-exit is obvious at the start, in your face, and you don't need to worry about it "forgotten" 50 lines down the function. In my bug-introducing experience, of course.

This is a more tricky call if you set things up and have to tear them down as jumping out can force you to juggle 17 Schrodinger's cat states in your head when making changes.

Answer 6

In my opinion, neither is the correct answer.

The correct solution would be to only include the "do stuff" and "do more stuff" portions in the procedure. Then, wrap the procedure call in an if statement, only calling it when necessary.

If, as you say in your comment, that "do stuff" and "do more stuff" cover two different domains, then perhaps you want two procedures - one to "do stuff" and another to "do more stuff". If you perform both of the procedures together often enough, you can also include a third procedure that simply calls the other two procedures.

Answer 7

Can I just make a plea that if you do use the second form, you don't add a gratuitous ectra level of indentation. So insrtead of:

procedure MyProc(Eval: Boolean);  
begin  
   if Eval then  
      begin  
         /* do stuff */  
         /* do more stuff */  
      end;

  /* no Exit needed, but now we got what I think unpleasing code:
  having a indentation level and a begin-end statement */
end;

say:

procedure MyProc(Eval: Boolean);  
begin  
   if Eval then  
   begin  
      /* do stuff */  
      /* do more stuff */  
   end;

  /* no Exit needed, but now we got what I think unpleasing code:
  having a indentation level and a begin-end statement */
end;

Answer 8

I think the less exit points a code segment has the better so it's easier to read and understand. Noting is worse than debugging someone else's 100 line function and discovering there are 12 different situations peppered inside of it that can cause it to return early.

Answer 9

I prefer:

if not Eval then Exit;

because the code looks cleaner that way.