Coding Conventions

Table of Contents

1. Pascal Coding Conventions

In general, we follow the standard Lazarus and Delphi coding conventions, used throughout most modern Object Pascal code.

These are documented nicely on:

Some particular conventions:

1.1. Indent by 2 spaces

procedure Foo;
var
  X: Integer;
begin
  X := 1;
  if Y > 4 then
  begin
    while Z = 'hello' do
    begin
      Bar;
      Xyz;
    end'
    WriteLn(X);
  end;
end;

1.2. Do not use tabs or trailing whitespace

Do not leave "trailing whitespace" at the end of lines.

Reason: In the long run, it causes unnecessary diffs when someone removes this whitespace, which many text editors (and some humans :) ) do automatically.

Do not use tabs. Use only spaces for all indentation.

Reason: Tabs are nice when they are used 100% consistently for indentation, but in the long run it’s hard to enforce using them 100% consistently esp. when many people are working on the same codebase. And when tabs are used inconsistently (sometimes mixed with spaces for indentation), problems arise — because people have diffent tab width in their editors, so for some people indentation will look broken. It’s much easier to enforce the rule always use spaces.

1.3. Use PascalCase for everything

PascalCase means that the first character of each word is capatalized.

  • Including constants. So write MyConstant instead of e.g. MY_CONSTANT.

  • Including local variables. Even 1-letter variable names (so write I instead of i).

  • Including type names. Even the type names that are Pascal keywords (so write String / Boolean instead of string / boolean). Note: this rule was changed during CGE 6.5 development. So you will find a lot of code using lowercase string now in engine sources, but new code should use String.

1.4. true and false are lowercase, not True and False

Reason: We just historically wrote them always as lowercase true and false, and now it would be too big effort (for too little gain) to change to True and False. Some Pascal codebases write them also lowercase (e.g. LCL, although it is not 100% consistent) so we’re not alone in this decision.

1.5. Put begin on a new line, indentation of begin and end should match

Do not mimic C "K & R" style (https://en.wikipedia.org/wiki/Indent_style#K.26R) in Pascal:

// DON'T WRITE THIS:
for I := 1 to 10 do begin
  Writeln(I);
end;

Instead, the "begin" should usually be indented the same as "end".

// THIS IS OK:
for I := 1 to 10 do
begin
  Writeln(I);
end;

To look simpler, it’s OK to omit begin/end when they would surround only 1 statement:

// THIS IS EVEN BETTER:
for I := 1 to 10 do
  Writeln(I);

1.6. Put else on a new line, unless it’s right after end

The "else" keyword is written on a new line, unless it’s right after "end". So:

// THIS IS OK:
if Foo then
  Bar
else
  Xyz;

// THIS IS ALSO OK:
if Foo then
begin
  Bar
end else
begin
  Xyz;
end;

// THIS IS ALSO OK:
if Foo then
begin
  Bar
end else
  Xyz;

// THIS IS ACCEPTABLE, BUT BETTER AVOID IT:
if Foo then
  Bar
else
begin
  Xyz;
end;

// THIS IS NOT OK:
if Foo then
begin
  Bar
end
else
begin
  Xyz;
end;

// THIS IS NOT OK, BUT IS USED IN A LOT OF CODE:
// (Michalis was using this convention for a long time,
// until it was pointed to him that it doesn't look optimal,
// and Michalis agreed :)
// Do not use this in new code, but don't be surprised if it still occurs somewhere.
// Michalis will gradually get rid of it in CGE sources.)
if Foo then
  Bar else
  Xyz;

1.7. Put if on a new line, even if it’s right after end

// THIS IS OK:
if Foo then
  Bar
else
if SomethingElse then
  Xyz;

// THIS IS ALSO OK:
if Foo then
begin
  Bar
end else
if SomethingElse then
begin
  Xyz;
end;

// THIS IS ALSO OK:
if Foo then
begin
  Bar
end else
if SomethingElse then
  Xyz;

1.8. Do not use with

Never use "with" keyword. Using "with" makes the code very difficult to read, as some of the symbols inside the "with A do begin …​. end" clause are bound to A, and some are not, and it’s completely invisible to the human reader which symbols are which.

And it’s impossible to determine it, without intimately knowing the complete API of class/record A.

E.g. what does this code do?

with A do
begin
  SourceX := X;
  SourceY := Y;
end;

Does it modify A contents, or does it modify outside variables, merely reading the A contents? You really don’t know, until I show you the documentation of the class of A, and all it’s ancestors.

Compare with a clear:

SourceX := A.X;
SourceY := A.Y;

or

A.SourceX := X;
A.SourceY := Y;

The "with" also makes the code very fragile to any changes of A API. Every time you add a new field/property/method to A, then the code inside "with A do begin …​. end" may change it’s meaning. It may compile, but suddenly will do something completely different.

Likewise, every time you remove a field/property/method from A, the code inside "with A do begin …​. end" may compile, if you happen to have a variable outside of this block with a name matching the name inside A.

1.9. Order of the uses clause: standard, CGE, application

The uses clause of our units and examples should follow the order

  • standard units (RTL, LCL, VCL…​)

  • then our own (CastleXxx) units

  • then eventual game-specific units (GameXxx)

Each part should start from a newline.

// THIS IS OK:
uses SysUtils, Classes,
  CastleUtils, CastleViewport,
  GameStatePlay;
Note
We don’t have a strict rule for the order of units within each group. They often loosely follow the "dependency order" (if unit A is used by B, then we write A, B, like SysUtils, Classes, Controls) but we really don’t make this a strict rule (it would be too tiresome to maintain, for no gain). Many uses clauses have in practice an order dictated by history and that’s OK.

1.10. Use strict private where possible

Use strict private whenever you can, that is: use it for private stuff that is not accessed by other classes/routines in the same unit.

Use just private for stuff that is accessed by other classes/routines in the same unit. Basically, use private where you would use friends in C++.

This improves code readability in case of large units, that feature more than just a single class. And we have many such large units.

1.11. Do not use strict protected

Using strict protected is not advised in CGE.

Reason: The distinction between strict protected and protected is not very useful for readability (regardless if something is strict protected or just protected, you must think "something outside of this class, even outside of this unit, may access it"). Moreover, it is forced downward, on all descendants of this class (that must then differentiate between overriding in strict protected vs protected, which is uncomfortable because the decision whether to use strict protected or protected should be an internal (implementation) decision within the ancestor, not affecting the descendants).

So, use just one protected section, do not bother splitting it into strict protected and protected.

1.12. Write abbreviations using PascalCase

Use Url, Http etc. in Pascal identifiers. Because it looks much better in long identifiers, e.g. GetHttpResponse is much more readable than GetHTTPResponse.

Note that in the comments, you should still use English convention of writing the whole abbreviation uppercase (not only the first letter). So e.g. documentation for GetHttpResponse would be Returns a response received over HTTP.

Note

In the special case of Url, we have additional complications:

  • For historic reasons, a lot of code used uppercase URL.

  • In some places we use the term URI instead of URL. The difference betwen URL and URI is explained in terminology section of the networking manual (URI is a more general concept than URL) but admittedly the difference is small and we didn’t pay strict attention to this difference. In effect, in some places we used the term URL where we should have used URI or vice-versa. X3D standard also mixes them a bit, in efffect TInlineNode.SetUrl actually accepts various URIs (including data URI scheme).

To make things simple, all future code should just use Url, and our documentation about URLs just clearly says "most URLs actually accept a URI (a bit broader term than URL); in particular you can use data URI scheme".

1.13. Indenting inside classes

type
  TMyClass = class
  private
    MyField: Integer;
    procedure Foo;
  public
    MyPublicField: Integer;
    procedure Bar;
  end;

If you use the nested types / constants, indent the fields inside the var block as well. See the example below, notice that MyField is now indented more than in the example above. Admittedly it’s not a perfect convention — MyField indentation is now inconsistent with MyPublicField. But on the other hand, MyField indentation is consistent with MyNestedConst and TMyNestedClass and how you usually indent var block.

type
  TMyClass = class
  private
    type
      TMyNestedClass = class
      end;
    const
      MyNestedConst = 123;
    var
      MyField: Integer;
    procedure Foo;
  public
    MyPublicField: Integer;
    procedure Bar;
  end;

1.14. File extensions

  • *.pas files are units,

  • *.inc are files to be included in other Pascal source files using $I (short for $Include).

  • *.dpr and *.lpr are Pascal program files. We advise using .dpr extension, unless it’s an application that will really only ever work with Lazarus (and not Delphi) e.g. because it relies on LCL.

    Reason: Lazarus accepts either .dpr or .lpr as extensions for the main program file. But Delphi tolerates only .dpr extension. And almost all CGE programs compile (or will compile) with both Delphi and Lazarus. So, like it or not, we have adjusted to Delphi, and just use .dpr.

1.15. Write reentrant routines

All the engine functions are "reentrant", which means that they are safe to be called recursively, even through your own callbacks. E.g. the TFileProc callback passed to FindFiles can call FindFiles again inside it’s own implementation, causing a recursion.

1.16. Some naming conventions

  • If some procedure modifies it’s 1st parameter then I usually end it’s name with "Var" ("to variable").

    Often you will be able to see the same operation coming in two flavors:

    function DoSomething(const X: SOME-TYPE, ...): SOME-TYPE;
    procedure DoSomethingVar(var X: SOME-TYPE,...);

    The 1st (functional-like) version is more flexible, but the 2nd version may be faster (especially if SOME-TYPE is large, or requires time-consuming initialization).

    See e.g. CastleVectors and CastleImages units.

    This rule doesn’t apply when SOME-TYPE is some class instance. It’s normal that a procedure may modify the given class instance contents, no need to signify this with a "Var" suffix.

  • The term "stride" refers to a distance in bytes between memory chunks, following OpenGL conventions.

    If somewhere I use parameters like V: ^SOME-TYPE and Stride: Integer then it means that these parameters define a table of SOME-TYPE values. Address of 1st item is V, address of i-th is (V + i * Stride).

    Stride may be negative. Stride may also be 0, then it means that Stride = SizeOf(SOME-TYPE).

  • Do not use Minimal or Maximal. Use Min and Max.

1.17. Compilation symbols

We use standard FPC and Delphi compilation symbols like:

  • MSWINDOWS (do not use WINDOWS which isn’t defined by all compilers),

  • LINUX,

  • UNIX (remember this is broader than Linux, it includes also FreeBSD, macOS, Android, iOS…​ pretty much everything except Windows),

  • CPUI386,

  • CPUX86_64,

  • FPC

to differentiate between compiler versions, and some more.

We detect Delphi by just lack of FPC, e.g. property Foo: Integer …​; {$ifdef FPC}deprecated 'Delphi does not support deprecated clause for properties';{$endif}.

See castleconf.inc (included in every CGE unit).

We also use DEBUG symbol. The build tool, when compiling in debug mode (--mode=debug), defines the DEBUG symbol, and adds some runtime checks, like range checking and overflow checking. You can use {$ifdef DEBUG} in your own code to add additional things. There’s also the RELEASE symbol, but usually we don’t check for it’s existence — if DEBUG then we’re in debug mode, else we’re in release mode.

1.18. Exceptions' messages

This is good:

raise Exception.Create('Invalid parameter');

raise Exception.CreateFmt('Foo must be > 0 but is %d', [
  Foo
]);

raise Exception.CreateFmt('%s:%s: Cannot draw an uninitialized component', [
  Foo.Name,
  Foo.ClassName
]);

raise Exception.CreateFmt('Invalid 3D model file %s', [
  URIDisplay(Url)
]);

raise Exception.CreateFmt('Too many seconds: %f', [
  TimeInSeconds
]);

Guidelines:

  • Do not start the message with 'Error: ' or anything else that just means "we have an error". This is redundant, since all exceptions signal some error.

  • Don’t end the Message with '!' character. Do not cause panic :) The exception message must look normal when presented to the end-user. If something should not occur (and signals a bug) then use EInternalError exception class to mark this.

  • If the exception is related to some object instance, include useful information to identify this instance. If you have a TComponent descendant, then both Name and ClassName contain useful information to show. (see above example)

  • If the exception is related to some file / URL, include this URL using URIDisplay(Url).

  • Usually, Message should be a single sentence, and not end with the '.' character. But we do not follow this rule 100%, it’s OK to break it for good reasons — sometimes a multi-line sentence message is useful. The last sentence in this case should not the dot.

  • Message should not contain any line-breaks. Reason: this doesn’t look good when displayed in some situations. Especially when one Message is embedded as part of the Message of other exception.

    We do not follow this rule 100%, it’s OK to break it with good reasons. We know that some information really looks much cleaner when split into multiple lines (e.g. TMatrix4.ToString output is multi-line already).

  • Message should not contain any general program information like ApplicationName, ExeName, ApplicationProperties.Version etc. (The exception to this rule is when such information is really related to the error that happened, may help to explain this error etc.) The code that finally catches and outputs the exception should make such information available.

1.19. Prefer to make callbacks of object

ObjectPascal is a hybrid OOP language and it has global function pointers and method pointers. They are incompatible, since the method pointer is actually two pointers (the class instance, and the code address). When designing a function that takes a callback, you’re faced with a problem: define "a pointer to a method" or "a pointer to a global function/procedure"?

In the past, I often chose to use "a pointer to a global function/procedure". With a generic "Data: Pointer" parameter, to allow passing user data. This is easier to use when you don’t have a class instance (and you don’t want to create a dummy class just for this), and it’s always allows to add overridden version with "of object" callback (passing object instance as the Data);

Nowadays, I usually define "of object" callbacks, assuming that all non-trivial code is usually in some class, and the "of object" is more natural to be used in OOP.

1.20. Order of methods

Place the implementation of constructors (Create*) first, then destructor (Destroy), and then the rest of methods. I do not have a precise rule about the ordering of the rest of methods — I usually like to group related methods together.

1.21. Remember that StrToFloat and friends are locale-dependent. Almost always use StrToFloatDot instead.

Standard StrToFloat in FPC and Delphi converts floats to/from Strings using locale-dependent DecimalSeparator value. On some systems (e.g. on Polish Windows) it is equal to comma (,), not a dot (.). This is usually not what you want: when you read/write files, or command-line arguments, you usually want to have "dot" as the only decimal separator, so that your application works regardless of user’s system locale.

So instead use StrToFloatDot. As a bonus, it is also marginally faster.

Same advise applies for related functions:

  • Use StrToFloatDefDot instead of StrToFloatDef

  • Use TryStrToFloatDot instead of TryStrToFloat

  • Use FormatDot instead of Format

  • Use FloatToStrDot instead of FloatToStr

1.22. Do not use LongInt / LongWord. Use Int32 / UInt32, Int64 / UInt64, Integer / Cardinal.

Embarcadero decided to make things weird: https://docwiki.embarcadero.com/RADStudio/Sydney/en/Simple_Types_(Delphi). The LongInt / LongWord are

  • 32-bit on 32-bit platforms, and on 64-bit Windows.

  • They are 64-bit on 64-bit OSes that are not Windows (like Linux, Android, iOS).

This is

  • Completely weird (why did you make it inconsistent across platforms???).

  • And contrary to older Pascal documentation statements, that suggested that LongInt / LongWord have 32-bit always. It was the Integer / Cardinal that were supposed to be (maybe) system-dependent! (though they remain in practice 32-bit always, in both FPC and Delphi.)

  • Incompatible with FPC.

So just don’t use these types in CGE code.

  • Use Int32 / UInt32 when you want to have integers of guaranteed 32-bit size. The names are consistent with Int64 / UInt64.

  • Use Int64 / UInt64 when you want to have integers of guaranteed 64-bit size. The QWord (FPC name for UInt64) is also good.

  • Use Integer / Cardinal when you don’t care much about the bit size. In practice they are always 32-bit on all platforms (with both FPC / Lazarus), although long time ago they were supposed to be platform-dependent.

  • Use TListSize for counts and capacities of lists. (It is signed, to not cause overflows with frequent constructions like for I := 0 to List.Count - 1 do...)

  • Use PtrInt / PtrUInt when you want to have integers of guaranteed pointer-size.

1.23. Do not use Extended. Use Single / Double.

Traditionally, Extended used to be a 10-byte floating-point type available in old Pascal compilers. But it is not that useful anymore, in modern FPC and Delphi.

The size and precision of Extended depends now on the platform and compiler:

  • FPC: Extended=Double for most of non-i386 architectures. One known exception to the above is Linux on x86-64, that allows to use normal Extended. Use EXTENDED_EQUALS_DOUBLE to check for it.

  • Delphi: See https://docwiki.embarcadero.com/RADStudio/Sydney/en/Simple_Types_(Delphi) . Similar to FPC, Extended is just Double (8 bytes) on most platforms except Win32.

    Moreover, Delphi defines Extended to be a new 16-byte floating-point type on some platforms:

    • 64-bit Intel Linux

    • 32-bit Intel macOS

    • 32-bit Intel iOS Simulator

TBH, the end result makes Extended not very useful at all, at least for general cross-platform (and cross-compiler) code, due to this uncertainty. And GPUs don’t support anything above Double anyway.

1.24. Most code should use just String, and be prepared that it is 8-bit on FPC and 16-bit on Delphi. Only if writing to stream, explicitly use 8-bit AnsiString (in usual case, when you write UTF-8).

On FPC, we follow the same approach to String as in Lazarus: String is an alias to AnsiString, and it should always contain UTF-8 data. We use necessary compiler switches to make String = AnsiString, and the CastleUtils has necessary initialization to make sure that strings can just carry UTF-8 on all platforms.

See FPC docs:

On Delphi, we follow the standard approach of modern Delphi: String is an alias to UnicodeString, and it contains UTF-16 encoded data.

See Delphi docs:

Correspondingly, Char is 8-bit with FPC, and 16-bit with Delphi. And PChar points to 8-bit characters on FPC, 16-bit on Delphi.

With both compilers, you can explicitly use AnsiString to request 8-bit string. And AnsiChar for 8-bit character, PAnsiChar to have a pointer to them.

What to do?

  • In most CGE code, just use String and Char and most of the time it just does what you want. You can often ignore the fact that FPC will do this using 8-bit chars and Delphi will do this using 16-bit chars.

  • Exceptions:

    • When we read/write to streams, like using various CastleClassUtils routines, we use 8-bit strings. Since UTF-8 is the file format that most software expects, it is a superset of ASCII (that is: simplest text files) etc. So CastleClassUtils routines dealing with streams + strings just declare AnsiString as input/output type.

      There are exceptions marked with DefaultString in the name, right now only MemoryStreamLoadFromDefaultString. This routine writes 8-bit on FPC, and 16-bit on Delphi.

    • When interacting with external libraries, you will most often use PAnsiChar (not PChar) as most of them expect 8-bit UTF-8 (or just ASCII) text.

1.25. Use BeforeDestruction for finalization tasks that keep the object state consistent

While most of the finalization code should usually go to the destructor (Destroy), it is OK (and sometimes even desirable) to override BeforeDestruction and put there code that finalizes things, but keeps the object state consistent. The idea is that finalization can be split into 2 phases:

  1. BeforeDestruction : Do only things that keep the object state consistent. This means that 100% of class implementation is prepared to handle the state in the middle (and after) BeforeDestruction happens. Examples of things to do here:

    • Detach things, e.g. calling BeforeParentDetach and clearing the behaviors from parent TCastleTransform.

    • Calling notifications.

  2. Destroy : Do things that put the object in half-uninitialized state. Not all of class implementation has to be prepared to handle this state.

    • For example, here you can free (always using FreeAndNil) a child object owned by this instance.

    • This is also when TComponent free notification mechanism (provoked by FreeNotification, TFreeNotificationObserver) happens. This happens in TComponent.Destroy (at least in case of FPC).

100% of class implementation must be ready to work during BeforeDestruction. So

  • Do not just blindly move parts of finalization from Destroy to BeforeDestruction. It would be a mistake to fix things this way, as at some point we would then need BeforeBeforeDestruction and so on.

  • Things that can happen from BeforeDestruction can safely still call methods/properties of the class that is being destroyed. This is important, because detaching things often causes some callbacks, some virtual method calls — and in general you cannot control what the code there will do.

As a bonus, implementing BeforeDestruction is sometimes easier than implementing Destroy: Because BeforeDestruction is only called if the class constructor reached the end, i.e. fully initialized everything (see Delphi docs of TObject.BeforeDestruction). This is in contrast to implementation of Destroy, that has to be very careful, because destructor can be called even on half-initialized object state (because destructor is called even when the constructor fails with an exception).

Once the actual Destroy starts, the object may be in half initialized state and in general it is not guaranteed that operating on such object works. The object may have some critical fields cleared (uninitialized), and in general methods/properties may assume they are initialized. E.g.

  • Create does Something := TMySomething.Create

  • Destroy does FreeAndNil(Something)

  • Nothing else can free/change Something instance

  • → This means that most of the implementation may assume that Something is not nil. There is no need to write if Something <> nil then…​ before accessing Something methods/properties.

  • Only a subset of class implementation, that is known it can be reached from Destroy calls (directly or indirectly) has to be prepared to handle Something = nil case.

We often make implementation of some class "robust" to work even when this class is in the middle of Destroy. But there is no guarantee about it and we do it on a case-by-case basis (when it is needed by something). In general, class implementation may assume that it is never called in "half initialized" state, and that user code checks if not (csDestroying in SomeObject.ComponentState) then…​ if it could happen when SomeObject is in the middle of Destroy operation.

1.26. Blank lines

Note
This is a guideline, not a strict rule. There are many cases where we added (or didn’t add) blank lines on a case-by-case basis, because "here it looked better". Still, do follow this guideline if there are no good reasons to deviate from it.

In the interface, put a blank line before a comment in

  • protected,

  • (not advised) strict protected,

  • public,

  • published

sections. Unless that comment is preceded by the line with section keyword (like public).

Putting a blank line before a comment in a private or strict private section is allowed but not required. Often you want to make these private sections more terse (also there is no need to strictly document everything private) so it’s OK to not use blank lines.

Do not add blank lines before the line with section keyword (like public). Do not add blank lines before every undocumented (private) method / property. E.g. this is good:

type
  TMyClass = class
  private
    Foo: Integer;
    Bar: String;
    procedure DoSomething;
    { This is something else. }
    procedure DoSomethingElse;
    procedure SetSuperProperty(const Value: Integer);
  public
    { Incredibly useful operation. }
    procedure DoSomethingIncrediblyUseful;

    { Another incredibly useful operation. }
    procedure AnotherIncrediblyUseful;

    { Super important. }
    property SuperProperty: Integer read FSuperProperty write SetSuperProperty;
  published
    { Published super important property. }
    property PublishedSuperProperty: Integer ...;

    { Another published super important property. }
    property AnotherPublishedSuperProperty: Integer ...;
  end;

This is not good:

// DON'T WRITE LIKE THIS
type
  TMyClass = class
  private

    Foo: Integer;

    Bar: String;

    procedure DoSomething;

    { This is something else. }
    procedure DoSomethingElse;

    procedure SetSuperProperty(const Value: Integer);
  protected

    procedure Something;

    procedure Something;

  public

    { Incredibly useful operation. }
    procedure DoSomethingIncrediblyUseful;
    { Another incredibly useful operation. }
    procedure AnotherIncrediblyUseful;
    { Super important. }
    property SuperProperty: Integer ...;

  { Another published super important property. }
  published property PublishedSuperProperty: Integer ...;
  end;

1.27. Start a property setter with if FXxx <> Value then

A typical setter implementation should check whether the new value is different than the previous, and early abort (not do anything) when they are equal.

Reason: This is an easy and useful optimization. The cost of checking for equality is usually trivial and it can save non-trivial operation cost. Do this even if you suspect (or know) that layer below is also doing such checks. Don’t trust the authors of "layer below" (esp. if it’s an external library).

Implementing setters like this from the start is much easier than latter adding such conditions. If your setter doesn’t make the check if Xxx <> Value then, then later developers will be afraid of adding it, fearing that it breaks something (by not reinitializing something in some cases). So we generally require to implement setters like this from the start.

Our convention is to do it by if FXxx <> Value then followed by the FXxx := Value; assignment (with the FXxx always on the left side, so it’s easy to see that comparison and assignment work on the same identifiers). Like this:

procedure TMyClass.SetFoo(const Value: Integer);
begin
  if FFoo <> Value then
  begin
    FFoo := Value;
    // do some side-effect
    SynchronizedObject.Foo := Value;
    Invalidate;
  end;
end;
Note
Compare vectors like if not TVector3.PerfectlyEquals(FFoo, Value) then…​.

Lazarus CodeTools generate a setter with early exit using Exit instead, we discourage this setter form (not because it’s inherently worse, but just the above is also good and it is prevalent in CGE sources, so better follow existing convention):

// DON'T WRITE LIKE THIS
procedure TMyClass.SetFoo(const Value: Integer);
begin
  if FFoo = Value then
    Exit;

  FFoo := Value;
  // do some side-effect
  SynchronizedObject.Foo := Value;
  Invalidate;
end;

1.28. Document at least every protected / public / published identifier, use a language that looks good in English and uses PasDoc tags

All protected, public, published identifiers should be documented (unless you really don’t have anything to add that isn’t already spelled out by the identifier name).

Documenting the private and strict private identifiers is also encouraged if there’s anything that isn’t obvious. We don’t stricly require documenting them — as their meaning / purpose is often obvious enough from the name, and the details can be inferred by analyzing the implementation code. But if you’re unsure, it is better to write docs.

Do not make a documentation that just repeats what the identifier already says. If you define property RotationAxis: TVector3 …​ then documenting it like this isn’t very helpful:

// DON'T WRITE LIKE THIS
{ The axis of rotation. }
property RotationAxis: TVector3 ...;

If this is really everything you have to say, then you can skip the documentation in such case — there’s no point in writing exactly the same words that the identifier is already composed of.

But there’s almost always something more that you can add that is useful.

  • You can usually summarize connections of the identifier to other identifiers or to the core function of the class.

  • If the property doesn’t contain a default clause (e.g. records, like TVector3, cannot have such clause) then say in the documentation what is the default value (that the constructor sets).

  • You can often mention as @seealso closely related things.

  • If there are any limitations in how something works, describe them.

  • If the property value is ignored in some circumstances, it’s useful to mention it.

This is a better example:

{ Object rotates around the given axis with the speed of @link(RotationSpeed).
  This property is ignored if @link(Rotating) is @false.
  Default value is +Y (0,1,0).
  @seealso RotationSpeed
  @seealso Rotating }
property RotationAxis: TVector3 ...;

As for English:

  • Make it correct English, but also make it terse and precise.

  • Do not waste words on useless prefixes like "This property defines rotation axis." or "This method starts the rotation.". Instead be shorter: "Rotation axis." (for a property) or "Start rotating." (for a method).

  • Especially make the 1st sentence really good.

    • We use PasDoc with --auto-abstract, so the 1st sentence is used in various places as a good summary of this identifier.

    • It also means that the 1st sentence should "stand on its own".

    • Be aware that first sentence is detected by looking for the first dot followed by a space character. Avoid using mid-sentence abbreviations that would confuse this mechanism, like i.e., etc., e.g..

      For example this is not good: Rotation axis, i.e. the axis around which it rotates. Here, PasDoc would accidentally think that 1st sentence is just Rotation axis, i.e..

      To avoid this trap, rephrase the sentence, or don’t use abbreviation (Rotation axis, that is: the axis around which it rotates.). Or use explicit PasDoc @abstract tag.

  • We use PasDoc with --auto-link, so identifiers are converted into links automatically, without the need to use PasDoc @link tag.

    Advantage: the comments look more like regular English.

    Disadvantage: if you make a typo in related identifier name, or the related identifier will be renamed — we will not warn about it.

    When the risk of latter is significant, we often still write @link(SomethingRelated) explicitly, instead of just SomethingRelated. So, there’s no strict rule whether to write or not to write @link(…​) explicitly, follow your heart :)

2. Other languages

  • Indent by 4 spaces in Java and Objective-C.

  • Never use tabs. (Unless they are inherent to the language, like Makefile).

  • Follow the standard coding conventions for that language.

3. Guidelines (how to write good code)

3.1. You can (and should) free the things you allocated; you should never free other things, unless explicitly allowed

Note
See Modern Object Pascal Introduction for Programmers if you’re unsure what some of the terminology used in this section means.

Pascal is a language with manual memory management, i.e. you have to remember to free the things you allocate.

It’s often quite easy to manage this, assuming that other code "plays nice":

  1. If you create something in your constructor, then you are responsible for destroying it in your destructor (or when it needs to be recreated for some reason).

    This works OK and assumes that nothing else will free your instance. Otherwise you will get a dangling pointer, i.e. a non-nil reference that is invalid because something else freed the memory it pointed to.

    Some code may allow others to free the things it created. It can avoid having a dangling pointer e.g. by observing the reference using FreeNotification or TFreeNotificationObserver, see custom components. (Or by not storing the reference at all.) But this permission ("others can free this thing I created") has to be explicitly documented as allowed.

  2. You can also use the "ownership" mechanism:

    • Ownership mechanism on TComponent: owner TComponent will make sure that children TComponents are freed.

      Other things can also free the children, this will be handled OK.

    • Ownership mechanism on TObjectList: list will make sure that children are freed (when they are removed from the list, or when list is destroyed).

      Other things cannot free the children, or you will get dangling pointers. That is, TObjectList cannot detect when an item was freed through some other mechanism, e.g. because you did O := List[123]; FreeAndNil(O);.

    • Ownership mechanism on TComponentList: list will make sure that children are freed (when they are removed from the list, or when list is destroyed).

      Other things can also free the children, this will be handled OK.

In short, things are easy when nothing else frees the object you were supposed to free. Without explicit allowance, the other code should not free the things you created.

This guideline is thus: *When you create an instance of something, then you decide when and how it should be freed. Nothing else should free it, unless explicitly permitted by the documentation.*a

This includes the cases when you receive an instance of something by accessing some property, function or class function. Do not free it, unless it is explicitly documented that you should free it. Only free things where you have called constructor.

This rule has some additional consequences:

3.1.1. If you receive an instance from some code you don’t control, do not assume anything about the lifetime of this instance

The prominent example of this is when you expose a settable property like property FooInstance: TFoo read FFooInstance write SetFooInstance.

The best code should not assume that FooInstance will exist as long as it is set. Developer should be allowed to free it and the class that refers to it should account for it (automatically dropping all references to it, in particular FooInstance should change to nil). This is a consequence of the above guideline: developer can free the things that the developer created!

The solution is to use components (TComponent descendants) with FreeNotification or TFreeNotificationObserver, see custom components.

This allows the developer to do this:

C := TSomeParent.Create(Application);
Foo := TFoo.Create(Application);

C.FooInstance := Foo;
FreeAndNil(Foo)

Assert(C.FooInstance = nil);

3.1.2. Avoid making functions/methods that return new instance of some object and pass the responsibility to free it on the caller

For example this is allowed (if you document it like below) but discouraged:

{ Creates TFoo.
  The caller is responsible for freeing the resulting TFoo instance. }
function MakeSomeInstance: TFoo;
begin
  Result := TFoo.Create;
end;

Instead of creating such methods, it is better to enable the same functionality by advising to use a constructor of the proper type. In the above example, the recommendation should be "use the TFoo.Create explicitly instead of relying on a wrapper MakeSomeInstance ". This makes things more obvious: when developer does F := TFoo.Create, then developer knows (s)he should take care of freeing F.

Of course, this recommendation is harder to achieve in real-life cases that look more complicated than MakeSomeInstance :) You probably had some reason to introduce such function. Maybe the new TFoo instance must be configured in some special way, maybe it needs some special parameter coming from the internal variable of this unit or private field of containing class. The point is to think about remaking it. Maybe it should rather be a new TFoo constructor that takes special parameter, and it can convey the same functionality?

The idea is that, by default, developer should be able to assume that when (s)he does F := MakeSomeInstance, (s)he doesn’t need to care to free F. Because the developer didn’t call constructor explicitly.

If your function returns something that the caller should free, it must be explicitly documented.

3.2. What can be nil

In Pascal, a value of an object instance (like X: TMyObject) can always be nil.

This guideline limits the cases when you have to worry that something is nil, in order to avoid writing lots of checks X <> nil in the code.

Follow this:

  • All the object instances given as routine parameters must be non-nil. (That is, they cannot be nil.) If a routine allows some particular parameter as nil, it should be explicitly documented as allowed.

    This puts the burden on the caller, while the implementation of a routine can assume they are not nil.

    This means that writing code like this is considered OK (there’s no need to check Person <> nil) and you don’t even need to document explicitly "Person cannot be nil!":

    procedure GreetPerson(const Person: TPerson);
    begin
      Writeln('Hello ' + Person.FirstName + ' ' + Person.LastName);
    end;
  • All the functions that return an object instance always return a non-nil value. If a particular function can return nil, it should be explicitly documented as possible.

    This puts the burden on the function implementation, not on the caller.

    This means that writing code like this is considered OK (there’s no need to check GetCurrentUser(Context) <> nil):

    procedure GreetCurrentUser;
    begin
      Writeln('Hello ' + GetCurrentUser(Context).FirstName + ' ' + GetCurrentUser(Context).LastName);
    end;
  • All the properties (of an object instance type) have non-nil value (after the object is created). If a property may be nil, it should be explicitly documented.

    This means that writing code like this is considered OK (there’s no need to check GetCurrentUser <> nil). And you don’t need to care whether GetCurrentUser is a property or a parameter-less method, since the last 2 rules are consistent:

    procedure GreetCurrentUser;
    begin
      Writeln('Hello ' + SomeObject.GetCurrentUser.FirstName + ' ' + SomeObject.GetCurrentUser.LastName);
    end;

3.3. Declarative API (classes with independent properties) is simple to use

In general prefer declarative API (properties) over imperative (methods, esp. with complicated usage scenarios).

The classes that expose the "solution" as a set of properties are simple to use. Exposing a "solution" as a set of methods (that must be called in some specific order for the desired effect) is usually not as simple. Of course this is just a general guideline, I’m sure you know lots of exceptions to this rule! CGE itself is a big OOP library with lots of classes with lots of properties and lots of methods. If something is naturally an "action" ("draw it now!") then it should be a method (Render). But if something is a "state" ("use this color when drawing") then it is a property (property Color: TCastleColor; property Text: String; works better than procedure Draw(const Color: TCastleColor; const AText: String)).

Properties should work independently. Property value should not be "automatically" set by setting another unrelated property (e.g. setting TCastleUserInterface.HeightFraction does not set also TCastleUserInterface.Height) or by doing something (e.g. adding a control does not set its TCastleUserInterface.Height; it represents "the desired height", and programmer should instead read EffectiveHeight to know the resulting height). Of course there are exceptions to the latter — some methods naturally set some properties, but then it should be clear that given method does this, e.g. TCastleViewport.Setup2D sets Viewport.Camera.ProjectionType.

You want getting/setting properties to work naturally, regardless of the order in which it happens (this is nice for the programmer using the API, and necessary for reliable deserialization).

Properties should generally work like you would expect a variable works. E.g. reading a property right after setting it should result in the same value. Setting property multiple times to the same value should have no effect. See the Pascal guidelines on Properties (modern Pascal introduction) : …​it’s a good convention to design properties to behave more-or-less like fields:…​.

Classes with independent properties are simple to use — both from CGE editor (that exposes any published properties of TComponent descendants), and from code (code that sets a few properties is obvious to follow).

3.4. Optimize smartly: profile, optimize where it matters (and not where it doesn’t), think about smarter algorithms and moving CPU work to GPU to get big benefits

If you want to suggest some optimization (of speed, of memory usage) to the engine, especially if it:

  • makes a significant code complication to the existing code,

  • or it adds a significant amount of new code (which is also a code complication)

→ then always first do some tests/thinking whether it’s really worth it.

There are many situations where optimizing is not a good idea, because it will not change the "bottleneck" code (which means that the speed / memory use of something else is so large (in comparison) that it completely "masks" the thing that you optimize, making it irrelevant). In such cases, optimization is actually harmful, because the code quality goes down — the optimized code is usually longer and/or more convoluted.

Note
In the rare cases when the optimized code is also shorter and cleaner, you have a full green light to do it just because the code quality is better.

Bottom line:

  • We want to have less code.

  • We want to have simpler code.

  • Do not optimize just because you have an idea how to make some line of code faster. This thinking often leads to performing many tiny optimizations (and thus reducing code quality) that have no noticeable effect on the execution speed or memory use of real applications. First test/think whether it’s worthwhile to optimize this piece of code.

As you can see, I put more emphasis on thinking about code quality than optimization. That is because I see some of us often making the mistake of not caring about code quality enough, and instead rushing to make an optimization (that lowers code quality for little-to-no gain to the final applications).

Of course, this does not mean we don’t want to optimize. It just means that we require justification for each optimization, the optimization must have a noticeable effect on some real-world use-case. We want the code to be fast and use little memory — there are various ways to achieve this, often using smart algorithms on CPU, and/or thinking about how the CPU cache is used, and/or delivering data in better chunks to GPU. Low-level optimization of some local routine is often not the most effective approach.

There is also a dreaded "death by 1000 cuts" that we want to avoid, which is sometimes caused by missing a number of small optimizations that would have a noticeable effect overall. E.g. that’s why we use "Single" throughout the engine code, not Double or Extended. (except some special code where we have testcases that "Single" precision is not enough). Using "Double" everywhere would have a noticeable negative effect on the speed (yes, I tested it long time ago). But e.g. paranoidally avoiding calling Sqrt in the engine…​ proved to be usually a useless optimization, causing various bugs and not achieving any speed gain.

So, there are cases to be made for some low-level optimizations. But don’t fall into the trap of implementing lots of useless low-level optimizations blindly.

3.5. Fix warnings (let the compiler help you write reliable code)

We want the compilation of the engine, and all examples, to pass without any warnings. On all supported platforms and with all supported compilers.

We consider compiler warnings as generally useful, i.e. we don’t want to hide them, we want to obey them. A warning is a TODO — you need to deal with this, maybe by improving the code to be more reliable.

Sometimes the warning isn’t really valid, but you still should deal with it. It is acceptable, in justified cases, to selectively turn off the warning, like this:

{$warnings off} // knowingly calling deprecated method in another deprecated method
X := Foo;
{$warnings on}

We still consider this a "win": while you will occasionally have to deal with useless warnings from the compiler, but on the other hand you will pay attention to compiler warnings and the compiler will help you detect real problems in code. The warnings reported by the compiler are often about "edge-cases", situations that don’t happen often (or maybe even you think they will never happen), but sometimes the compiler is right: that special situation can happen, and it is better to have reliable answer for it then let code do something undefined.

3.6. Be consistent in API design

Ideally, we add new features to the engine by just "naturally" extending the existing API. By "naturally" I mean that the new concept (class, method, property, whatever)

  • uses similar terminology as existing API concepts (e.g. word "scene" in CGE context refers to TCastleScene, not something else),

  • uses similar conventions (e.g. "Y goes up, even in 2D"),

  • uses existing CGE constructs (e.g. for vectors, use CGE vectors from CastleVectors, like TVector3).

The engine API should be consistent (with itself).

Example: If you ever need 4x3 matrix, make it consistent with existing TMatrix4. Do not declare it like this:

// DON'T DO THIS, THIS MATRIX TYPE IS INCONSISTENT WITH CGE
TNewMatrix4x3 = class
  MyData: array [1..4*3] of Float;
end;

Can you spot all the inconsistent things in this TNewMatrix4x3 declaration above? That would make using TNewMatrix4x3 weird alongside the existing TMatrix4? This would be much better:

TMatrix4x3 = record
public
  const
    ColsCount = 4;
    RowsCount = 3;
  var
    Data: array [0..ColsCount - 1, 0.. RowsCount - 1] of Single;

  // See TMatrix4 declaration for more things to put here:
  // - overload operators,
  // - define Rows, Colums, Transpose, Zero, Identity
end;

3.7. Backward compatibility is important; having a consistent (easy to learn) API and useful features is even more important

Sometimes adding new things without breaking old things, in such way that the end API is consistent, is not possible. We didn’t predict everything when designing CGE API :) In this case, we have to change our API, to incorporate the new feature, such that the resulting engine API feels consistent. We don’t want features to reinvent some concepts, e.g. because some detail in the existing concept was not perfectly suitable. We want to adjust existing concepts to be better.

In this case, when the API of existing concept must change, try hard to keep the old API still available. Use deprecated Pascal directive, use it with a string parameter that explains shortly the advised alternative, like

procedure Foo; deprecated 'use Bar';

It works nicely, will display a visible warning to users that they use the deprecated feature, but at the same time Foo will continue to function normally.

But there are rare cases when providing backward compatibility is not possible. Or maybe it would be posisble, but would be a lot of work on engine development side (to make it / maintain it), or the existence of "compatibility crutch" would be too confusing to new users. In this case, after considering all the options, I say it is acceptable to break backward compatibility. You’re doing it for the greater good. The engine must have room to evolve, to be better and better, we must have a way to change old/bad/unoptimal API design into new API design. This is the only way to keep our API consistent and keep adding new features. IOW:

  • We don’t want an engine that, in 10 years, has a poor API (because new features (e.g. VR) are added to it in a weird way) and is hard to pick up by new people.

  • In exchange, we can accept the fact that, over the years, you will sometimes have to adjust your code to keep it compiling with the latest engine version.

Think about how the engine will look in a few years. Think about the perception of new people that learn the engine. If the engine has now 100 users, think that it will have 10x (1000) users in a year. We don’t want the new 900 people to suffer from bad design decisions we make today, and suffer because we could not fix these decisions out of fear of breaking backward compatibility.

Note

I am not trying to say that this reasoning applies equally to all the software. Though I think that it does apply to most of the software, actually.

You need to plan to be perfect in the future, you cannot be stuck in bad design decisions. And (no matter how smart you are) you have inevitably made some bad design decisions (which was not evident when you started, maybe even it was impossible to predict as you didn’t knew all your features, but may become evident as you move along and explore possibilities of this thing you make).

But I certainly recognize that there are cases when it’s easier or harder to break backward compatibiilty. E.g.

  • designing a file format, like X3D or glTF,

  • designing API of a library that is dynamically linked, and distributed independently of the using applications, like OpenGL. You need to keep ABI compatibility then, which is even stronger than API. Not just language level breakage is not possible, also e.g. calling convention cannot change.

In such cases, it is insanely difficult to break compatibility, because your users (in the wild) do not have automatic mechanisms that would force them to upgrade. Instead, their files / applications would suddently stop to work.

However, CGE is a library that is statically linked into your project. In the worst case, if we really break compatibility — your game will not compile after you upgrade CGE. But we don’t make it crashing for users. Developer will see that it will not compile, and will have to fix it.

4. Submitting your code contributions

It’s best to use GitHub’s pull requests.

  1. Fork the https://github.com/castle-engine/castle-engine/ . This is done by clicking on the appropriate button on GitHub.

  2. Clone your fork (i.e. download it to your local computer).

  3. Optional: Create a new branch in your fork, just for this specific feature, e.g. doing git checkout -b my-new-feature. This allows to separate your work on various CGE features.

  4. Work on your feature, committing and pushing as usual, to your branch in your fork.

  5. When ready, submit a pull request using https://github.com/castle-engine/castle-engine/pulls

See GitHub documentation (and other sites) for information about pull requests:

Advantages of pull requests:

  • They allow you to comfortably work on your pull request, committing and pushing and showing your changes to anyone. There is no need to ask for any permission to do this. (But, if you want, you can of course let us know about your work, see https://castle-engine.io/talk.php . We may be able to advise on a best way to add something to CGE.)

  • They allow us to use "code review" features of GitHub. This is a comfortable way to comment on your changes.

  • They allow everyone to submit, review and merge the changes relatively easily. And all operations can be done using the command-line or web interface, so it’s comfortable / easy / flexible for everyone.

If for some reason you really cannot follow this workflow, it is OK to simply send a traditional ".patch" file, done by "git diff" or "svn diff" (you can access https://github.com/castle-engine/castle-engine/ as a GIT or SVN repository.) You can attach it to a new issue.

5. Examples

Examples in CGE subdirectory examples should follow the snake_case naming (for their subdirectories and for the project name).

If you create a new example using editor "New Project" template, you should edit it:

  • Edit the README.md to describe the purpose of this example.

  • Edit the qualified_name of the example to be like qualified_name="io.castleengine.my.great.project".

  • Insert the standard engine copyright at the top of Pascal files, like code/gameinitialize.pas and code/gamestatemain.pas.

  • Remove the .gitignore from the example subdirectory.

  • Edit the top-level CGE .gitignore to ignore Unix binary of this executable. All other things should be already ignored by existing masks in that .gitignore.


To improve this documentation just edit the source of this page in AsciiDoctor (simple wiki-like syntax) and create a pull request to Castle Game Engine WWW (cge-www) repository.