- Submitting code:
  It's best to use GitHub's pull requests.

  - Clone the https://github.com/castle-engine/castle-engine/
    (or just make a new branch inside it, if you have write-access to CGE repository.)
  - Work on your private clone/branch, comming and pushing.
  - When read, submit a pull request using on
    https://github.com/castle-engine/castle-engine/pulls

  You will find a lot of instructions how to use this on GitHub
  (and other places Google can find).

  Advantages:
  This allows you to comfortably work on your pull request, committing and pushing
  and showing your changes to anyone.
  This allows us to use code review features of GitHub, that are comfortable to comment
  on your code changes.
  This allows to submit and merge the changes 100% automatically,
  using the command-line or web interface, so it's usually less error-prone.

  Alternative:
  Simply send a traditional ".patch" file, done by "git diff"
  (versus GIT repository on https://github.com/castle-engine/castle-engine/ )
  or "svn diff" (versus SVN repository on https://sourceforge.net/projects/castle-engine/ ).

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

  These are documented nicely on:
  - http://edn.embarcadero.com/article/10280 - Object Pascal Style Guide
  - http://kodu.ut.ee/~jellen/delphi/cs.html - Delphi Language Coding Standards Document
    (Not available anymore? Access through Web Archive:
    https://web.archive.org/web/20170607183644/http://kodu.ut.ee/~jellen/delphi/cs.html
    )

  In particular:
  - Indent by 2 spaces.
  - Use CamelCase for everything,
    - including constants, so MyConstant instead of MY_CONSTANT,
    - and local variables, even "I" instead of "i".
  - Put "begin" on a separate line.
    I.e. 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 avoid verbosity, it's OK to omit begin/end for statements:
    ~~~~
    // THIS IS EVEN BETTER:
    for I := 1 to 10 do
      Writeln(I);
    ~~~~
  - 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 for a long time,
    // until it was pointed to him that it's not optimal,
    // and most e.g. Lazarus code doesn't use it):
    // Try not to use this in new code, but don't be surprised if it still occurs somewhere.
    // We gradually get rid of it.
    if Foo then
      Bar else
      Xyz;
    ~~~
  - Never use tabs (convert to spaces).
  - Never leave trailing whitespace at the end of lines (in the long run,
    it causes unnecessary diffs when someone removes it).
  - Never use "with".
    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, but 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.
  - 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, CastleSceneManager,
      GameStatePlay;
    ```
  - 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. It's not perfect -- `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;
    ```

- File extensions:

  *.pas files are units,
  *.inc are files to be included in other Pascal source files using $I
        (short for $Include).
  *.dpr are main program files.
        This changes in CGE >= 6.3.
        In CGE < 6.3, program files had .lpr extension, since we used only Lazarus.
        In CGE >= 6.3, we use both Lazarus and Delphi.
        While Lazarus accepts either .dpr or .lpr extension for the program file,
        Delphi tolerates only .dpr extension. So we have to (unfortunately)
        adjust to Delphi, and just use .dpr.

  Do not use *.pp (not familiar to people from Delphi).

- The engine is, in general, not thread-safe.
  You cannot call our functions from different threads.

  Reasons:
  - We use some global caches, and securing access to them from multiple
    threads would cost us speed (and make code more complex).
  - OpenGL must be operated from a single thread anyway.

  There are some things that in practice can be safely used from multiple
  threads now (some image and file loading, some OpenAL operations),
  but please don't depend on it. Unless something is clearly documented
  as "thread-safe", DO NOT assume it.

- 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 EnumFiles can call EnumFiles inside
  it's own implementation.

- 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
    flavours:

    ~~~~
    function DoSomething(const X: <type>, ...):<type>;
    procedure DoSomethingVar(var X: <type>,...);
    ~~~~

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

    See e.g. CastleVectors and CastleImages units.

    This rule doesn't apply when <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.

  - If somewhere I use parameters like V: ^<type> and Stride: Integer
    then it means that these parameters define a table of <type> values.
    Address of 1st item is V, address of i-th is (V + i * Stride).
    This is a convention used by OpenGL's vertex array routines.

    Stride may be negative. Stride may also be 0, then it means
    that Stride = SizeOf(<type>).

- Compilation symbols used:

  Standard FPC and Delphi ones: MSWINDOWS, UNIX, LINUX,
  CPUI386, CPUX86_64, FPC to differentiate between compiler versions,
  and some more.

  See castleconf.inc.

  We also use DEBUG symbol.
  Also castle-fpc.cfg adds some checks when -dDEBUG.
  The build tool when compiled in debug mode (--mode=debug) also defines
  DEBUG, and adds some checks.
  You can use 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.
  So there's no need to check for RELEASE symbol.

- Exceptions' messages:

  - Do not start them with 'Error: ' or 'Error - ' or anything else
    that just says "we have an error".
    This would be redundant, it would be necessary for *almost all* exception messages,
    since almost all signal some error.
    So don't say it -- the fact that you're raising an Exception
    already signals that this is some kind of error.

  - Don't end the Message with '!' character.
    All error messages signal that something bad happened,
    so '!' would be necessary for *almost all* exception messages if you would
    follow this.
    Instead, keep the cold blood, and keep the error message "clean and calm".

  - Usually, Message should be a single sentence,
    and not end with the '.' character.
    We do not follow this rule 100%, it's OK to break it with good reasons.

  - 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.
    I 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 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.)
    In normal situation, the code that finally catched and outputs
    this exception should show such information.

- Callbacks: "of object" or not?

  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.

- Other languages:
  - Indent by 4 spaces in Java and Objective-C.
  - In general, we never use tabs in our sources (unless the language requires them, like for `Makefile`).

- Optimization guidelines:

  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 "shadows"
  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.

  (Exception: in the rare cases when the optimized code is 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'm biased to "think more about code quality",
  mostly because I see many people (including myself) to often think to little
  of code quality (and fall into the trap "let's optimize this").

  Of course, there is a balance. We want the code to be fast and use little memory
  -- there are various ways to achieve this, often using smart algoriths on CPU,
  and/or thinking about how the CPU cache is used,
  and/or delivering data in better chunks to GPU.

  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.

  Bottom line 2:
  - It depends:) I like the IBM motto: "Think".

Michalis Kamburelis (aka Kambi)
<michalis.kambi@gmail.com>
http://castle-engine.sourceforge.net/