Why do we need Guidelines?

Developing code in even moderately sized projects is challenging. This project is not exempt from this rule, since for many students this might be the first bigger project that they work on. Due to the high developer turnover, maintaining high code quality is of utmost importance for the future generation of developers who are entering the project. Especially for semester projects or master theses, which have a relatively tight timeline, clean code can lessen the initial barrier to entry substantially.

The following guidelines have been adapted from our local ETH Robocup SPL team but its relevancy still applies to this project.

C++

Style

NOTE: please take some time to read carefully this section to streamline reviews

We largely follow the Google C++ Style Guide, especially the following items:

• Header files
  • Include what you use
• Scoping
  • Names and order of includes
  • Namespaces
• Classes
  • Doing work in constructors
  • Structs vs Classes
  • Inheritance
• Functions
  • Inputs And Outputs
  • Write short functions
• Naming
  • General Naming Rules
  • Type Names
  • Variable Names
  • Namespace Names
  • Enumerator Names
• Comments

The most important differences, i.e. where we slightly diverge from Google C++ Style Guide, are summarized below.

Standard

Currently, all new code should target C++17, i.e. it should not use C++20 features. Therefore, when creating new packages, make sure to set the right version of the standard in the top-level CMakeLists.txt

File Names

Filenames should be all lowercase and can include underscores (_).
C++ files should end in .cpp and header files should end in .h.

Header files

Every .cpp file should have a corresponding .h. Some exceptions to this rule include:

• Unit tests
• Small .cpp files containing a main().

Function names

We use lowerCamelCase for function names i.e.:

void function(); // good
void aVeryLongFunction();  // good

void a_short_function(); // wrong
void AnotherShortFunction() ; // wrong

Constants

Upper case snake case, e.g. OUT_OF_MEMORY, must be used for all constants, including:

• Enums

enum AlternateUrlTableError {
  OK = 0,
  OUT_OF_MEMORY = 1,
  MALFORMED_INPUT = 2,
};

• Scoped enums

enum class AlternateUrlTableError: uint8_t {
  OK = 0,
  OUT_OF_MEMORY = 1,
  MALFORMED_INPUT = 2,
};

• Class-level constants

class CameraDevice {
private:
  // vanilla const
  inline static const std::string DEVICE_NAME = "/dev/video";

  // constexpr
  static constexpr uint32_t NUM_OF_BUFFERS = 3;
}

• Global variables

const std::vector<int> TEST_VALUES = {1, 2, 3};

TEST(ValuesTest, TestFailure){
  // some test using TEST_VALUE
}

Formatting

We use clang-format as our default formatter. The codebase provides a .clang-format file, indicating the style we use. Most modern text editors/IDEs support format on save and it is encouraged to enable this feature. Furthermore, the pre-commit hooks also check/apply the formatting which is necessary for the CI to succeed. For installation instructions see here

ROS

CRS currently uses ROS Noetic Ninjemys.

Prerequisites

If you are not familiar with ROS, please go through the beginner and intermedtiate tutorials from the official documentation of ROS noetic. Please don't just blindly copy-paste the example code and make sure you fully understand the core concepts and tools, especially:

• Nodes, topics, parameters
• Messages and Publishers / Subscribers
• Services and Servers / Clients
• Actions and Action Servers / Action Clients
• Launch files
• Main CLI and graphical tools: e.g.:
  • ros[topic,node,run,launch]
  • rviz

Package organization

From [an old but still relevant answer on the ROS forum]:

• The overhead of a ROS package is not large. Define separate packages wherever they make sense. Often, code can be useful in contexts other than those for which it was built.
• Avoid combining nodes that pull in mutually unneeded dependencies and are often used separately (to eliminate unnecessary build overhead).
• The package dependency graph must be acyclic, i.e. no package may depend on another that directly or indirectly depends on it.
• If programs with similar dependencies are generally used together, consider combining them into a single package.
• If some nodes have common dependencies on shared code that you do not wish to export publicly, they can be combined internally within a single package.
• Create separate packages that contain only messages, services and actions (separation of interface and implementation). Examples for separate message packages are the ros/common_msgs packages.
• Group packages in stacks.

Git

Prerequisites

If you are unfamiliar with git, please go through a good tutorial (for example this one) to understand the basics. Whatever resource you choose, you should know the following things:

• What these keywords mean within the context of git: staging commit branch merge rebase push pull
• Understand what is changed on your local copy i.e. understand the output of git status
• Commit new changes
• Inspect what branch you are on
• Inspect the commit history of a branch
• How merge works
• How rebase works

General rules

• Avoid long-lived feature branches: pull requests (PRs) with many changes across tens of files are pretty much impossible to review and should thus be split into multiple, incremental ones (exception to this are of course MT and SP).
• Limit the scope of your changes: if you are working on, for example, adding a new package for a perception feature, and fix an unrelated bug or make an unrelated change in the process, create a separate PR for that to keep track of all such changes in the commit history.

Branching workflow

We roughly follow the GitHub flow.
The recommended workflow is summarized here:

1. For any new task, create a new branch from the latest master - give it a meaningful name.
2. Make changes on your local branch. Commit often.
3. When you are finished, make sure that your changes are compliant with the style guidelines in this page and open a pull request. Make sure all checks in the CI pipeline pass before adding a reviewer.
4. Address all the review comments until your PR is approved.
5. Merge your PR. In this repo, only squash merge (recommended) and rebase merge are allowed. All commits which will be merged in master should respect the commit message convention below, so if you choose squash merge, the squash commit message should be compliant, and if you choose rebase merge, all the commits should be compliant.

Pull Request Naming Conventions

In general, we want to aim for the following format [package_changed] brief description of what has been changed. This gives every viewer all the necessary information of what packages or what part of the code has been/is affected by a PR at a glance without the mental overhead of trying to understand ambiguous PR names. Furthermore, it makes it very easy to group PRs by area of concern.

Some caveats:

• Absolutely avod just using the branch name in the PR title
• Exhaustive descriptions belong in the PR description. Try and keep the PR titles as short and concise as possible.
• Always assume the reviewer is an idiot. Its possible that someone will review your code who isn't fully aware of your project and you therefore have to provide at least some meaningful guidance in you description as to why you are doing what you are doing, how you are doing it and how the change has been tested/validated.
• If a PR targets multiple packages, consider whether you can break the PR down into smaller PRs. Smaller code changes are easier to track and digest
• If you absolutely have to change multiple packages or your PR does not target one specific package (e.g. if you're working on some tooling or the build process) try and find a substitute for the package name which makes sense. E.g. [WorkpackageName], [Build Process], etc.

Commit message convention

No need to reinvent the wheel here, quoting from Linus Torvalds, a good commit message should look like this:

Header line: explain the commit in one line (use the imperative)

Body of commit message is a few lines of text, explaining things
in more detail, possibly giving some background about the issue
being fixed, etc.

The body of the commit message can be several paragraphs, and
please do proper word-wrap and keep columns shorter than about
74 characters or so. That way "git log" will show things
nicely even when it's indented.

Make sure you explain your solution and why you're doing what you're
doing, as opposed to describing what you're doing. Reviewers and your
future self can read the patch, but might not understand why a
particular solution was implemented.

Testing

While testing cannot ensure that no bugs are introduces, it's certainly a very good way to prevent regressions in existing code. If you are unfamiliar with the concept of testing for software, this SO answer provides a short and concise definition.

Unit testing

While reaching 100% coverage is not and should not be our goal with testing, unit tests should be added for all code which implements non trivial logic, especially if it is "mission critical". In general, it's a good idea to write code in such a way that testing should come easy, for example by using techniques such as composition and dependency injection.

• C++ For C++, we use GoogleTest (Gtest), with GoogleMock for mocking. If you are not familiar with either of these two frameworks, please go through their official documentation:

• GoogleTest Primer

• GoogleMock Cook Book