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;

Writing reusable code – part 2

There are a few practices that will help you write solid, maintainable, reusable code. A lot of software practitioners of greater fame than mine have written about these before, but it doesn’t hurt to repeat good advice.

Note that I in no way claim this to be the one and only true way to software development nirvana, but just some practices that work for me. Your own opinions and experiences on the topic will be greatly appreciated, so feel free to comment.

Keep It Simple, Stupid
Reusable code should be kept low-feature. It can be a challenge to avoid feature creep and dependency bloat, so make an effort to follow the KISS rule. The more sophisticated stuff you add, the more you will need to “fix” and not to forget: Test! – when inheriting from the class.

Design for extendability
When you design a reusable class, you really have to define the scope of what you want to achieve. Decide on the basic feature set, then evaluate the possible offspring to this class and conceivable additional features, but to adhere to the KISS rule – you should not add the features until you need them. Instead, spend a little time on evaluating the impact of the features and put in any necessary hooks and leave enough comments to allow you to rediscover why you put them in in the first place.

Later, if you absolutely have to add a feature and cannot add it in an inherited class – make sure you don’t break existing behavior in class instancing. A common mistake is to change a default property value in the base class constructor, which some derived class assumed had a specific state or value.

…and on the point of assumptions…

Never Assume, Always Assert
I bet you never heard that one before! 🙂 Use Asserts to check your assumptions. It will save you from a number of embarrassing moments of broken code. The purists probably want this done in unit testing, but it can help add clarity to your code if it is done in the actual library code. Think of it as design by contract.

procedure TSimpleClass.ActOn(aParameter:TSomeClass; aValue:Integer);
begin
// Ensure aParameter is assigned and of correct type
Assert(Assigned(aParameter) and (aParameter is TSomeClass));
// and that aValue is in legal range
Assert(aValue >= 0);
...

Using asserts like these will help you catch runtime problems faster. They also will remind you of what conditioning you need to do before you call the method, and what assumptions you can make within the routine itself.

Avoid Dependencies
When you start pulling in code from other libraries or classes, you should consider the level of dependency that you introduce. A simple extra nice-to-have attribute may come with a payload of extra code that hardly ever will be called. If possible, put the extra attribute in a separate descendant class in a separate unit. The less code you have to compile in, the better.

Embrace Polymorphism
When you create classes that are intended as foundation for descendant classes, you should consider how to best support polymorphic instancing.

For example – if you are writing a container class that will hold a polymorph collection of elements – you may want to have a virtual class function that return the default class type used to instance new elements in the container, instead of just instancing the base element class. Doing so will allow descendant classes make reliable assumptions about the safety of typecasting within the extended methods in the descendant container class.

unit BasicClass;

interface

type
TElementClass = class of TBaseElement;

TBaseElement = class(TObject)
end;

TBaseContainer = class(TObject)
class function ElementClass:TElementClass; virtual;
function ValidElement(anElement:TBaseElement):Boolean;
end;

implementation

{ TBaseContainer }

class function TBaseContainer.ElementClass: TElementClass;
begin
Result := TBaseElement;
end;

function TBaseContainer.ValidElement(anElement: TBaseElement): Boolean;
begin
Result := anElement is ElementClass;
end;

end.

“But…” – you will say; “you can just override methods in the descendant class for the same effect?”. Yes, you can and it works well when you add new functionality, but what about functionality that already has been implemented in class higher in your inheritance tree?

unit AdvancedClass;

interface
uses
BasicClass;

type
TAdvancedElement = class (TBaseElement)
end;

TAdvancedContainer = class(TBaseContainer)
class function ElementClass:TElementClass; override;
end;

implementation

{ TAdvancedContainer }

class function TAdvancedContainer.ElementClass: TElementClass;
begin
Result := TAdvancedElement;
end;

end.

The base class won’t have an understanding of it’s descendants, so if you want it to be able to work with content from descendant classes, you would either have to override/redo the functionality or – do as above – allow the base class to work with their own child classes as well.

program ExtendableClasses;
{$APPTYPE CONSOLE}
uses
SysUtils,
BasicClass in 'BasicClass.pas',
AdvancedClass in 'AdvancedClass.pas';

procedure Main;
var
Element1, Element2 : TBaseElement;
Container1, Container2 : TBaseContainer;
begin
Container1 := TBaseContainer.Create;
Container2 := TAdvancedContainer.Create;

Element1 := Container1.ElementClass.Create;
Element2 := Container2.ElementClass.Create;

Writeln('E1: ', Element1.ClassName);
Writeln('E2: ', Element2.ClassName);

Writeln('E1 valid in C1: ', Container1.ValidElement(Element1));
Writeln('E2 valid in C1: ', Container1.ValidElement(Element2));

Writeln('E1 valid in C2: ', Container2.ValidElement(Element1));
Writeln('E2 valid in C2: ', Container2.ValidElement(Element2));
end;

begin
try
try
Main;
except
on E:Exception do
Writeln(E.Classname, ': ', E.Message);
end;
finally
Write('Press Enter: ');
Readln;
end;
end.

Result

This particularly goes for dialogs and forms that use a polymorph class – make a virtual class function to enable overriding the class instancing. Avoid the rigidly defined TSomeInheritableClass.Create constructs and call the virtual class type function instead.

Note on constructors
When I finally “got” OOP as opposed to structured programming, it was hard to drop the structured way of thinking when it came to instancing. Most of my classes would only have a constructor that took a ton of parameters to save a few lines at the point of instancing that class.

While this might work well for monomorphic classes, it really will not be good for a class hierarchy, and it took me some time to get to the habit of having a parameterless Create. Today, I almost wish for a compiler hint if I should happen to write constructors that require parameters.

Hide detail with Layers
Think in APIs and layers and try to propagate only content between the layers, and not details that are specific to connectivity, storage, etc. A good layered design will allow you to rip out and replace parts of the architecture without massive rewrites.

When you design your APIs or layers, you should consider leaving space or hooks for piggybacking in more content in yet-to-be defined formats where appropriate.

Isolate Data from the GUI
The less your GUI know about your data, the better. Vise versa, the less your data knows about the GUI, the better. Use a mediating class that does the job of knowing both and how to tie them together. This will ease the job of making the data structures as well as the GUI reusable.

It can be particularly useful to tie your business logic to your data and avoid implementing biz.logic in the GUI presentation. Your data will be easier to move to a different presentation form when the rules are not hard-coded in some draw routine. You don’t want to have duplication of biz.logic code between the display routines, the report routines, the export routines, etc.

Thats it for now. More on the topic of Layers and GUI abstraction to follow.
Also, part 3 of the reusable grid.

Asking the right questions: What if…

To me, software development is a continuous search for answers…

  • What is the problem?
  • Is that really the problem?
  • What is the preferred solution?
  • How should it work?
  • How must it work?
  • How do we make that happen?
  • How should we break down the tasks?
  • How long will it take?
  • What will it cost?
  • Is it worth it?
  • Is there a plan B?

When you finally get down to the core programming task, again it boils down to asking the same question over and over:

What if … ?

There are no stupid questions.