Delphi Programming

and software in general.

Thursday, August 7, 2008

Writing Readable Code - Paul's Snippet Rewritten

Five brave people have contributed rewritten versions of Paul's code. Let's take a look at their approach.

I reformatted Paul's example using two spaces for every indent level (after then, begin, else, etc. ), hoping that this is the way he originally wrote it.

IMO, Paul's formatting (or my assumptions about his formatting) does not reflect the true code path, and trying to decipher the code paths become unnecessarily hard.
  try
if ConditionA then
Code(1)
else if ConditionB then
if ConditionC then begin
if ConditionD then
code(2);
code(3);
end
else if ConditionE then begin
code(4);
morecode('X');
if ConditionF then
code(5)
else if ConditionG then
code(6);
end else begin
code(7);
morecode('Y');
end;
finally
code(8);
end;
Here is a small exercize. If we say that the string "ABCDEFG" are all conditions TRUE, and the string "abcdefg" are all conditions false:
• What codes are run by "ABCDEFG"?
• What codes are run by "aBcdefg"?
• What string(s) would make code(6) run?

TS contributed a very nicely formatted version, which is effective in guiding us through the potential code paths. I like his style.
  try
if ConditionA then
Code(1)
else if ConditionB then
if ConditionC then
begin
if ConditionD then
code(2);
code(3);
end // without the semi-colon
else if ConditionE then
begin
code(4);
morecode('X');
if ConditionF then
code(5)
else if ConditionG then
code(6);
end
else
begin
code(7);
morecode('Y');
end;
finally
code(8);
end;

SS didn't quite manage to reflect the flow in the code and his formatting is similar to Paul's code.
  try
if ConditionA then
Code(1)
else if ConditionB then
if ConditionC then
begin
if ConditionD then
code(2);
code(3);
end //; I assume this semi-colon has to go
else if ConditionE then
begin
code(4);
morecode('X');
if ConditionF then
code(5)
else if ConditionG then
code(6);
end
else
begin
code(7);
morecode('Y');
end;
finally
code(8);
end;

Jolyon scores high on restructuring and simplifying, but he made one mistake in his change. Under which condition will his code behave differently form the original?
  procedure WhenC;
begin
if ConditionD then
code(2);
code(3);
end;

procedure WhenE;
begin
code(4);
morecode('X');
if ConditionF then
code(5)
else if ConditionG then
code(6);
end;

begin
try
if ConditionA then
Code(1)
else if NOT ConditionB then
EXIT;

if ConditionC then
WhenC
else if ConditionE then
WhenE
else
begin
code(7);
morecode('Y');
end;
finally
code(8);
end;

AO goes even further than Jolyon in reformatting and comments: As I believe that proper formatting is not the only way to increase readability of code, I also refactored it a bit to reduce nesting where possible. If this were an actual code, I would have probably gone even further and introduced separate routines for the individual blocks, depending on their complexity.
  try
{ This comment explains why ConditionA is handled first. }
if ConditionA then begin
Code(1);
Exit;
end;

{ This comment explains why the aggregate of ConditionB and ConditionC is handled next. }
if ConditionB and ConditionC then begin
{ This comment explains why ConditionD is relevant only in this block. }
if ConditionD then
code(2);

code(3);
Exit;
end;

{ This comment explains why ConditionE is handled next. }
if ConditionE then begin
code(4);
morecode('X');

{ This comment explains why ConditionF and ConditionG are relevant only in this block. }
if ConditionF then
code(5)
else if ConditionG then
code(6);

Exit;
end;

{ This comment explains the default handling. }
code(7);
morecode('Y');
finally
{ This comment explains why the following code must execute no matter what. }
code(8);
end;

This is readable, but personally I think AO went a bit too far. Like Jolyon, he also missed a structural detail in the refactoring and broke the code. Which condition state(s) will cause the code to misbehave?

I am divided on the use of Exit. It can add clarity, but it can also be a big problem as you leave a lot of code "dangling". If you decide to move the exit - you have to be really careful to ensure that any states that suddenly move in or out of scope behave correctly. If there is a nesting church and a chunking church, I'm probably a "Nestorian".

I do agree that refactoring is a valuable tool to clarify and simplify, and we should make an effort to break down our code into manageable blocks, but in this particular case it probably isn't necessary.

Another thing: I avoid using {curly braces} for in-code commentary and use // instead. Why? Because if you need to comment out code containing select count(*) from table, those curlies will work where the (* comment *) fail. Should CodeGear add support for comment nesting? I don't know...

Anyways...
Here's how I would format the example. This is very similar to TS's example, except that I showel all the reserved words to the left side to leave the logic more visible and commentable, but I also add indentation to the innermost conditional code.
  try
if ConditionA
then Code(1)
else if ConditionB
then if ConditionC
then begin
if ConditionD
then code(2);
code(3);
end
else if ConditionE
then begin
code(4);
morecode('X');
if ConditionF
then code(5)
else if ConditionG
then code(6);
end
else begin
code(7);
morecode('Y');
end;
finally
code(8);
end;

MJ adds a contribution with the following comment: "To be sure not to create any future bugs you should consider adding a begin end section after every if statement even if it is not required, but that will make the code more unreadable."
  try
if ConditionA then
Code(1)
else if ConditionB then
begin
if ConditionC then
begin
if ConditionD then
code(2);
code(3);
end
else if ConditionE then
begin
code(4);
morecode('X');
if ConditionF then
code(5)
else if ConditionG then
code(6);
end
else
begin
code(7);
morecode('Y');
end;
end;
finally
code(8);
end;

Good Points! In all honesty, I also screwed up the code blocks on first try. Conditional code will bite you if you are not very very careful. You should indeed think about what may happen if you need to add more code and/or conditions. Personally, I don't think a few more enclosures makes the code less readable. Here is how I would be more explicit in the use of enclosures to make the code less ambiguous.
  try
if ConditionA
then Code(1)
else begin
if ConditionB
then begin
if ConditionC
then begin
if ConditionD
then code(2);
code(3);
end
else begin
if ConditionE
then begin
code(4);
morecode('X');
if ConditionF
then code(5)
else if ConditionG
then code(6);
end
else begin
code(7);
morecode('Y');
end;
end;
end;
end;
finally
code(8);
end;


There is no one true correct way of formatting.
The point I am trying to make is that code structure matter for understanding the code at first glance, and we should be mindful about how we lay it out. We should strive for consistency, but always keep clarity and unambiguity as priority one. Bend your rules, if you need to.

Paul, Thank you for creating such a devious code snippet!

P.S. If you haven't figured out the answers yet, load up your Delphi and run all the examples in StructureDemo.dpr.
No peeking until you have some suggestions! :)
(Hint: else starts a new block)

Edit: Added an example of using Exits instead of nesting to the initial post on formatting. It would be interesting to see DelphiFreak have a go at Paul's snippet.

4 comments:

  1. Grrr. How embarassing. That'll teach me to check in code without unit testing. :)

    And ironically, the fix is minor, eliminates the EXIT and looks if anything even nicer than my first go:

    procedure WhenC;
    begin
    with Condition
    do begin
    if Condition['D'] then
    code(2);
    code(3);
    end;
    end;

    procedure WhenE;
    begin
    with Condition
    do begin
    code(4);
    morecode('X');
    if Condition['F'] then
    code(5)
    else if Condition['G'] then
    code(6);
    end;
    end;

    begin
    try
    if Condition['A'] then
    Code(1)
    else if Condition['B'] then
    begin
    if Condition['C'] then
    WhenC
    else if Condition['E'] then
    WhenE
    else
    begin
    code(7);
    morecode('Y');
    end;
    end;
    finally
    code(8);
    end;
    end;


    An interesting topic for a post. I feel inspiration bubbling away as I type.... :)

    ReplyDelete
  2. @Jolyon: If it is any consolation, I made the same mistake. Multiple if/then/else if is dangerous territory.

    The irony is that we keep repeating the same silly mistakes, since we tend to be so casual about something as "trivial" as the matter of formatting.

    Our brain is highly efficient at spotting visual patterns, but if we format badly, we obscure the pattern.

    ReplyDelete
  3. hi all,

    only now that i read all the post here related to my comment as i just came back from my non-programming job. never thought that my simple comment would become the subject of different opinions regarding their coding style. actually, the code snippet that appears above is exactly my coding style and i find it more readable.

    i understand the point of other posters as well as the author of this blog/article so no further comment just to argue and try to depend my coding style. even if i have to follow the most acceptable coding style and the one who will maintain my code are used to with their own then it will end up them modifying my source to suite their own style. for now it is my preference as i understand and read my code better even the most complex code structure i had written 5 years ago. only few code comments, there's even no documentation.

    i would be more concerned and willing to be educated with optimizing my code for execution. perhaps a coding style for execution performance would be a good subject to take in a separate thread.

    thanks to all. i can now go back to my work as a multitask individual doing administrative/clerical job in our company.

    ReplyDelete
  4. Hi Paul! Thanks for not holding a grudge to me for riling about your style :)

    On the point of optimization, I am thinking about touching on that in a future post. There are some good practices that can be applied, but generally speaking - compilers these days are very clever when it comes to optimization. Hence, we probably get more value back from making sure that our code is easy to understand and maintain.

    Anyways, there are configurable reformatting and cross-referencing tools out there, so if we don't like the style in a particular file - we can always get it the way we like with the click of a button.

    P.S. I am not always too good at following my own advice on commenting. There isn't exactly an excessive amount of comments in the StructureDemo.dpr file *blush*.

    ReplyDelete