Delphi Programming

and software in general.

Wednesday, August 6, 2008

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;

12 comments:

  1. Whoah! Thats really ugly.

    Why don't you use the 'common' way for formatting code?

    if (condition) then
    begin
    doSomething();
    doeSomethingElse();
    end;

    if (condidtion) then
    doSomething()
    else
    doSomethingelse();

    ReplyDelete
  2. We've been putting the "then" and "else" keywords under and in-line with the "If" statements for about 12 years now. It tends to work better because it makes the "IF" condition stand out better and leaves room for a comment, just as you point out.

    ReplyDelete
  3. @Sebastian: I was hoping the reasoning for why I write like this was clear from my post.

    I do it for clarity and unambiguity. It took me years of writing code of my own as well as trying to get a grip on the code of others to arrive at this form. It also saves time if you add more conditions, as you don't have to shuffle around the code so much. But, as I wrote - coding style is something personal - in ways similar to religion.

    @SESummers: Phew, I am not all alone after all! :)

    ReplyDelete
  4. "But, as I wrote - coding style is something personal - in ways similar to religion."

    Even in other languages this is my style and I understand it better.

    try
    if condition then
    Code
    else if condition then
    if condition the begin
    if condtion then
    code;
    code;
    end;
    else if condition then begin
    code;
    morecode;
    if condition then
    code
    else if condition then
    code;
    end else begin
    code;
    morecode;
    end;
    finally
    code;
    end;

    ReplyDelete
  5. ignore my style.. that's even more unreadable. indenting is lost when displayed in this page.

    ReplyDelete
  6. My style:

    if n>0 then
    begin
    Tra := ta - ta;
    Tra2 := ta2 - ta2;
    end;

    Good Luck!

    ReplyDelete
  7. @Paul: If you lead in with &nbsp; instead of regular space, the formatting will be kept - but it is torture to do so in the tiny comment dialog. I am thinking about switching the comment layout to popup or full page with preview, to make it easier to check the results.

    ReplyDelete
  8. @Paul: btw, in your example, there is an else with an end; infront of it. I assume that ; was a typo. I'll have a look at it in the next post, so we can get a better impression of your style.

    ReplyDelete
  9. Yes, this is a source of endless debate ;-)
    after reading 'Code complete' years ago I switched to:

    if condition then begin
    line
    line
    end
    else begin
    line
    line
    end;
    continuation

    From a distance this makes it very clear that the indented lines all belong to the if or the else. You don't have to read the text to see that.

    However, you idea of making it
    if condition
    then begin
    to make the condition really stand out migjht be worth a try too...

    Bye
    Jan

    ReplyDelete
  10. Well, the blogging code just complete mangled my example.
    Here it is again with dots for spaces:
    if condition then begin
    ..line
    ..line
    ..end
    else begin
    ..line
    ..line
    ..end;
    continuation

    ReplyDelete
  11. Ok, here I present my coding style.

    Facts:
    A) Most people are used to read top->down.
    B) If you start doing a cake, you first check if you have all items you need.

    The lead me over the year's to the folling style:

    1.) setup method internal variables and return value.
    2.) check input items and leave immediatly with command "exit". This avoid's this ugly "else"-style as shown in the example SetData.
    3.) after step 1 and 2, now the real business logic of the method is implemented.

    This keeps the code most readable and nested-level for if-statements will almost never become deeper than 2 levels.

    For example the cleaned version of Lars, would not have three if-statements nested.

    if fiMsgLen > 0 then begin
    becomes
    if fiMsgLen=0 then exit;

    ReplyDelete
  12. @DelphiFreak: Valid points. I revised the post to include a version using Exits.

    ReplyDelete