FreeCAD - Code Review Guide

This document aims to provide set of good practices that should be helpful for both developers and code reviewers. They should be treated like food recipes - you can play with them, alter them - but every change should be thoughtful and intentional.

Just like the C++ practices - this document is very much inspired by the C++ Core Guidelines . You can always consult it to learn more useful stuff.

While points of this guide might not be consistently followed throughout all of the existing codebase, checking if new code follows them is possible and in long term should help us immensely.

Note

Remember that code review is a collaborative discussion. Don’t hesitate to ask for clarification or help when needed. Reviewers can also make mistakes, the goal is to work together to refine the code to a point where everyone is satisfied.

Tip

This is a tip alert blockquote.

Important

This is an important alert blockquote.

Warning

This is a warning alert blockquote.

Caution

This is a caution alert blockquote.

In this document the bolded text will indicate how important each suggestion is.

• must will be used for fundamental things that should be non-controversial and which you really should follow
• should will be used for important details that will apply for vast majority of cases, there could however be valid reasons to ignore them depending on context
• could/can will be used for best practices, things that you should try to follow but not following them is not an error per se

Common Rules

1. Consistency must be preferred over strict rule following. If, for example, in a given context code uses different naming scheme - it should be followed instead of one described in that document.
2. The aim of Code Review is not to find errors in code but to ensure code quality.
3. There are no dedicated reviewers, everyone can help with the effort!
4. Reviewers should comment mostly on added code and discuss existing one only if making change to existing code would help the new fragment.
5. Comments could be made to existing comments just to note that other (potentially better) soluyions are available and should be used instead when writing new code.
6. Reviewer can hint to make more changes to the existing code in order to improve the proposed solution (for example, refactor another class so it can be used).
7. Code must follow the Scout Rule - https://biratkirat.medium.com/step-8-the-boy-scout-rule-robert-c-martin-uncle-bob-9ac839778385 - i.e. leave code in better shape than you found it.
8. If possible, code cleanup done while working on the feature should be split into separate commits or even better PR.
9. PRs must not contain any remains of development code, like debug statements other than actual logs.
10. New code must not introduce any new linter warnings
11. Developer should have pre-commit installed and working. pre-commit-ci commits should be avoided.

PRs and Commits

1. Commit message should include prefix with module name, e.g. Part:
2. Commit message should use imperative mode - just like git does, e.g. Part: Add fillet feature

3. If changeset is big and spans across multiple modules it should be split into one commit per module.
4. Every commit in the PR must compile and run properly. git bisect workflow does not work if some commits are not possible to compile.
5. Commit message can contain additional information after one blank line.
6. Every PR should have description, even if it seems obvious.

Basic Code Rules

1. (C++ only) New code must be formatted with clang-format tool or in a way that is compatible with clang-format result if file is excluded from auto formatting.
2. (Python only) Code must follow the PEP 8 standard and formatted with Black.
3. Main execution path should be the least indented one, i.e. conditions should cover specific cases.
4. Early-Exit should be preferred to prune unwanted execution branches fast.
5. Global state (global variables, static fields, singletons) should be avoided.
6. (C++ only) enum class should be preferred over normal enum whenever possible.
7. (C++ only) C++ libraries/structures should be preferred over C ones. i.e. std::array/vector should be used instead of C array.
8. (C++ only) C++ standard library should be utilized if possible, especially the <algorithms> one.

    std::vector<int> vertices;

    std::set<int> vertexSet;
    for (auto &s : face.getSubShapes(TopAbs_VERTEX)) {
        int idx = shape.findShape(s) - 1;
        if (idx >= 0 && vertexSet.insert(idx).second) {
            vertices.push_back(idx);
        }
    }

    std::vector<int> vertices;

    std::set<int> vertexSet;
    for (auto &vertex : face.getSubShapes(TopAbs_VERTEX)) {
        int vertexId = shape.findShape(vertex);

        if (vertexId > 0) {
            vertexSet.insert(vertexId);
        }
    }


    std::copy(vertexSet.begin(), vertexSet.end(), std::back_inserter(vertices));

9. (C++ only) Return values should be preferred over out arguments. a. For methods that can fail std::optional should be used b. For methods that return multiple values it may be better to either provide dedicated struct for result or use std::tuple with expression binding.
10. (C++ only) If dealing with pointers auto must be suffixed with *: auto*.
11. (C++ only) If dealing with references auto must be suffixed with &: auto&
12. (C++ only) auto should be used if: a. the right-hand side makes clear which type it is, e.g. with static_cast, or new b. the type is long or verbose (iterators, ranges, …) c. it reduces redundancy in code (repeated type)
13. (C++ only) auto should not be used if: a. dealing with primitives (the type is not always clear, int vs unsigned etc) b. the type is not clear from context, method name or template arguments
14. If expression is not obvious - it should be given a name by using variable.

15. (C++ only) Code should not be put in anonymous namespace.
16. All members must be initialized.

17. If possible, functions / methods should not use more than 3 indentation levels.

18. (C++ only) dedicated casts should be preferred over dynamic_cast due to performance a. QObjects should use qobject_cast b. FreeCAD types (deriving from BaseClass) should use freecad_cast

Design / Architecture

1. Each class / method / function should be doing only one thing and should do it well.
2. Classes that do provide business logic should be stateless. This helps with reusability.
3. Functions / Methods should be pure i.e. their result should only depend on arguments (and object state in case of method). This helps with reusability, predictability and testing.
4. Immutable data should be used whenever possible, (For C++ use const)

5. (C++ only) If possible constexpr should be used for storing constant data.
6. Long methods should be split into smaller, better described ones.
7. (C++ only) Defining new macros must be avoided, unless absolutely necessary.
8. Integers must not be used to express anything other than numbers. For enumerations enums must be used.
9. Code should be written in a way that it expresses intent, i.e. what should be done, rather than just how it is done.

    void setOverlayMode(OverlayMode mode)
    {
        // ... some code ...

        QDockWidget *dock = nullptr;

        for (auto w = qApp->widgetAt(QCursor::pos()); w; w = w->parentWidget()) {
            dock = qobject_cast<QDockWidget*>(w);
            if (dock) {
                break;
            }
            auto tabWidget = qobject_cast<OverlayTabWidget*>(w);
            if (tabWidget) {
                dock = tabWidget->currentDockWidget();
                if (dock) {
                    break;
                }
            }
        }

        if (!dock) {
            for (auto w = qApp->focusWidget(); w; w = w->parentWidget()) {
                dock = qobject_cast<QDockWidget*>(w);
                if (dock) {
                    break;
                }
            }
        }

        // some more code ...

        toggleOverlay(dock, m);
    }

    void setOverlayMode(OverlayMode mode)
    {
        // ... some code ...

        QDockWidget *dock = findClosestDockWidget();

        // ... some more code ...

        toggleOverlay(dock, m);
    }

auto findClosestDockWidget = []() { ... }

10. Boolean arguments must be avoided. Use enumerations instead - enum with 2 values is absolutely fine. For python boolean arguments are ok, but they must forced to be keyword ones.

mapper.populate(false, it.key(), it.value());

mapper.populate(mappingstatus::modified, it.key(), it.value());

11. (C++ only) Code should prefer uniform initialization { } (also called brace initialization). Prevents narrowing.
12. (C++ only) Class member variables should be initialized at declaration, not in constructors
13. Magic numbers or other literals must be avoided.

Naming Things

Naming described in that section of the document refers mostly to newer parts of code. Some modules use different naming conventions, while checking the code Reviewer should take into account surrounding code and first of all ensure consistency within the closest scope.

1. Code symbols (classes, structs, methods, functions, variables…) must have names that are meaningful and grammatically correct.
2. Variables should not be named using abbreviations and/or 1 letter names. Iterator variables or math related ones like i or u are obviously not covered by this rule.
3. Names must not use the hungarian notation.
4. (C++ only) Classes/Structs/Enums must be written in PascalCase, underscores are allowed but should be avoided.
5. (C++ only) Class/Struct members should be written in camelCase, underscores are allowed but should be avoided.
6. (C++ only) Global functions should be written in camelCase, underscores are allowed but should be avoided.
7. (C++ only) Enum cases should use PascalCase
8. (C++ only) Enums should use singular form (i.e. enum ObjectState instead of enum ObjectStates)
9. (C++ only) Local variables should use camelCase

Naming Conventions

1. Coin nodes should be prefixed with So, e.g. SoTransformDragger. Reimplementation of existing Coin nodes should use SoFC prefix, e.g. SoFCTransform.
2. Features in Part and Part Design workbenches should be prefixed with Feature, e.g. FeatureHole.
3. View Providers corresponding to features / other objects should be prefixed ViewProvider, e.g. ViewProviderHole
4. Task Views (content for task panels) should be prefixed with Task, e.g. TaskTransform
5. Task Dialog for Task View should be additionally suffixed with Dialog, e.g. TaskTransformDialog
6. Preference Pages should be prefixed with DlgSettings, e.g. DlgSettingsLights.
7. Commands should be prefixed with Cmd and Workbench name e.g. CmdPartDesignCreateSketch

Commenting the code

1. Good naming must be preferred over commenting the code.
2. Comments that describe what code does should be avoided, instead comments should explain intended result.
3. All edge-cases in code must be described with comment describing when such edge-case can occur and why it is handled in certain way.
4. All “Hacks” must be described with how the hack works, why it is applied and when it no longer will be needed.
5. Commented code must be contain additional information on why it was commented out and when it is safe to remove it.