Writing Readable Code - Formatting and Comments

I guess there are at least as many coding styles as there are programmers times programming languages. Many of us are conformists, and some of us have our highly personal style, but most of us probably fail at being 100% consistant. Code formatting can probably be considered a slightly religious area, so please feel free to disagree with me :)

So - why bother to put any effort into the formatting anyways? It is not like the compiler care? I mean, apart from the obvious requirement that things should be reasonable readable and not just a jumble of code?

Layout matters. It is significantly easier to pick up code that is consistant in style than wading through spaghetti (or gnocchi) code where the indentation has not been considered much.

I have noticed that my personal style with regards to block formatting definitively is non-conformist. Whenever there is a do, if/then/else, with or without related begin/end, I will probably annoy the heck out of the conformists.

My formatting goals:
• I want clear indication of flow
• I want to separate conditions from actions
• I want room to comment

In the examples below, I will place comments in the individualist examples to give an indication of why I choose to wrap/indent in this way rather than the "global standard"

Let's start with the do in with and the for-loop (I do at times, albeit rarely, confess to the occasional use of with).

// Conformist
  for i := 1 to 5 do Something;

  for i := 1 to 5 do begin
    Something;
    SomethingMore;
  end;
  
  with SomeObject do begin
    Something;
    SomethingMore;
  end;
  
// Individualist
  for i := 1 to 5     // For selected range
   do Something(i);   // get stuff done

  for i := 0 to (count - 1) // For every item
  do begin              
    Something(i);       // Step 1
    SomethingMore(i);   // Step 2
  end;
  
  with SomeObject  // Focusing on this specific item,  
  do begin         // I can add this comment for clarity
    Something;
    SomethingMore;
  end;

The if/then/else statement have too many combinations to even get close to cover them all, so I am only going to do a handful.

// Conformist
  if (SomeVariable = Condition) and (SomeOtherExpression) then
    PerformSomeRoutine;
    
  if (SomeVariable = Condition) and (SomeOtherExpression) then begin
    Code;
    MoreCode;
    LotsOfCode;
  end else OtherCode;

  if Condition then DoFirstAlternative else DoSecondAlternative;
  
  if FirstCondition then DoFirstAlternative 
  else if SecondCondition then DoSecondAlternative
  else DoThirdAlternative;
    
// Individualist
  if (SomeVariable = Condition)    // I always put the condition alone
   then PerformSomeRoutine;

  if   (SomeVariable = Condition)  // Condition X found
   and (SomeOtherExpression)       // Requirement Y fulfulled
  then begin  // we can do our stuff
    Code;
    MoreCode;
    LotsOfCode;
  end
   else OtherCode; // or we ended up with the alternative

  if Condition 
   then DoFirstAlternative   // The Condition compelled us to do this
   else DoSecondAlternative; // Optionally, we explain the alternative 
  
  // Here I find myself doing many different approaches, 
  // depending on the complexity of the logic, and the number
  // of alternatives, but with multiple nested if's, I tend to indent
  if FirstCondition            
   then DoFirstAlternative     // We are doing 1 because ...
   else if SecondCondition 
    then DoSecondAlternative   // We are doing 2 because ...
    else DoThirdAlternative;   // Otherwise, We are doing 3

  // I might chose this approach if it is a long chain
  if FirstCondition            
   then DoFirstAlternative   // We are doing 1 because ...
  else if SecondCondition 
   then DoSecondAlternative  // We are doing 2 because ...
  else DoThirdAlternative;   // Otherwise, We are doing 3

C++/C# and Java people likes to rant about how ugly and verbose our begin / end's are, and I am sure I could rile myself up over their curly brace enclosures and some of the religion connected to those too - but I am not going to go there. However, there are a few things about our enclosures that may be worth thinking over.

Some like to dangle their end's at various indentation levels. I prefer to align it with the matching start of the block (do begin, then begin). Why? It makes it is easier to identify the code path in relation to the condition.

More enclosures doesn't always mean better readability. Here is an example from Indy9(IdTunnelCommon.pas). Kudzu, I love you - but I disagree with your code style in this file :)
(I know I am going to get flamed for this...^^)

procedure TReceiver.SetData(const Value: string);
var
  CRC16: Word;
begin
  Locker.Enter;
  try
    try
      fsData := Value;
      fiMsgLen := Length(fsData);
      if fiMsgLen > 0 then begin
        Move(fsData[1], (pBuffer + fiPrenosLen)^, fiMsgLen);
        fiPrenosLen := fiPrenosLen + fiMsgLen;
        if (fiPrenosLen >= HeaderLen) then begin
          // copy the header
          Move(pBuffer^, Header, HeaderLen);
          TypeDetected := True;
          // do we have enough data for the entire message
          if Header.MsgLen <= fiPrenosLen then begin
            MsgLen := Header.MsgLen - HeaderLen;
            Move((pBuffer+HeaderLen)^, Msg^, MsgLen);
            // Calculate the crc code
            CRC16 := CRC16Calculator.HashValue(Msg^);
            if CRC16 <> Header.CRC16 then begin
              fCRCFailed := True;
            end
            else begin
              fCRCFailed := False;
            end;
            fbNewMessage := True;
          end
          else begin
            fbNewMessage := False;
          end;
        end
        else begin
          TypeDetected := False;
        end;
      end
      else begin
        fbNewMessage := False;
        TypeDetected := False;
      end;
    except
      raise;
    end;

  finally
    Locker.Leave;
  end;
end;

This little nugget very much bear the signs of being written for debugging, so I am not going to hold it to Kudzu for style (much). It includes a few nice examples of code that can be simplified, so it is all good.

So what do I do? • I assume the except block was for debugging, but I'm taking it out. • I might be changing the behaviour by setting two default values up top, but at least there is no doubt about their default value. • But what is with the conditional assignments? Please! BooleanVariable := Expression; !! Don't go "then true else false" on me! • Reworked the comments.

Personally, I think it is more readable like this.

procedure TReceiver.SetData(const Value: string);
var
  CRC16: Word;
begin
  fbNewMessage := False;
  TypeDetected := False;
  Locker.Enter;
  try
    fsData := Value;
    fiMsgLen := Length(fsData);
    if fiMsgLen > 0
    then begin // Data found, Check for Type
      Move(fsData[1], (pBuffer + fiPrenosLen)^, fiMsgLen);
      fiPrenosLen := fiPrenosLen + fiMsgLen;
      TypeDetected := (fiPrenosLen >= HeaderLen);
      if TypeDetected
      then begin // We have a type, check header for content
        Move(pBuffer^, Header, HeaderLen);
        fbNewMessage := Header.MsgLen <= fiPrenosLen;
        if fbNewMessage
        then begin  // we have enough data for the entire message
          MsgLen := Header.MsgLen - HeaderLen;
          Move((pBuffer+HeaderLen)^, Msg^, MsgLen);
          CRC16 := CRC16Calculator.HashValue(Msg^);  // Calculate the crc code
          fCRCFailed := CRC16 <> Header.CRC16;       // and check if it was correct
        end;
      end;
    end;
  finally
    Locker.Leave;
  end;
end;

Now, where is that flameproof tin foil dress...

Next: I'll be rambling a little more on effective comments, naming and structure.

Edit - added after Paul's Snippet Rewritten: Delphifreak likes to exit early. He also likes to initialize up top, and I totally agree - that is a good practice. Exit works well in this example where all the conditions are dependant on the previous one. IMO, things get a lot more hairy if there are multiple conditions with alternatives (if/then/else if). For this example, Exits are not bad.

procedure TReceiver.SetData(const Value: string);
var
  CRC16: Word;
begin
  fbNewMessage := False;
  TypeDetected := False;
  Locker.Enter;
  try
    fsData := Value;
    fiMsgLen := Length(fsData);
    if fiMsgLen = 0
     then Exit;

    // Data found, Check for Type
    Move(fsData[1], (pBuffer + fiPrenosLen)^, fiMsgLen);
    fiPrenosLen := fiPrenosLen + fiMsgLen;
    TypeDetected := (fiPrenosLen >= HeaderLen);
    if not TypeDetected
     then Exit;

    // We have a type, check header for content
    Move(pBuffer^, Header, HeaderLen);
    fbNewMessage := Header.MsgLen <= fiPrenosLen;
    if not fbNewMessage
     then Exit;

    // we have enough data for the entire message
    MsgLen := Header.MsgLen - HeaderLen;
    Move((pBuffer+HeaderLen)^, Msg^, MsgLen);
    CRC16 := CRC16Calculator.HashValue(Msg^);  // Calculate the crc code
    fCRCFailed := CRC16 <> Header.CRC16;       // and check if it was correct

  finally
    Locker.Leave;
  end;
end;