Programming Contribution Guidelines

From acidic light community wiki

This page describes the rules you should follow when contributing to any of my code. It covers Git conventions as well as general programming guidelines on how you should style your code. If you're using an IDE like Jetbrains Rider, my coding style will be enforced by the editor.

Git conventions

Managing branches

You must create your own branch to do your work on. How this is done depends on whether you're a core contributor to one of my projects and thus have direct write access to the repo.

If you do:

  • check out a new feature branch under work/your-feature-name.
  • do your work
  • submit a merge request from your work branch into master

If you don't:

  • fork the repository to your own account
  • add your fork as a secondary remote, e.g: git remote add fork https://github.com/m88youngling/SociallyDistant
  • check out a new branch: git checkout -b your-feature-name
  • push it to your fork: git push -u fork your-feature-name
  • do your work
  • submit a merge request from your fork's branch to my repo's master branch

If you have a work/* branch and it gets merged, it will be deleted after the merge. You will need to create a new branch for future work.

Rebasing

As the journey continues, your working branch will fall behind master. If it does, it's on you to integrate master's latest work into your branch. You should rebase your branch onto latest master, resolve any conflicts, and force-push to your remote branch.

It's generally a good idea to do this every day before you start writing code, that way you're (mostly) always up-to-date with latest master.

Commit messages

I can barely read, but I do need to know what the fuck you're actually doing. Please be clear, concise and descriptive with your commit messages.

  • Use the imperative mood.
  • Use small, incremental commits.
  • Don't use emoji or emoticons.

Some examples of good commit messages are:

# fixing a bug
Fix memory leak in terminal renderer

# adding a feature
Add support for text selection in editor

# removing something
Remove broken light theme

# refactoring something
Refactor terminal rendering code

Before submitting a merge request, you should squash your commits together. This lets you turn each individual commit into a changelog. You should also write a short summary of all your changes. It should look something like this:

Improve the Socially Distant terminal emulator

 - Fix a memory leak in the terminal renderer
 - Remove broken light theme
 - Refactor terminal rendering code
 - Add support for text selection

If your merge request fixes an open issue, you should note that in the commit message at the bottom like this:

Improve the Socially Distant terminal emulator

 - Fix a memory leak in the terminal renderer
 - Remove broken light theme
 - Refactor terminal rendering code
 - Add support for text selection
 
Fixes #7420

Class Structure

When writing a class in C# or other object-oriented languages, please follow these general rules when structuring the class. If you come across code not following these guidelines, please feel free to update it!

File-level conventions

You should only have one class per file. The class name and file name must match, and the class must be in a namespace matching the directory path of the file. As an example, for a given class Foo in the directory Core/Serialization, the file name must be Foo.cs and it should look like the following code:

namespace Core.Serialization
{
    public sealed class Foo
    {
    
    }
}

This requirement ensures that your code, and thus the entire project, remains searchable by IDEs and command-line tools. This is also a requirement enforced by Unity, making it especially important when contributing to any of my games or plugins.

Inheritance

Class-based inheritance is heavily discouraged. You should avoid it at all costs unless strictly necessary. Instead, prefer encapsulation and interface-based inheritance. Design any systems such that they are completely unaware of how work is done.

To enforce this, many APIs in Socially Distant and my other projects mark casses as sealed preventing them from being derived from. This is by design, and you must design your code around this as well.

There are two appropriate use cases for class-based inheritance:

  1. When working with Unity objects like MonoBehaviour - class inheritance is strictly required by the game engine. However, the inheritance must stop beyond the first class in the hierarchy deriving from a Unity object type.
  2. The base class performs validation on external input and/or the base class holds internal state required by all derived classes. In this case, you should ensure that all derived types require and will use this shared state or behaviour. An example of this is the Socially Distant Command API where all command classes validate input in the same exact way before reaching user code.

By avoiding class-based inheritance, our systems can adapt to many more use cases and integrate with larger systems.

Member ordering

When defining your class members, please follow the below order. Doing so makes the code even more searchable.

  1. Non-public static and const fields
  2. Public static or const fields
  3. Unity serialized instance fields
  4. Non-public instance fields
  5. Public readonly fields
  6. Protected properties
  7. Public properties
  8. Constructors
  9. Unity event handler methods
  10. Public instance methods
  11. Non-public instance methods
  12. Public static methods
  13. Non-public static methods
  14. Support types (enums, structs, nested classes)

Note that there is no place for public writable fields in my code whatsoever, and I will reject any contributions that use them. There are other, better language constructs and API-specific utilities to facilitate what you may be trying to achieve by using public writable fields and you are expected to know them.

Principle of least privilege

Code I write follows the principle of least privilege, and yours should as well. Your API should only expose the least amount of access necessary for your system to be usable. (This is why public writable fields are banned in any of my code.)

You can achieve this by generally following these rules:

  1. Mark any fields that don't need to change, such as references to other objects, as private readonly. This makes it so they can only be written by your constructor, and only accessible by your class and supporting types.
  2. Mark your supporting types as private, and nest them within your class. If they need to be accessed outside of your class and cannot be exposed by an interface, then they are no longer supporting types and should be in their own file decoupled from your class.
  3. Reference types by interface when possible. This allows you to pick an interface only exposing the functionality you need, such as IReadOnlyList<T> for a list of items you only ever need to read.

Your IDE should make suggestions on the correct access modifier to use based on static analysis, you should follow its advice.

Supporting types

Sometimes, we need to represent complex state in our classes. For example, a gameplay script might need to store the state of a boss character's attacks. If you need to internally represent complex state, you are encouraged to use nested types to assist with this. However, please keep the following in mind:

  1. All supporting types for a class must be private. Do not expose them publically.
  2. If you need to expose the API of a supporting type to other types, you must do so by interface.

If a nested type can or should be used outside of the class it's defined in, it is no longer just a supporting type and should be promoted to its own file. This allows other code in the project to use that type.